Track account writable deescalation (bp #14626) (#14787)

* Track account writable deescalation (#14626) (cherry picked from commit 77572a7) # Conflicts: # sdk/src/feature_set.rs * fix conflicts Co-authored-by: Jack May <[email protected]>
solana-labs · Jan 23, 2021 · 480a35d · 480a35d
1 parent e6b53c2
commit 480a35d
Show file tree

Hide file tree

Showing 13 changed files with 236 additions and 46 deletions.
diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs
@@ -210,6 +210,13 @@ impl program_stubs::SyscallStubs for SyscallStubs {
         let program_id_index = message.instructions[0].program_id_index as usize;
         let program_id = message.account_keys[program_id_index];
         let program_account_info = &account_infos[program_id_index];
+        // TODO don't have the caller's keyed_accounts so can't validate writer or signer escalation or deescalation yet
+        let caller_privileges = message
+            .account_keys
+            .iter()
+            .enumerate()
+            .map(|(i, _)| message.is_writable(i))
+            .collect::<Vec<bool>>();
 
         stable_log::program_invoke(&logger, &program_id, invoke_context.invoke_depth());
 
@@ -268,6 +275,7 @@ impl program_stubs::SyscallStubs for SyscallStubs {
             &message,
             &executables,
             &accounts,
+            &caller_privileges,
             invoke_context,
         )
         .map_err(|err| ProgramError::try_from(err).unwrap_or_else(|err| panic!("{}", err)))?;

diff --git a/programs/bpf/c/src/invoke/invoke.c b/programs/bpf/c/src/invoke/invoke.c
@@ -17,6 +17,7 @@ static const uint8_t TEST_INSTRUCTION_META_TOO_LARGE = 10;
 static const uint8_t TEST_RETURN_ERROR = 11;
 static const uint8_t TEST_PRIVILEGE_DEESCALATION_ESCALATION_SIGNER = 12;
 static const uint8_t TEST_PRIVILEGE_DEESCALATION_ESCALATION_WRITABLE = 13;
+static const uint8_t TEST_WRITE_DEESCALATION = 14;
 
 static const int MINT_INDEX = 0;
 static const int ARGUMENT_INDEX = 1;
@@ -251,6 +252,26 @@ extern uint64_t entrypoint(const uint8_t *input) {
     for (int i = 0; i < accounts[INVOKED_ARGUMENT_INDEX].data_len; i++) {
       sol_assert(accounts[INVOKED_ARGUMENT_INDEX].data[i] == i);
     }
+
+    sol_log("Verify data write before ro cpi call");
+    {
+      for (int i = 0; i < accounts[ARGUMENT_INDEX].data_len; i++) {
+        accounts[ARGUMENT_INDEX].data[i] = 0;
+      }
+
+      SolAccountMeta arguments[] = {
+          {accounts[ARGUMENT_INDEX].key, false, false}};
+      uint8_t data[] = {VERIFY_PRIVILEGE_DEESCALATION};
+      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)));
+
+      for (int i = 0; i < accounts[ARGUMENT_INDEX].data_len; i++) {
+        sol_assert(accounts[ARGUMENT_INDEX].data[i] == 0);
+      }
+    }
     break;
   }
   case TEST_PRIVILEGE_ESCALATION_SIGNER: {
@@ -443,7 +464,8 @@ extern uint64_t entrypoint(const uint8_t *input) {
     break;
   }
   case TEST_RETURN_ERROR: {
-    SolAccountMeta arguments[] = {{accounts[ARGUMENT_INDEX].key, true, true}};
+    sol_log("Test return error");
+    SolAccountMeta arguments[] = {{accounts[ARGUMENT_INDEX].key, false, true}};
     uint8_t data[] = {RETURN_ERROR};
     const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key,
                                         arguments, SOL_ARRAY_SIZE(arguments),
@@ -484,6 +506,18 @@ extern uint64_t entrypoint(const uint8_t *input) {
     break;
   }
 
+  case TEST_WRITE_DEESCALATION: {
+    sol_log("Test writable deescalation");
+
+    SolAccountMeta arguments[] = {
+        {accounts[INVOKED_ARGUMENT_INDEX].key, false, false}};
+    uint8_t data[] = {WRITE_ACCOUNT, 10};
+    const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key,
+                                        arguments, SOL_ARRAY_SIZE(arguments),
+                                        data, SOL_ARRAY_SIZE(data)};
+    sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts));
+    break;
+  }
   default:
     sol_panic();
   }

diff --git a/programs/bpf/c/src/invoked/instruction.h b/programs/bpf/c/src/invoked/instruction.h
@@ -15,3 +15,4 @@ const uint8_t RETURN_OK = 7;
 const uint8_t VERIFY_PRIVILEGE_DEESCALATION = 8;
 const uint8_t VERIFY_PRIVILEGE_DEESCALATION_ESCALATION_SIGNER = 9;
 const uint8_t VERIFY_PRIVILEGE_DEESCALATION_ESCALATION_WRITABLE = 10;
