Fix CPI duplicate account privilege escalation

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<InstructionAccount> = duplicate_indicies
             .into_iter()
             .map(|duplicate_index| deduplicated_instruction_accounts[duplicate_index].clone())

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();

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;

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!(),
     }
 

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));