diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index 0a4a29539b2cc..5750c1c4dc18e 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -752,10 +752,6 @@ pub mod pallet { HostConfigurationNotAvailable, /// No validation function upgrade is currently scheduled. NotScheduled, - /// No code upgrade has been authorized. - NothingAuthorized, - /// The given code upgrade has not been authorized. - Unauthorized, } /// Latest included block descendants the runtime accepted. In other words, these are diff --git a/cumulus/pallets/parachain-system/src/tests.rs b/cumulus/pallets/parachain-system/src/tests.rs index d3b65dcdc6390..1f9cb6d54de4f 100755 --- a/cumulus/pallets/parachain-system/src/tests.rs +++ b/cumulus/pallets/parachain-system/src/tests.rs @@ -1170,15 +1170,14 @@ fn receive_hrmp_many() { #[test] fn upgrade_version_checks_should_work() { use codec::Encode; - use sp_runtime::DispatchErrorWithPostInfo; use sp_version::RuntimeVersion; let test_data = vec![ - ("test", 0, 1, Err(frame_system::Error::::SpecVersionNeedsToIncrease)), - ("test", 1, 0, Err(frame_system::Error::::SpecVersionNeedsToIncrease)), - ("test", 1, 1, Err(frame_system::Error::::SpecVersionNeedsToIncrease)), - ("test", 1, 2, Err(frame_system::Error::::SpecVersionNeedsToIncrease)), - ("test2", 1, 1, Err(frame_system::Error::::InvalidSpecName)), + ("test", 0, 1, frame_system::Error::::SpecVersionNeedsToIncrease), + ("test", 1, 0, frame_system::Error::::SpecVersionNeedsToIncrease), + ("test", 1, 1, frame_system::Error::::SpecVersionNeedsToIncrease), + ("test", 1, 2, frame_system::Error::::SpecVersionNeedsToIncrease), + ("test2", 1, 1, frame_system::Error::::InvalidSpecName), ]; for (spec_name, spec_version, impl_version, expected) in test_data.into_iter() { @@ -1193,13 +1192,21 @@ fn upgrade_version_checks_should_work() { let mut ext = new_test_ext(); ext.register_extension(sp_core::traits::ReadRuntimeVersionExt::new(read_runtime_version)); ext.execute_with(|| { + System::set_block_number(1); + let new_code = vec![1, 2, 3, 4]; let new_code_hash = H256(sp_crypto_hashing::blake2_256(&new_code)); let _authorize = System::authorize_upgrade(RawOrigin::Root.into(), new_code_hash); - let res = System::apply_authorized_upgrade(RawOrigin::None.into(), new_code); + assert_ok!(System::apply_authorized_upgrade(RawOrigin::None.into(), new_code)); - assert_eq!(expected.map_err(DispatchErrorWithPostInfo::from), res); + System::assert_last_event( + frame_system::Event::RejectedInvalidAuthorizedUpgrade { + code_hash: new_code_hash, + error: expected.into(), + } + .into(), + ); }); } } diff --git a/prdoc/pr_7812.prdoc b/prdoc/pr_7812.prdoc new file mode 100644 index 0000000000000..97098651734ab --- /dev/null +++ b/prdoc/pr_7812.prdoc @@ -0,0 +1,13 @@ +title: '`apply_authorized_upgrade`: Remote authorization if the version check fails' +doc: +- audience: Runtime User + description: |- + This pr ensures that we remove the authorization for a runtime upgrade if the version check failed. + If that check is failing, it means that the runtime upgrade is invalid and the check will never succeed. + + Besides that the pr is doing some clean ups. +crates: +- name: cumulus-pallet-parachain-system + bump: major +- name: frame-system + bump: major diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index e0db9002ab301..8cddd4b72e7ac 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -712,7 +712,7 @@ pub mod pallet { #[pallet::weight((T::SystemWeightInfo::set_code(), DispatchClass::Operational))] pub fn set_code(origin: OriginFor, code: Vec) -> DispatchResultWithPostInfo { ensure_root(origin)?; - Self::can_set_code(&code)?; + Self::can_set_code(&code, true).into_result()?; T::OnSetCode::set_code(code)?; // consume the rest of the block to prevent further transactions Ok(Some(T::BlockWeights::get().max_block).into()) @@ -729,6 +729,7 @@ pub mod pallet { code: Vec, ) -> DispatchResultWithPostInfo { ensure_root(origin)?; + Self::can_set_code(&code, false).into_result()?; T::OnSetCode::set_code(code)?; Ok(Some(T::BlockWeights::get().max_block).into()) } @@ -863,8 +864,32 @@ pub mod pallet { _: OriginFor, code: Vec, ) -> DispatchResultWithPostInfo { - let post = Self::do_apply_authorize_upgrade(code)?; - Ok(post) + let res = Self::validate_code_is_authorized(&code)?; + AuthorizedUpgrade::::kill(); + + match Self::can_set_code(&code, res.check_version) { + CanSetCodeResult::Ok => {}, + CanSetCodeResult::MultiBlockMigrationsOngoing => + return Err(Error::::MultiBlockMigrationsOngoing.into()), + CanSetCodeResult::InvalidVersion(error) => { + // The upgrade is invalid and there is no benefit in trying to apply this again. + Self::deposit_event(Event::RejectedInvalidAuthorizedUpgrade { + code_hash: res.code_hash, + error: error.into(), + }); + + // Not the fault of the caller of call. + return Ok(Pays::No.into()) + }, + }; + T::OnSetCode::set_code(code)?; + + Ok(PostDispatchInfo { + // consume the rest of the block to prevent further transactions + actual_weight: Some(T::BlockWeights::get().max_block), + // no fee for valid upgrade + pays_fee: Pays::No, + }) } } @@ -894,6 +919,8 @@ pub mod pallet { TaskFailed { task: T::RuntimeTask, err: DispatchError }, /// An upgrade was authorized. UpgradeAuthorized { code_hash: T::Hash, check_version: bool }, + /// An invalid authorized upgrade was rejected while trying to apply it. + RejectedInvalidAuthorizedUpgrade { code_hash: T::Hash, error: DispatchError }, } /// Error for the System pallet @@ -1091,16 +1118,17 @@ pub mod pallet { type Call = Call; fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity { if let Call::apply_authorized_upgrade { ref code } = call { - if let Ok(hash) = Self::validate_authorized_upgrade(&code[..]) { + if let Ok(res) = Self::validate_code_is_authorized(&code[..]) { return Ok(ValidTransaction { - priority: 100, + priority: u64::max_value(), requires: Vec::new(), - provides: vec![hash.as_ref().to_vec()], + provides: vec![res.code_hash.encode()], longevity: TransactionLongevity::max_value(), propagate: true, }) } } + #[cfg(feature = "experimental")] if let Call::do_task { ref task } = call { if task.is_valid() { @@ -1113,6 +1141,7 @@ pub mod pallet { }) } } + Err(InvalidTransaction::Call.into()) } } @@ -1470,6 +1499,28 @@ pub enum DecRefStatus { Exists, } +/// Result of [`Pallet::can_set_code`]. +pub enum CanSetCodeResult { + /// Everything is fine. + Ok, + /// Multi-block migrations are on-going. + MultiBlockMigrationsOngoing, + /// The runtime version is invalid or could not be fetched. + InvalidVersion(Error), +} + +impl CanSetCodeResult { + /// Convert `Self` into a result. + pub fn into_result(self) -> Result<(), DispatchError> { + match self { + Self::Ok => Ok(()), + Self::MultiBlockMigrationsOngoing => + Err(Error::::MultiBlockMigrationsOngoing.into()), + Self::InvalidVersion(err) => Err(err.into()), + } + } +} + impl Pallet { /// Returns the `spec_version` of the last runtime upgrade. /// @@ -2213,71 +2264,55 @@ impl Pallet { /// Determine whether or not it is possible to update the code. /// - /// Checks the given code if it is a valid runtime wasm blob by instantiating - /// it and extracting the runtime version of it. It checks that the runtime version - /// of the old and new runtime has the same spec name and that the spec version is increasing. - pub fn can_set_code(code: &[u8]) -> Result<(), sp_runtime::DispatchError> { + /// - `check_version`: Should the runtime version be checked? + pub fn can_set_code(code: &[u8], check_version: bool) -> CanSetCodeResult { if T::MultiBlockMigrator::ongoing() { - return Err(Error::::MultiBlockMigrationsOngoing.into()) + return CanSetCodeResult::MultiBlockMigrationsOngoing } - let current_version = T::Version::get(); - let new_version = sp_io::misc::runtime_version(code) - .and_then(|v| RuntimeVersion::decode(&mut &v[..]).ok()) - .ok_or(Error::::FailedToExtractRuntimeVersion)?; + if check_version { + let current_version = T::Version::get(); + let Some(new_version) = sp_io::misc::runtime_version(code) + .and_then(|v| RuntimeVersion::decode(&mut &v[..]).ok()) + else { + return CanSetCodeResult::InvalidVersion(Error::::FailedToExtractRuntimeVersion) + }; - cfg_if::cfg_if! { - if #[cfg(all(feature = "runtime-benchmarks", not(test)))] { + cfg_if::cfg_if! { + if #[cfg(all(feature = "runtime-benchmarks", not(test)))] { // Let's ensure the compiler doesn't optimize our fetching of the runtime version away. core::hint::black_box((new_version, current_version)); - Ok(()) - } else { - if new_version.spec_name != current_version.spec_name { - return Err(Error::::InvalidSpecName.into()) - } + } else { + if new_version.spec_name != current_version.spec_name { + return CanSetCodeResult::InvalidVersion( Error::::InvalidSpecName) + } - if new_version.spec_version <= current_version.spec_version { - return Err(Error::::SpecVersionNeedsToIncrease.into()) + if new_version.spec_version <= current_version.spec_version { + return CanSetCodeResult::InvalidVersion(Error::::SpecVersionNeedsToIncrease) + } } - - Ok(()) } } + + CanSetCodeResult::Ok } - /// To be called after any origin/privilege checks. Put the code upgrade authorization into - /// storage and emit an event. Infallible. + /// Authorize the given `code_hash` as upgrade. pub fn do_authorize_upgrade(code_hash: T::Hash, check_version: bool) { AuthorizedUpgrade::::put(CodeUpgradeAuthorization { code_hash, check_version }); Self::deposit_event(Event::UpgradeAuthorized { code_hash, check_version }); } - /// Apply an authorized upgrade, performing any validation checks, and remove the authorization. - /// Whether or not the code is set directly depends on the `OnSetCode` configuration of the - /// runtime. - pub fn do_apply_authorize_upgrade(code: Vec) -> Result { - Self::validate_authorized_upgrade(&code[..])?; - T::OnSetCode::set_code(code)?; - AuthorizedUpgrade::::kill(); - let post = PostDispatchInfo { - // consume the rest of the block to prevent further transactions - actual_weight: Some(T::BlockWeights::get().max_block), - // no fee for valid upgrade - pays_fee: Pays::No, - }; - Ok(post) - } - - /// Check that provided `code` can be upgraded to. Namely, check that its hash matches an - /// existing authorization and that it meets the specification requirements of `can_set_code`. - pub fn validate_authorized_upgrade(code: &[u8]) -> Result { + /// Check that provided `code` is authorized as an upgrade. + /// + /// Returns the [`CodeUpgradeAuthorization`]. + fn validate_code_is_authorized( + code: &[u8], + ) -> Result, DispatchError> { let authorization = AuthorizedUpgrade::::get().ok_or(Error::::NothingAuthorized)?; let actual_hash = T::Hashing::hash(code); ensure!(actual_hash == authorization.code_hash, Error::::Unauthorized); - if authorization.check_version { - Self::can_set_code(code)? - } - Ok(actual_hash) + Ok(authorization) } /// Reclaim the weight for the extrinsic given info and post info.