+const uint8_t WRITE_ACCOUNT = 11;
diff --git a/programs/bpf/c/src/invoked/invoked.c b/programs/bpf/c/src/invoked/invoked.c
@@ -158,6 +158,7 @@ extern uint64_t entrypoint(const uint8_t *input) {
     sol_assert(accounts[ARGUMENT_INDEX].is_writable);
     break;
   }
+
   case VERIFY_PRIVILEGE_ESCALATION: {
     sol_log("Should never get here!");
     break;
@@ -188,6 +189,7 @@ extern uint64_t entrypoint(const uint8_t *input) {
                sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts)));
     break;
   }
+
   case VERIFY_PRIVILEGE_DEESCALATION_ESCALATION_WRITABLE: {
     sol_log("verify privilege deescalation escalation writable");
     static const int INVOKED_PROGRAM_INDEX = 0;
@@ -245,6 +247,18 @@ extern uint64_t entrypoint(const uint8_t *input) {
     }
     break;
   }
+
+  case WRITE_ACCOUNT: {
+    sol_log("write account");
+    static const int INVOKED_ARGUMENT_INDEX = 0;
+    sol_assert(sol_deserialize(input, &params, 1));
+
+    for (int i = 0; i < params.data[1]; i++) {
+      accounts[INVOKED_ARGUMENT_INDEX].data[i] = params.data[1];
+    }
+    break;
+  }
+
   default:
     return ERROR_INVALID_INSTRUCTION_DATA;
   }

diff --git a/programs/bpf/rust/invoke/src/lib.rs b/programs/bpf/rust/invoke/src/lib.rs
@@ -29,6 +29,7 @@ const TEST_INSTRUCTION_META_TOO_LARGE: u8 = 10;
 const TEST_RETURN_ERROR: u8 = 11;
 const TEST_PRIVILEGE_DEESCALATION_ESCALATION_SIGNER: u8 = 12;
 const TEST_PRIVILEGE_DEESCALATION_ESCALATION_WRITABLE: u8 = 13;
+const TEST_WRITE_DEESCALATION: u8 = 14;
 
 // const MINT_INDEX: usize = 0;
 const ARGUMENT_INDEX: usize = 1;
@@ -331,6 +332,28 @@ fn process_instruction(
                     assert_eq!(data[i as usize], i);
                 }
             }
+
+            msg!("Verify data write before cpi call with deescalated writable");
+            {
+                {
+                    let mut data = accounts[ARGUMENT_INDEX].try_borrow_mut_data()?;
+                    for i in 0..100 {
+                        data[i as usize] = 42;
+                    }
+                }
+
+                let invoked_instruction = create_instruction(
+                    *accounts[INVOKED_PROGRAM_INDEX].key,
+                    &[(accounts[ARGUMENT_INDEX].key, false, false)],
+                    vec![VERIFY_PRIVILEGE_DEESCALATION],
+                );
+                invoke(&invoked_instruction, accounts)?;
+
+                let data = accounts[ARGUMENT_INDEX].try_borrow_data()?;
+                for i in 0..100 {
+                    assert_eq!(data[i as usize], 42);
+                }
+            }
         }
         TEST_PRIVILEGE_ESCALATION_SIGNER => {
             msg!("Test privilege escalation signer");
@@ -534,6 +557,15 @@ fn process_instruction(
             );
             invoke(&invoked_instruction, accounts)?;
         }
+        TEST_WRITE_DEESCALATION => {
+            msg!("Test writable deescalation");
+            let instruction = create_instruction(
+                *accounts[INVOKED_PROGRAM_INDEX].key,
+                &[(accounts[INVOKED_ARGUMENT_INDEX].key, false, false)],
+                vec![WRITE_ACCOUNT, 10],
+            );
+            let _ = invoke(&instruction, accounts);
+        }
         _ => panic!(),
     }
 

diff --git a/programs/bpf/rust/invoked/src/instruction.rs b/programs/bpf/rust/invoked/src/instruction.rs
@@ -16,6 +16,7 @@ pub const RETURN_OK: u8 = 7;
 pub const VERIFY_PRIVILEGE_DEESCALATION: u8 = 8;
 pub const VERIFY_PRIVILEGE_DEESCALATION_ESCALATION_SIGNER: u8 = 9;
 pub const VERIFY_PRIVILEGE_DEESCALATION_ESCALATION_WRITABLE: u8 = 10;
+pub const WRITE_ACCOUNT: u8 = 11;
 
 pub fn create_instruction(
     program_id: Pubkey,

diff --git a/programs/bpf/rust/invoked/src/processor.rs b/programs/bpf/rust/invoked/src/processor.rs
@@ -229,6 +229,12 @@ fn process_instruction(
                 }
             }
         }
+        WRITE_ACCOUNT => {
+            msg!("write account");
+            for i in 0..instruction_data[1] {
+                accounts[0].data.borrow_mut()[i as usize] = instruction_data[1];
+            }
+        }
         _ => panic!(),
     }
 

diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs
@@ -697,6 +697,7 @@ fn test_program_bpf_invoke_sanity() {
     const TEST_RETURN_ERROR: u8 = 11;
     const TEST_PRIVILEGE_DEESCALATION_ESCALATION_SIGNER: u8 = 12;
     const TEST_PRIVILEGE_DEESCALATION_ESCALATION_WRITABLE: u8 = 13;
+    const TEST_WRITE_DEESCALATION: u8 = 14;
 
     #[allow(dead_code)]
     #[derive(Debug)]
@@ -813,6 +814,7 @@ fn test_program_bpf_invoke_sanity() {
                 invoked_program_id.clone(),
                 invoked_program_id.clone(),
                 invoked_program_id.clone(),
+                invoked_program_id.clone(),
             ],
             Languages::Rust => vec![
                 solana_sdk::system_program::id(),
@@ -830,6 +832,7 @@ fn test_program_bpf_invoke_sanity() {
                 invoked_program_id.clone(),
                 invoked_program_id.clone(),
                 invoked_program_id.clone(),
+                invoked_program_id.clone(),
             ],
         };
         assert_eq!(invoked_programs.len(), expected_invoked_programs.len());
@@ -931,6 +934,12 @@ fn test_program_bpf_invoke_sanity() {
             &[invoked_program_id.clone()],
         );
 
+        do_invoke_failure_test_local(
+            TEST_WRITE_DEESCALATION,
+            TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified),
+            &[invoked_program_id.clone()],
+        );
+
         // Check resulting state
 
         assert_eq!(43, bank.get_balance(&derived_key1));

diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs
@@ -1528,7 +1528,14 @@ fn call<'a>(
     signers_seeds_len: u64,
     memory_mapping: &MemoryMapping,
 ) -> Result<u64, EbpfError<BPFError>> {
-    let (message, executables, accounts, account_refs, abort_on_all_cpi_failures) = {
+    let (
+        message,
+        executables,
+        accounts,
+        account_refs,
+        caller_privileges,
+        abort_on_all_cpi_failures,
+    ) = {
         let invoke_context = syscall.get_context()?;
 
         invoke_context
@@ -1555,6 +1562,20 @@ fn call<'a>(
         let (message, callee_program_id, callee_program_id_index) =
             MessageProcessor::create_message(&instruction, &keyed_account_refs, &signers)
                 .map_err(SyscallError::InstructionError)?;
+        let caller_privileges = message
+            .account_keys
+            .iter()
+            .map(|key| {
+                if let Some(keyed_account) = keyed_account_refs
+                    .iter()
+                    .find(|keyed_account| key == keyed_account.unsigned_key())
+                {
+                    keyed_account.is_writable()
+                } else {
+                    false
+                }
+            })
+            .collect::<Vec<bool>>();
         if invoke_context.is_feature_active(&limit_cpi_loader_invoke::id()) {
             check_authorized_program(&callee_program_id, &instruction.data)?;
         }
@@ -1590,6 +1611,7 @@ fn call<'a>(
             executables,
             accounts,
             account_refs,
+            caller_privileges,
             invoke_context.is_feature_active(&abort_on_all_cpi_failures::id()),
         )
     };
@@ -1601,6 +1623,7 @@ fn call<'a>(
         &message,
         &executables,
         &accounts,
+        &caller_privileges,
         *(&mut *(syscall.get_context_mut()?)),
     ) {
         Ok(()) => (),

diff --git a/runtime/benches/message_processor.rs b/runtime/benches/message_processor.rs
@@ -16,15 +16,18 @@ fn bench_verify_account_changes_data(bencher: &mut Bencher) {
     let pre = PreAccount::new(
         &pubkey::new_rand(),
         &Account::new(0, BUFSIZE, &owner),
-        true,
         false,
     );
     let post = Account::new(0, BUFSIZE, &owner);
-    assert_eq!(pre.verify(&owner, &Rent::default(), &post), Ok(()));
+    assert_eq!(
+        pre.verify(&owner, Some(false), &Rent::default(), &post),
+        Ok(())
+    );
 
     // this one should be faster
     bencher.iter(|| {
-        pre.verify(&owner, &Rent::default(), &post).unwrap();
+        pre.verify(&owner, Some(false), &Rent::default(), &post)
+            .unwrap();
     });
     let summary = bencher.bench(|_bencher| {}).unwrap();
     info!("data no change by owner: {} ns/iter", summary.median);
@@ -38,11 +41,11 @@ fn bench_verify_account_changes_data(bencher: &mut Bencher) {
     let pre = PreAccount::new(
         &pubkey::new_rand(),
         &Account::new(0, BUFSIZE, &owner),
-        true,
         false,
     );
     bencher.iter(|| {
-        pre.verify(&non_owner, &Rent::default(), &post).unwrap();
+        pre.verify(&non_owner, Some(false), &Rent::default(), &post)
+            .unwrap();
     });
     let summary = bencher.bench(|_bencher| {}).unwrap();
     info!("data no change by non owner: {} ns/iter", summary.median);