Frame fixes medium low

frame/metadata/src/lib.rs

@@ -83,11 +83,31 @@ pub mod v16;
 pub const META_RESERVED: u32 = 0x6174656d; // 'meta' warning for endianness.
 
 /// Metadata prefixed by a u32 for reserved usage
+///
+/// # Decoding untrusted input
+///
+/// [`Decode`] rejects blobs whose prefix is not [`META_RESERVED`] after reading only the first
+/// four bytes. Beyond that, decoding materializes unbounded owned containers (`Vec`s, maps,
+/// strings) whose size is proportional to the input, with no schema-level caps: callers decoding
+/// metadata from an untrusted source must bound the input size *before* decoding.
 #[derive(Eq, Encode, PartialEq, Debug)]
-#[cfg_attr(feature = "decode", derive(Decode))]
 #[cfg_attr(feature = "serde_full", derive(Serialize))]
 pub struct RuntimeMetadataPrefixed(pub u32, pub RuntimeMetadata);
 
+#[cfg(feature = "decode")]
+impl Decode for RuntimeMetadataPrefixed {
+	fn decode<I: Input>(input: &mut I) -> Result<Self, Error> {
+		// Validate the reserved prefix *before* decoding the payload, so a blob that is not
+		// runtime metadata is rejected after reading four bytes instead of after fully
+		// materializing an attacker-sized metadata structure.
+		let prefix = u32::decode(input)?;
+		if prefix != META_RESERVED {
+			return Err("Invalid metadata prefix: expected `META_RESERVED` ('meta')".into())
+		}
+		Ok(Self(prefix, RuntimeMetadata::decode(input)?))
+	}
+}
+
 impl From<RuntimeMetadataPrefixed> for Vec<u8> {
 	fn from(value: RuntimeMetadataPrefixed) -> Self {
 		value.encode()
@@ -272,4 +292,19 @@ mod test {
 			Decode::decode(&mut load_metadata(14).as_slice()).unwrap();
 		assert!(matches!(meta.1, RuntimeMetadata::V14(_)));
 	}
+
+	#[test]
+	fn should_reject_invalid_prefix_before_decoding_payload() {
+		// A wrong magic followed by a huge claimed payload: the prefix check must fail after
+		// the first four bytes, without attempting to materialize the payload.
+		let mut blob = 0xdeadbeef_u32.encode();
+		blob.extend_from_slice(&load_metadata(14)[4..]);
+		let res = RuntimeMetadataPrefixed::decode(&mut blob.as_slice());
+		assert!(res.is_err());
+
+		// A valid prefix still decodes.
+		let meta: RuntimeMetadataPrefixed =
+			Decode::decode(&mut load_metadata(14).as_slice()).unwrap();
+		assert_eq!(meta.0, META_RESERVED);
+	}
 }

frame/support-procedural/src/construct_runtime/expand/call.rs

@@ -162,7 +162,10 @@ pub fn expand_outer_dispatch(
 							<<#pallet_names as Callable<#runtime>>::RuntimeCall
 								as GetCallName>::get_call_names(),
 					)*
-					_ => unreachable!(),
+					// An unknown module name is a recoverable "not found" rather than an
+					// impossible state: this is public metadata API reachable with
+					// caller-supplied strings, so return an empty slice instead of panicking.
+					_ => &[],
 				}
 			}
 		}

frame/support-procedural/src/construct_runtime/expand/task.rs

