Make tasks local only.

prdoc/pr_10162.prdoc

@@ -0,0 +1,26 @@
+title: Make tasks local only.
+doc:
+- audience:
+  - Runtime Dev
+  - Node Dev
+  description: |-
+    Related to https://github.com/paritytech/polkadot-sdk/issues/9693
+
+    In the transaction pool, transaction are identified by the tag they provide.
+
+    For tasks the provided tag is simply the hash for the encoded task.
+
+    Nothing in the doc says that implementers should be careful that tasks are not too many for a single operation. What I mean is if a task is `migrate_keys(limit)`, with valid first from 1 to 10_000. Then all tasks `migrate_keys(1)`, `migrate_keys(2)` ... `migrate_keys(10_000)` are valid and effectively do the same operation: they all migrate part of the keys.
+    In this case a malicious person can submit all those tasks at once and spam the transaction pool with 10_000 transactions.
+
+    I see multiple solution:
+    * (1) we are careful when we implement tasks, we make the doc clear, but the API is error prone. (in my example above we would implement just `migrate_keys` and inside the call we would do a basic rate of migration of 1000 keys in a bulk).
+    * (2) we have a new value returned that is the provided tag for the task. Or we use the task index as provided tag.
+    * (3) we only accept local tasks: <-- implemented in this PR.
+
+    maybe (2) is a better API if we want external submission of tasks.
+crates:
+- name: frame-support-procedural
+  bump: major
+- name: frame-system
+  bump: major

substrate/frame/support/procedural/src/lib.rs

@@ -1120,10 +1120,9 @@ pub fn composite_enum(_: TokenStream, _: TokenStream) -> TokenStream {
 	pallet_macro_stub()
 }
 
-/// Allows you to define some service work that can be recognized by a script or an
-/// off-chain worker.
+/// Allows you to define some service work that can be recognized by the off-chain worker.
 ///
-/// Such a script can then create and submit all such work items at any given time.
+/// The off-chain worker can then create and submit all such work items at any given time.
 ///
 /// These work items are defined as instances of the `Task` trait (found at
 /// `frame_support::traits::Task`). [`pallet:tasks_experimental`](macro@tasks_experimental) when
@@ -1140,11 +1139,11 @@ pub fn composite_enum(_: TokenStream, _: TokenStream) -> TokenStream {
 /// All of such Tasks are then aggregated into a `RuntimeTask` by
 /// [`construct_runtime`](macro@construct_runtime).
 ///
-/// Finally, the `RuntimeTask` can then used by a script or off-chain worker to create and
-/// submit such tasks via an extrinsic defined in `frame_system` called `do_task`.
+/// Finally, the `RuntimeTask` can then be used by the off-chain worker to create and
+/// submit such tasks via an extrinsic defined in `frame_system` called `do_task` which accepts
+/// unsigned transaction from local source.
 ///
-/// When submitted as unsigned transactions (for example via an off-chain workder), note
-/// that the tasks will be executed in a random order.
+/// When submitted as unsigned transactions, note that the tasks will be executed in a random order.
 ///
 /// ## Example
 #[doc = docify::embed!("examples/proc_main/tasks.rs", tasks_example)]

substrate/frame/support/test/tests/tasks.rs

@@ -133,3 +133,23 @@ fn tasks_work() {
 		assert_eq!(my_pallet_2::SomeStorage::<Runtime, Instance1>::get(), (0, 2));
 	});
 }
+
+#[test]
+fn do_task_unsigned_validation_rejects_external_source() {
+	new_test_ext().execute_with(|| {
+		use frame_support::pallet_prelude::{
+			InvalidTransaction, TransactionSource, TransactionValidityError, ValidateUnsigned,
+		};
+
+		let task = RuntimeTask::MyPallet(my_pallet::Task::<Runtime>::Foo { i: 0u32, j: 2u64 });
+		let call = frame_system::Call::do_task { task };
+
+		assert!(matches!(
+			System::validate_unsigned(TransactionSource::External, &call),
+			Err(TransactionValidityError::Invalid(InvalidTransaction::Call))
+		));
+
+		assert!(System::validate_unsigned(TransactionSource::InBlock, &call).is_ok());
+		assert!(System::validate_unsigned(TransactionSource::Local, &call).is_ok());
+	});
+}

substrate/frame/system/src/lib.rs

@@ -1136,7 +1136,7 @@ pub mod pallet {
 	#[pallet::validate_unsigned]
 	impl<T: Config> sp_runtime::traits::ValidateUnsigned for Pallet<T> {
 		type Call = Call<T>;
-		fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity {
+		fn validate_unsigned(source: TransactionSource, call: &Self::Call) -> TransactionValidity {
 			if let Call::apply_authorized_upgrade { ref code } = call {
 				if let Ok(res) = Self::validate_code_is_authorized(&code[..]) {
 					if Self::can_set_code(&code, false).is_ok() {
@@ -1153,17 +1153,29 @@ pub mod pallet {
 
 			#[cfg(feature = "experimental")]
 			if let Call::do_task { ref task } = call {
-				if task.is_valid() {
-					return Ok(ValidTransaction {
-						priority: u64::max_value(),
-						requires: Vec::new(),
-						provides: vec![T::Hashing::hash_of(&task.encode()).as_ref().to_vec()],
-						longevity: TransactionLongevity::max_value(),
-						propagate: true,
-					})
+				// If valid, the tasks provides the tag: hash of task.
+				// But it is allowed to have many task for a single process, e.g. a task that takes
+				// a limit on the number of item to migrate is valid from 1 to the limit while
+				// actually advancing a single migration process.
+				// In the transaction pool, transaction are identified by their provides tag.
+				// So in order to protect the transaction pool against spam, we only accept tasks
+				// from local source.
+				if source == TransactionSource::InBlock || source == TransactionSource::Local {
+					if task.is_valid() {
+						return Ok(ValidTransaction {
+							priority: u64::max_value(),
+							requires: Vec::new(),
+							provides: vec![T::Hashing::hash_of(&task.encode()).as_ref().to_vec()],
+							longevity: TransactionLongevity::max_value(),
+							propagate: false,
+						})
+					}
 				}
 			}
 
+			#[cfg(not(feature = "experimental"))]
+			let _ = source;
+
 			Err(InvalidTransaction::Call.into())
 		}
 	}