From 1a658c7f31e1e0d2d39d9efbc0e929350e2c2bcb Mon Sep 17 00:00:00 2001 From: Sebastian Bor Date: Thu, 22 Apr 2021 00:06:59 +0100 Subject: [PATCH] Allow SetUpgradeAuthority instruction in CPI calls (#16676) * feat: allow SetAuthority in CLI calls * chore: clippy match_like_matches_macro * chore: clippy match_like_matches_macro * chore: rename CLI to CPI * chore: move check for cpi authorised instruction to syscalls * chore: add set_upgrade_authority cpi test * chore: assert upgrade authority was changed * feat: gate set_upgrade_authority via cpi with a feature * chore: move feature to the end of the list * chore: remove white spaces * chore: remove white spaces * chore: update comment to rerun build --- programs/bpf/Cargo.lock | 1 + programs/bpf/Cargo.toml | 1 + programs/bpf/tests/programs.rs | 96 +++++++++++++++++- programs/bpf_loader/src/syscalls.rs | 11 ++- sdk/program/src/bpf_loader_upgradeable.rs | 114 +++++++++++++++------- sdk/src/feature_set.rs | 4 + 6 files changed, 187 insertions(+), 40 deletions(-) diff --git a/programs/bpf/Cargo.lock b/programs/bpf/Cargo.lock index 2c0fdbfe0f..f6bb26a552 100644 --- a/programs/bpf/Cargo.lock +++ b/programs/bpf/Cargo.lock @@ -2736,6 +2736,7 @@ dependencies = [ "itertools 0.10.0", "miow 0.2.2", "net2", + "solana-account-decoder", "solana-bpf-loader-program", "solana-cli-output", "solana-logger 1.7.0", diff --git a/programs/bpf/Cargo.toml b/programs/bpf/Cargo.toml index f1d458b00f..113d9957b1 100644 --- a/programs/bpf/Cargo.toml +++ b/programs/bpf/Cargo.toml @@ -33,6 +33,7 @@ solana_rbpf = "=0.2.8" solana-runtime = { path = "../../runtime", version = "=1.7.0" } solana-sdk = { path = "../../sdk", version = "=1.7.0" } solana-transaction-status = { path = "../../transaction-status", version = "=1.7.0" } +solana-account-decoder = { path = "../../account-decoder", version = "=1.7.0" } [[bench]] diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index c5f814c90c..af5e27b567 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -4,6 +4,9 @@ extern crate solana_bpf_loader_program; use itertools::izip; +use solana_account_decoder::parse_bpf_loader::{ + parse_bpf_upgradeable_loader, BpfUpgradeableLoaderAccountType, +}; use solana_bpf_loader_program::{ create_vm, serialization::{deserialize_parameters, serialize_parameters}, @@ -42,7 +45,10 @@ use solana_transaction_status::{ token_balances::collect_token_balances, ConfirmedTransaction, InnerInstructions, TransactionStatusMeta, TransactionWithStatusMeta, UiTransactionEncoding, }; -use std::{cell::RefCell, collections::HashMap, env, fs::File, io::Read, path::PathBuf, sync::Arc}; +use std::{ + cell::RefCell, collections::HashMap, env, fs::File, io::Read, path::PathBuf, str::FromStr, + sync::Arc, +}; /// BPF program file extension const PLATFORM_FILE_EXTENSION_BPF: &str = "so"; @@ -2085,6 +2091,94 @@ fn test_program_bpf_upgrade_self_via_cpi() { ); } +#[cfg(feature = "bpf_rust")] +#[test] +fn test_program_bpf_set_upgrade_authority_via_cpi() { + solana_logger::setup(); + + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_genesis_config(50); + let mut bank = Bank::new(&genesis_config); + let (name, id, entrypoint) = solana_bpf_loader_program!(); + bank.add_builtin(&name, id, entrypoint); + let (name, id, entrypoint) = solana_bpf_loader_upgradeable_program!(); + bank.add_builtin(&name, id, entrypoint); + let bank_client = BankClient::new(bank); + + // Deploy CPI invoker program + let invoke_and_return = load_bpf_program( + &bank_client, + &bpf_loader::id(), + &mint_keypair, + "solana_bpf_rust_invoke_and_return", + ); + + // Deploy upgradeable program + let buffer_keypair = Keypair::new(); + let program_keypair = Keypair::new(); + let program_id = program_keypair.pubkey(); + let authority_keypair = Keypair::new(); + load_upgradeable_bpf_program( + &bank_client, + &mint_keypair, + &buffer_keypair, + &program_keypair, + &authority_keypair, + "solana_bpf_rust_upgradeable", + ); + + // Set program upgrade authority instruction to invoke via CPI + let new_upgrade_authority_key = Keypair::new().pubkey(); + let mut set_upgrade_authority_instruction = bpf_loader_upgradeable::set_upgrade_authority( + &program_id, + &authority_keypair.pubkey(), + Some(&new_upgrade_authority_key), + ); + + // Invoke set_upgrade_authority via CPI invoker program + set_upgrade_authority_instruction.program_id = invoke_and_return; + set_upgrade_authority_instruction + .accounts + .insert(0, AccountMeta::new(bpf_loader_upgradeable::id(), false)); + + let message = Message::new( + &[set_upgrade_authority_instruction], + Some(&mint_keypair.pubkey()), + ); + bank_client + .send_and_confirm_message(&[&mint_keypair, &authority_keypair], message) + .unwrap(); + + // Assert upgrade authority was changed + let program_account_data = bank_client.get_account_data(&program_id).unwrap().unwrap(); + let program_account = parse_bpf_upgradeable_loader(&program_account_data).unwrap(); + + let upgrade_authority_key = match program_account { + BpfUpgradeableLoaderAccountType::Program(ui_program) => { + let program_data_account_key = Pubkey::from_str(&ui_program.program_data).unwrap(); + let program_data_account_data = bank_client + .get_account_data(&program_data_account_key) + .unwrap() + .unwrap(); + let program_data_account = + parse_bpf_upgradeable_loader(&program_data_account_data).unwrap(); + + match program_data_account { + BpfUpgradeableLoaderAccountType::ProgramData(ui_program_data) => ui_program_data + .authority + .map(|a| Pubkey::from_str(&a).unwrap()), + _ => None, + } + } + _ => None, + }; + + assert_eq!(Some(new_upgrade_authority_key), upgrade_authority_key); +} + #[cfg(feature = "bpf_rust")] #[test] fn test_program_upgradeable_locks() { diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index 28b49055ed..0e5ec43e84 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -21,7 +21,8 @@ use solana_sdk::{ epoch_schedule::EpochSchedule, feature_set::{ cpi_data_cost, cpi_share_ro_and_exec_accounts, demote_sysvar_write_locks, - enforce_aligned_host_addrs, ristretto_mul_syscall_enabled, sysvar_via_syscall, + enforce_aligned_host_addrs, ristretto_mul_syscall_enabled, + set_upgrade_authority_via_cpi_enabled, sysvar_via_syscall, }, hash::{Hasher, HASH_BYTES}, ic_msg, @@ -1874,12 +1875,16 @@ fn check_account_infos( fn check_authorized_program( program_id: &Pubkey, instruction_data: &[u8], + invoke_context: &Ref<&mut dyn InvokeContext>, ) -> Result<(), EbpfError> { if native_loader::check_id(program_id) || bpf_loader::check_id(program_id) || bpf_loader_deprecated::check_id(program_id) || (bpf_loader_upgradeable::check_id(program_id) - && !bpf_loader_upgradeable::is_upgrade_instruction(instruction_data)) + && !(bpf_loader_upgradeable::is_upgrade_instruction(instruction_data) + || (bpf_loader_upgradeable::is_set_authority_instruction(instruction_data) + && invoke_context + .is_feature_active(&set_upgrade_authority_via_cpi_enabled::id())))) { return Err(SyscallError::ProgramNotSupported(*program_id).into()); } @@ -1994,7 +1999,7 @@ fn call<'a>( } }) .collect::>(); - check_authorized_program(&callee_program_id, &instruction.data)?; + check_authorized_program(&callee_program_id, &instruction.data, &invoke_context)?; let (accounts, account_refs) = syscall.translate_accounts( &message.account_keys, &caller_write_privileges, diff --git a/sdk/program/src/bpf_loader_upgradeable.rs b/sdk/program/src/bpf_loader_upgradeable.rs index 131a9206d0..25a4878ee7 100644 --- a/sdk/program/src/bpf_loader_upgradeable.rs +++ b/sdk/program/src/bpf_loader_upgradeable.rs @@ -192,6 +192,10 @@ pub fn is_upgrade_instruction(instruction_data: &[u8]) -> bool { 3 == instruction_data[0] } +pub fn is_set_authority_instruction(instruction_data: &[u8]) -> bool { + 4 == instruction_data[0] +} + /// Returns the instructions required to set a buffers's authority. pub fn set_buffer_authority( buffer_address: &Pubkey, @@ -262,44 +266,82 @@ mod tests { ); } + fn assert_is_instruction( + is_instruction_fn: F, + expected_instruction: UpgradeableLoaderInstruction, + ) where + F: Fn(&[u8]) -> bool, + { + let result = is_instruction_fn( + &bincode::serialize(&UpgradeableLoaderInstruction::InitializeBuffer).unwrap(), + ); + let expected_result = matches!( + expected_instruction, + UpgradeableLoaderInstruction::InitializeBuffer + ); + assert_eq!(expected_result, result); + + let result = is_instruction_fn( + &bincode::serialize(&UpgradeableLoaderInstruction::Write { + offset: 0, + bytes: vec![], + }) + .unwrap(), + ); + let expected_result = matches!( + expected_instruction, + UpgradeableLoaderInstruction::Write { + offset: _, + bytes: _, + } + ); + assert_eq!(expected_result, result); + + let result = is_instruction_fn( + &bincode::serialize(&UpgradeableLoaderInstruction::DeployWithMaxDataLen { + max_data_len: 0, + }) + .unwrap(), + ); + let expected_result = matches!( + expected_instruction, + UpgradeableLoaderInstruction::DeployWithMaxDataLen { max_data_len: _ } + ); + assert_eq!(expected_result, result); + + let result = + is_instruction_fn(&bincode::serialize(&UpgradeableLoaderInstruction::Upgrade).unwrap()); + let expected_result = matches!(expected_instruction, UpgradeableLoaderInstruction::Upgrade); + assert_eq!(expected_result, result); + + let result = is_instruction_fn( + &bincode::serialize(&UpgradeableLoaderInstruction::SetAuthority).unwrap(), + ); + let expected_result = matches!( + expected_instruction, + UpgradeableLoaderInstruction::SetAuthority + ); + assert_eq!(expected_result, result); + + let result = + is_instruction_fn(&bincode::serialize(&UpgradeableLoaderInstruction::Close).unwrap()); + let expected_result = matches!(expected_instruction, UpgradeableLoaderInstruction::Close); + assert_eq!(expected_result, result); + } + + #[test] + fn test_is_set_authority_instruction() { + assert_is_instruction( + is_set_authority_instruction, + UpgradeableLoaderInstruction::SetAuthority {}, + ); + } + #[test] fn test_is_upgrade_instruction() { - assert_eq!( - false, - is_upgrade_instruction( - &bincode::serialize(&UpgradeableLoaderInstruction::InitializeBuffer).unwrap() - ) - ); - assert_eq!( - false, - is_upgrade_instruction( - &bincode::serialize(&UpgradeableLoaderInstruction::Write { - offset: 0, - bytes: vec![], - }) - .unwrap() - ) - ); - assert_eq!( - false, - is_upgrade_instruction( - &bincode::serialize(&UpgradeableLoaderInstruction::DeployWithMaxDataLen { - max_data_len: 0, - }) - .unwrap() - ) - ); - assert_eq!( - true, - is_upgrade_instruction( - &bincode::serialize(&UpgradeableLoaderInstruction::Upgrade).unwrap() - ) - ); - assert_eq!( - false, - is_upgrade_instruction( - &bincode::serialize(&UpgradeableLoaderInstruction::SetAuthority).unwrap() - ) + assert_is_instruction( + is_upgrade_instruction, + UpgradeableLoaderInstruction::Upgrade {}, ); } } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 3ccac9de37..04e6a17c9d 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -138,6 +138,9 @@ pub mod check_duplicates_by_hash { pub mod enforce_aligned_host_addrs { solana_sdk::declare_id!("6Qob9Z4RwGdf599FDVCqsjuKjR8ZFR3oVs2ByRLWBsua"); } +pub mod set_upgrade_authority_via_cpi_enabled { + solana_sdk::declare_id!("GQdjCCptpGECG7QfE35hKTAopB1umGoSrdKfax2VmZWy"); +} lazy_static! { /// Map of feature identifiers to user-visible description @@ -174,6 +177,7 @@ lazy_static! { (sysvar_via_syscall::id(), "provide sysvars via syscalls"), (check_duplicates_by_hash::id(), "use transaction message hash for duplicate check"), (enforce_aligned_host_addrs::id(), "enforce aligned host addresses"), + (set_upgrade_authority_via_cpi_enabled::id(), "set upgrade authority instruction via cpi calls for upgradable programs"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()