@@ -98,7 +98,9 @@ pub fn expand_outer_task(
 
 		#[automatically_derived]
 		impl #scrate::traits::Task for RuntimeTask {
-			type Enumeration = #prelude::IntoIter<RuntimeTask>;
+			type Enumeration = #scrate::__private::Box<
+				dyn #prelude::Iterator<Item = RuntimeTask>
+			>;
 
 			fn is_valid(&self) -> bool {
 				match self {
@@ -141,12 +143,18 @@ pub fn expand_outer_task(
 			}
 
 			fn iter() -> Self::Enumeration {
-				let mut all_tasks = Vec::new();
+				// Stream the pallet task iterators lazily instead of collecting them into
+				// heap vectors: task sets can be derived from attacker-growable state, so
+				// eager materialization allows unbounded allocation before the first yield.
+				let mut all_tasks =
+					#scrate::__private::Vec::<Self::Enumeration>::new();
 				#(
 					#cfg_attrs
-					all_tasks.extend(<#task_types>::iter().map(RuntimeTask::from).collect::<Vec<_>>());
+					all_tasks.push(#scrate::__private::Box::new(
+						<#task_types>::iter().map(RuntimeTask::from),
+					));
 				)*
-				all_tasks.into_iter()
+				#scrate::__private::Box::new(all_tasks.into_iter().flatten())
 			}
 		}
 

frame/support-procedural/src/pallet/expand/hooks.rs

@@ -214,24 +214,16 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream {
 			for #pallet_ident<#type_use_gen> #where_clause
 		{
 			fn before_all_runtime_migrations() -> #frame_support::weights::Weight {
-				use #frame_support::traits::{Get, PalletInfoAccess};
-				use #frame_support::__private::hashing::twox_128;
-				use #frame_support::storage::unhashed::contains_prefixed_key;
 				#frame_support::__private::sp_tracing::enter_span!(
 					#frame_support::__private::sp_tracing::trace_span!("before_all")
 				);
 
-				// Check if the pallet has any keys set, including the storage version. If there are
-				// no keys set, the pallet was just added to the runtime and needs to have its
-				// version initialized.
-				let pallet_hashed_prefix = <Self as PalletInfoAccess>::name_hash();
-				let exists = contains_prefixed_key(&pallet_hashed_prefix);
-				if !exists {
-					#initialize_on_chain_storage_version
-					<T as #frame_system::Config>::DbWeight::get().reads_writes(1, 1)
-				} else {
-					<T as #frame_system::Config>::DbWeight::get().reads(1)
-				}
+				// The on-chain storage version of a newly added pallet is initialized
+				// *after* all migrations ran (see `on_runtime_upgrade` below). Seeding it
+				// here would run before the runtime's custom migrations and cause
+				// version-gated migrations targeting a new (still empty) pallet prefix,
+				// e.g. renames or imports from another namespace, to be silently skipped.
+				#frame_support::weights::Weight::zero()
 			}
 		}
 
@@ -240,18 +232,36 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream {
 			for #pallet_ident<#type_use_gen> #where_clause
 		{
 			fn on_runtime_upgrade() -> #frame_support::weights::Weight {
+				use #frame_support::traits::Get;
 				#frame_support::__private::sp_tracing::enter_span!(
 					#frame_support::__private::sp_tracing::trace_span!("on_runtime_update")
 				);
 
 				// log info about the upgrade.
 				#log_runtime_upgrade
 
-				<
+				let mut weight = <
 					Self as #frame_support::traits::Hooks<
 						#frame_system::pallet_prelude::BlockNumberFor::<T>
 					>
-				>::on_runtime_upgrade()
+				>::on_runtime_upgrade();
+
+				// If the storage-version key is still absent after every migration had a
+				// chance to run, the pallet is genuinely new: initialize its on-chain
+				// storage version. Doing this after the migrations (instead of in
+				// `before_all_runtime_migrations`) keeps version-gated migrations that
+				// target a new pallet prefix from being skipped.
+				if !#frame_support::traits::StorageVersion::exists::<Self>() {
+					#initialize_on_chain_storage_version
+					weight = weight.saturating_add(
+						<T as #frame_system::Config>::DbWeight::get().reads_writes(1, 1)
+					);
+				} else {
+					weight = weight
+						.saturating_add(<T as #frame_system::Config>::DbWeight::get().reads(1));
+				}
+
+				weight
 			}
 
 			#frame_support::try_runtime_enabled! {

frame/support-procedural/src/pallet/expand/storage.rs

@@ -66,7 +66,10 @@ fn check_prefix_duplicates(
 		return Err(err);
 	}
 
