From a71f05f86c7ccc170f376f67f2227b8c3dc49880 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 28 Jan 2022 00:52:02 +0100 Subject: [PATCH] Fix CPI duplicate account privilege escalation (#22752) * Adds TEST_DUPLICATE_PRIVILEGE_ESCALATION_SIGNER and TEST_DUPLICATE_PRIVILEGE_ESCALATION_WRITABLE. * Moves CPI privilege verification out of deduplication loop. --- program-runtime/src/invoke_context.rs | 56 ++++++++++---------- programs/bpf/c/src/invoke/invoke.c | 38 +++++++++++++ programs/bpf/rust/invoke/src/instructions.rs | 2 + programs/bpf/rust/invoke/src/processor.rs | 35 +++++++++++- programs/bpf/tests/programs.rs | 12 +++++ 5 files changed, 115 insertions(+), 28 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index 8d1e6d9951..0850c9202b 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -698,33 +698,6 @@ impl<'a> InvokeContext<'a> { ); InstructionError::MissingAccount })?; - let borrowed_account = instruction_context - .try_borrow_account(self.transaction_context, index_in_caller)?; - - // Readonly in caller cannot become writable in callee - if account_meta.is_writable && !borrowed_account.is_writable() { - ic_msg!( - self, - "{}'s writable privilege escalated", - borrowed_account.get_key(), - ); - return Err(InstructionError::PrivilegeEscalation); - } - - // To be signed in the callee, - // it must be either signed in the caller or by the program - if account_meta.is_signer - && !(borrowed_account.is_signer() - || signers.contains(borrowed_account.get_key())) - { - ic_msg!( - self, - "{}'s signer privilege escalated", - borrowed_account.get_key() - ); - return Err(InstructionError::PrivilegeEscalation); - } - duplicate_indicies.push(deduplicated_instruction_accounts.len()); deduplicated_instruction_accounts.push(InstructionAccount { index_in_transaction, @@ -734,6 +707,35 @@ impl<'a> InvokeContext<'a> { }); } } + for instruction_account in deduplicated_instruction_accounts.iter() { + let borrowed_account = instruction_context.try_borrow_account( + self.transaction_context, + instruction_account.index_in_caller, + )?; + + // Readonly in caller cannot become writable in callee + if instruction_account.is_writable && !borrowed_account.is_writable() { + ic_msg!( + self, + "{}'s writable privilege escalated", + borrowed_account.get_key(), + ); + return Err(InstructionError::PrivilegeEscalation); + } + + // To be signed in the callee, + // it must be either signed in the caller or by the program + if instruction_account.is_signer + && !(borrowed_account.is_signer() || signers.contains(borrowed_account.get_key())) + { + ic_msg!( + self, + "{}'s signer privilege escalated", + borrowed_account.get_key() + ); + return Err(InstructionError::PrivilegeEscalation); + } + } let instruction_accounts: Vec = duplicate_indicies .into_iter() .map(|duplicate_index| deduplicated_instruction_accounts[duplicate_index].clone()) diff --git a/programs/bpf/c/src/invoke/invoke.c b/programs/bpf/c/src/invoke/invoke.c index 9f62ca3cef..3ed95533d4 100644 --- a/programs/bpf/c/src/invoke/invoke.c +++ b/programs/bpf/c/src/invoke/invoke.c @@ -29,6 +29,8 @@ static const uint8_t TEST_EXECUTABLE_LAMPORTS = 16; static const uint8_t TEST_CALL_PRECOMPILE = 17; static const uint8_t ADD_LAMPORTS = 18; static const uint8_t TEST_RETURN_DATA_TOO_LARGE = 19; +static const uint8_t TEST_DUPLICATE_PRIVILEGE_ESCALATION_SIGNER = 20; +static const uint8_t TEST_DUPLICATE_PRIVILEGE_ESCALATION_WRITABLE = 21; static const int MINT_INDEX = 0; static const int ARGUMENT_INDEX = 1; @@ -611,6 +613,42 @@ extern uint64_t entrypoint(const uint8_t *input) { sol_set_return_data(NULL, 1027); break; } + case TEST_DUPLICATE_PRIVILEGE_ESCALATION_SIGNER: { + sol_log("Test duplicate privilege escalation signer"); + SolAccountMeta arguments[] = { + {accounts[DERIVED_KEY3_INDEX].key, false, false}, + {accounts[DERIVED_KEY3_INDEX].key, false, false}, + {accounts[DERIVED_KEY3_INDEX].key, false, false}}; + uint8_t data[] = {VERIFY_PRIVILEGE_ESCALATION}; + const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, + arguments, SOL_ARRAY_SIZE(arguments), + data, SOL_ARRAY_SIZE(data)}; + sol_assert(SUCCESS == + sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts))); + + // Signer privilege escalation will always fail the whole transaction + instruction.accounts[1].is_signer = true; + sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts)); + break; + } + case TEST_DUPLICATE_PRIVILEGE_ESCALATION_WRITABLE: { + sol_log("Test duplicate privilege escalation writable"); + SolAccountMeta arguments[] = { + {accounts[DERIVED_KEY3_INDEX].key, false, false}, + {accounts[DERIVED_KEY3_INDEX].key, false, false}, + {accounts[DERIVED_KEY3_INDEX].key, false, false}}; + uint8_t data[] = {VERIFY_PRIVILEGE_ESCALATION}; + const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, + arguments, SOL_ARRAY_SIZE(arguments), + data, SOL_ARRAY_SIZE(data)}; + sol_assert(SUCCESS == + sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts))); + + // Writable privilege escalation will always fail the whole transaction + instruction.accounts[1].is_writable = true; + sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts)); + break; + } default: sol_panic(); diff --git a/programs/bpf/rust/invoke/src/instructions.rs b/programs/bpf/rust/invoke/src/instructions.rs index 3800af28be..97c1b75317 100644 --- a/programs/bpf/rust/invoke/src/instructions.rs +++ b/programs/bpf/rust/invoke/src/instructions.rs @@ -19,6 +19,8 @@ pub const TEST_EXECUTABLE_LAMPORTS: u8 = 16; pub const TEST_CALL_PRECOMPILE: u8 = 17; pub const ADD_LAMPORTS: u8 = 18; pub const TEST_RETURN_DATA_TOO_LARGE: u8 = 19; +pub const TEST_DUPLICATE_PRIVILEGE_ESCALATION_SIGNER: u8 = 20; +pub const TEST_DUPLICATE_PRIVILEGE_ESCALATION_WRITABLE: u8 = 21; pub const MINT_INDEX: usize = 0; pub const ARGUMENT_INDEX: usize = 1; diff --git a/programs/bpf/rust/invoke/src/processor.rs b/programs/bpf/rust/invoke/src/processor.rs index d466ee9ebf..5c456a3ea6 100644 --- a/programs/bpf/rust/invoke/src/processor.rs +++ b/programs/bpf/rust/invoke/src/processor.rs @@ -426,7 +426,6 @@ fn process_instruction( // Writable privilege escalation will always fail the whole transaction invoked_instruction.accounts[0].is_writable = true; - invoke(&invoked_instruction, accounts)?; } TEST_PPROGRAM_NOT_EXECUTABLE => { @@ -638,6 +637,40 @@ fn process_instruction( TEST_RETURN_DATA_TOO_LARGE => { set_return_data(&[1u8; 1028]); } + TEST_DUPLICATE_PRIVILEGE_ESCALATION_SIGNER => { + msg!("Test duplicate privilege escalation signer"); + let mut invoked_instruction = create_instruction( + *accounts[INVOKED_PROGRAM_INDEX].key, + &[ + (accounts[DERIVED_KEY3_INDEX].key, false, false), + (accounts[DERIVED_KEY3_INDEX].key, false, false), + (accounts[DERIVED_KEY3_INDEX].key, false, false), + ], + vec![VERIFY_PRIVILEGE_ESCALATION], + ); + invoke(&invoked_instruction, accounts)?; + + // Signer privilege escalation will always fail the whole transaction + invoked_instruction.accounts[1].is_signer = true; + invoke(&invoked_instruction, accounts)?; + } + TEST_DUPLICATE_PRIVILEGE_ESCALATION_WRITABLE => { + msg!("Test duplicate privilege escalation writable"); + let mut invoked_instruction = create_instruction( + *accounts[INVOKED_PROGRAM_INDEX].key, + &[ + (accounts[DERIVED_KEY3_INDEX].key, false, false), + (accounts[DERIVED_KEY3_INDEX].key, false, false), + (accounts[DERIVED_KEY3_INDEX].key, false, false), + ], + vec![VERIFY_PRIVILEGE_ESCALATION], + ); + invoke(&invoked_instruction, accounts)?; + + // Writable privilege escalation will always fail the whole transaction + invoked_instruction.accounts[1].is_writable = true; + invoke(&invoked_instruction, accounts)?; + } _ => panic!(), } diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 637db1056a..f2abc0cc6d 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -1116,6 +1116,18 @@ fn test_program_bpf_invoke_sanity() { &[], ); + do_invoke_failure_test_local( + TEST_DUPLICATE_PRIVILEGE_ESCALATION_SIGNER, + TransactionError::InstructionError(0, InstructionError::PrivilegeEscalation), + &[invoked_program_id.clone()], + ); + + do_invoke_failure_test_local( + TEST_DUPLICATE_PRIVILEGE_ESCALATION_WRITABLE, + TransactionError::InstructionError(0, InstructionError::PrivilegeEscalation), + &[invoked_program_id.clone()], + ); + // Check resulting state assert_eq!(43, bank.get_balance(&derived_key1));