-	if let Metadata::CountedMap { .. } = storage_def.metadata {
+	// Counted maps and counted N-maps both generate an auxiliary counter storage instance under
+	// the derived `CounterFor...` prefix, so that namespace must be reserved for both kinds;
+	// otherwise another storage item could silently alias the counter's physical key.
+	if let Metadata::CountedMap { .. } | Metadata::CountedNMap { .. } = storage_def.metadata {
 		let counter_prefix = counter_prefix(&prefix);
 		let counter_dup_err = syn::Error::new(
 			storage_def.prefix_span(),

frame/support-procedural/src/pallet/expand/tasks.rs

@@ -166,15 +166,22 @@ impl TasksDef {
 
 			impl #impl_generics #frame_support::traits::Task for #enum_use
 			{
-				type Enumeration = #frame_support::__private::IntoIter<#enum_use>;
+				type Enumeration = #frame_support::__private::Box<
+					dyn #frame_support::traits::tasks::__private::Iterator<Item = #enum_use>
+				>;
 
 				fn iter() -> Self::Enumeration {
-					let mut all_tasks = #frame_support::__private::vec![];
-					#(all_tasks
-						.extend(#task_iters.map(|(#(#task_arg_names),*)| #enum_ident::#task_fn_idents { #(#task_arg_names: #task_arg_names.clone()),* })
-						.collect::<#frame_support::__private::Vec<_>>());
+					// Stream the task lists lazily instead of collecting them into heap
+					// vectors: task lists can be derived from attacker-growable state, so
+					// eager materialization allows unbounded allocation before the first
+					// yield.
+					let mut all_tasks =
+						#frame_support::__private::Vec::<Self::Enumeration>::new();
+					#(all_tasks.push(#frame_support::__private::Box::new(
+						#task_iters.map(|(#(#task_arg_names),*)| #enum_ident::#task_fn_idents { #(#task_arg_names: #task_arg_names.clone()),* })
+					));
 					)*
-					all_tasks.into_iter()
+					#frame_support::__private::Box::new(all_tasks.into_iter().flatten())
 				}
 
 				fn task_index(&self) -> u32 {

frame/support-procedural/src/pallet/parse/call.rs

@@ -144,12 +144,26 @@ impl syn::parse::Parse for FunctionAttr {
 		content.parse::<keyword::pallet>()?;
 		content.parse::<syn::Token![::]>()?;
 
+		// Reject attributes with unparsed trailing tokens instead of silently ignoring
+		// them: attributes like `#[pallet::weight(...)]` define security-critical dispatch
+		// metadata, so accepting only a leading prefix of the attribute could silently
+		// underprice a call.
+		fn ensure_empty(stream: syn::parse::ParseStream) -> syn::Result<()> {
+			if stream.is_empty() {
+				Ok(())
+			} else {
+				Err(stream.error("unexpected trailing tokens in pallet attribute"))
+			}
+		}
+
 		let lookahead = content.lookahead1();
-		if lookahead.peek(keyword::weight) {
+		let attr = if lookahead.peek(keyword::weight) {
 			content.parse::<keyword::weight>()?;
 			let weight_content;
 			syn::parenthesized!(weight_content in content);
-			Ok(FunctionAttr::Weight(weight_content.parse::<syn::Expr>()?))
+			let weight = weight_content.parse::<syn::Expr>()?;
+			ensure_empty(&weight_content)?;
+			FunctionAttr::Weight(weight)
 		} else if lookahead.peek(keyword::call_index) {
 			content.parse::<keyword::call_index>()?;
 			let call_index_content;
@@ -159,33 +173,40 @@ impl syn::parse::Parse for FunctionAttr {
 				let msg = "Number literal must not have a suffix";
 				return Err(syn::Error::new(index.span(), msg));
 			}
-			Ok(FunctionAttr::CallIndex(index.base10_parse()?))
+			ensure_empty(&call_index_content)?;
+			FunctionAttr::CallIndex(index.base10_parse()?)
 		} else if lookahead.peek(keyword::feeless_if) {
 			content.parse::<keyword::feeless_if>()?;
 			let closure_content;
 			syn::parenthesized!(closure_content in content);
-			Ok(FunctionAttr::FeelessIf(
-				closure_content.span(),
-				closure_content.parse::<syn::ExprClosure>().map_err(|e| {
-					let msg = "Invalid feeless_if attribute: expected a closure";
-					let mut err = syn::Error::new(closure_content.span(), msg);
-					err.combine(e);
-					err
-				})?,
-			))
+			let closure = closure_content.parse::<syn::ExprClosure>().map_err(|e| {
+				let msg = "Invalid feeless_if attribute: expected a closure";
+				let mut err = syn::Error::new(closure_content.span(), msg);
+				err.combine(e);
+				err
+			})?;
+			ensure_empty(&closure_content)?;
+			FunctionAttr::FeelessIf(closure_content.span(), closure)
 		} else if lookahead.peek(keyword::authorize) {
 			content.parse::<keyword::authorize>()?;
 			let closure_content;
 			syn::parenthesized!(closure_content in content);
-			Ok(FunctionAttr::Authorize(closure_content.parse::<syn::Expr>()?))
+			let expr = closure_content.parse::<syn::Expr>()?;
+			ensure_empty(&closure_content)?;
+			FunctionAttr::Authorize(expr)
 		} else if lookahead.peek(keyword::weight_of_authorize) {
 			content.parse::<keyword::weight_of_authorize>()?;
 			let closure_content;
 			syn::parenthesized!(closure_content in content);
-			Ok(FunctionAttr::WeightOfAuthorize(closure_content.parse::<syn::Expr>()?))
+			let expr = closure_content.parse::<syn::Expr>()?;
+			ensure_empty(&closure_content)?;
+			FunctionAttr::WeightOfAuthorize(expr)
 		} else {
-			Err(lookahead.error())
-		}
+			return Err(lookahead.error());
+		};
+
+		ensure_empty(&content)?;
+		Ok(attr)
 	}
 }
 

frame/support-procedural/src/pallet/parse/tests/calls.rs

@@ -0,0 +1,92 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use syn::parse_quote;
+
+#[test]
+fn test_weight_with_trailing_tokens_is_rejected() {
+	assert_pallet_parse_error! {
+		#[manifest_dir("../examples/basic")]
+		#[error_regex("unexpected trailing tokens in pallet attribute")]
+		#[frame_support::pallet]
+		pub mod pallet {
+			#[pallet::config]
+			pub trait Config: frame_system::Config {}
+
+			#[pallet::pallet]
+			pub struct Pallet<T>(_);
+
+			#[pallet::call]
+			impl<T: Config> Pallet<T> {
+				#[pallet::call_index(0)]
+				#[pallet::weight(Weight::zero(), DispatchClass::Operational)]
+				pub fn noop(origin: OriginFor<T>) -> DispatchResult {
+					Ok(())
+				}
+			}
+		}
+	}
+}
+
+#[test]
+fn test_call_index_with_trailing_tokens_is_rejected() {
+	assert_pallet_parse_error! {
+		#[manifest_dir("../examples/basic")]
+		#[error_regex("unexpected trailing tokens in pallet attribute")]
+		#[frame_support::pallet]
+		pub mod pallet {
+			#[pallet::config]
+			pub trait Config: frame_system::Config {}
+
+			#[pallet::pallet]
+			pub struct Pallet<T>(_);
+
+			#[pallet::call]
+			impl<T: Config> Pallet<T> {
+				#[pallet::call_index(0, 1)]
+				#[pallet::weight(Weight::zero())]
+				pub fn noop(origin: OriginFor<T>) -> DispatchResult {
+					Ok(())
+				}
+			}
+		}
+	}
+}
+
+#[test]
+fn test_well_formed_call_attributes_parse() {
+	assert_pallet_parses! {
+		#[manifest_dir("../examples/basic")]
+		#[frame_support::pallet]
+		pub mod pallet {
+			#[pallet::config]
+			pub trait Config: frame_system::Config {}
+
+			#[pallet::pallet]
+			pub struct Pallet<T>(_);
+
+			#[pallet::call]
+			impl<T: Config> Pallet<T> {
+				#[pallet::call_index(0)]
+				#[pallet::weight(Weight::zero())]
+				pub fn noop(origin: OriginFor<T>) -> DispatchResult {
+					Ok(())
+				}
+			}
+		}
+	};
+}