sim: Override slot hashes account on simulation bank #24543

joncinque · 2022-04-20T22:57:37Z

Problem

As pointed out in #22880, it's possible for a program to detect if it's in a simulation.

Summary of Changes

Creating new banks has unintended knock-on effects all over the system, and a single source of truth for sysvars that falls back to the accounts db is error-prone, so #24033 and #24034 may cause more problems than they solve.

This is a much simpler approach, and simply overrides the slot history sysvar for simulation banks.

Fixes #23260

jstarry

This approach looks much cleaner!

jstarry · 2022-04-21T07:49:18Z

runtime/src/bank.rs

+                    .as_ref()
+                    .map(|account| from_account::<SlotHistory, _>(account).unwrap())
+                    .unwrap_or_default();
+                slot_history.add(self.slot().saturating_sub(1));


I don't think this fix will quite work because a frozen bank has already called slot_history.add(self.slot()) which will flip the bit for self.slot() % MAX_ENTRIES to 1. When you call slot_history.add(self.slot() - 1), the flipped bit is still set and could be detected by manual inspection (even if the SlotHistory api doesn't expose it)

Correct, we'd have to know what the bit was set to beforehand to reset it to its previous value. However, this is only exploitable if you know a particular slot was skipped exactly MAX_ENTRIES away from your simulation bank, which means you'd need to supply some other data about slot skips to detect it. Although there's still an exploit present, it would probably be close to random. Do you have a suggestion for resetting this properly?

I see, yeah it's tricky that the bank doesn't know what the previous value was. If we always leave the bit flipped as a 1, then a program will know it's not being simulated if it sees that bit set to 0. Thinking now if there's a good way to do this...

The only thing I can come up with is randomizing the bit...

Randomizing the bit is probably sufficient. Another approach could be fetching the state of the sysvar for the previous block with self.load_slow_with_fixed_root(self.proper_ancestors(), ..)

Ah, that's even better, great suggestion.

runtime/src/bank.rs

behzadnouri

+1 this seems better approach than previous one.

behzadnouri · 2022-04-21T13:27:41Z

programs/bpf/rust/simulation/src/lib.rs

+    let data = slot_account.data.borrow();
+    let slot: u64 = u64::from_le_bytes(data[data.len() - 8..].try_into().unwrap());
+
+    let clock = Clock::get().unwrap();


Just to confirm only Clock and SlotHistory expose the slot right?

Do we want to test for SlotHistory::get() as well, to check the other way to obtain the sysvar?

Also for the case that Clock is included in the accounts rather than obtained through Clock::get?

Just to confirm only Clock and SlotHistory expose the slot right?

Explicitly, yes. You could maybe suss it out through the recent blockhashes or slot hashes, but they don't give an explicit slot.

Do we want to test for SlotHistory::get() as well, to check the other way to obtain the sysvar?

Thankfully, it's not possible to do SlotHistory::get()! Which makes this implementation even tidier. Otherwise we'd need to also override it in the sysvar cache.

Also for the case that Clock is included in the accounts rather than obtained through Clock::get?

Good call, I'll add that case as well to be sure. Clock is only updated at the start of the slot, and not at the end, so we should be good, but but it's better to add a test for the future.

programs/bpf/rust/simulation/src/lib.rs

behzadnouri · 2022-04-21T13:55:51Z

runtime/src/accounts.rs

+                    if let Some(account_override) =
+                        account_overrides.and_then(|overrides| overrides.get(key))
+                    {
+                        account_override.clone()


I guess we don't need to do any of these stuff below
https://github.com/solana-labs/solana/blob/02bfb85c1/runtime/src/accounts.rs#L274-L326
for overridden account here because right now it is only SlotHistroy.

But the AccountOverrides = HashMap<Pubkey, AccountSharedData> type implies that these overrides are open to be extended to any account and not just SlotHistory, in which case the assumption above might not be true any more.

Would it make sense to do the override later here instead:
https://github.com/solana-labs/solana/blob/02bfb85c1/runtime/src/accounts.rs#L275-L276
so the rest of the code is symmetric between overridden accounts and account-db accounts?

Alternatively. would it make sense to tighten AccountOverrides a little bit so only explicit set of accounts can be overridden. For now seems like we just need SlotHistory. So maybe instead define:

struct AccountOverrides { slot_history: Option<AccountSharedData>, }

Would it make sense to do the override later here instead:
https://github.com/solana-labs/solana/blob/02bfb85c1/runtime/src/accounts.rs#L275-L276
so the rest of the code is symmetric between overridden accounts and account-db accounts?

Are you suggesting to plumb the overrides down into AccountsDb::load_with_fixed_root? To be honest, I'd prefer not to, since that complicates the AccountsDb interface unnecessarily. Accounts is meant to be a wrapper interface over AccountsDb, so I think it's better for the overrides to be processed there. If you want to keep things symmetric, however, I can refactor the loading logic to either use accounts_db or the overrides. What do you think?

Edit: I think I misunderstood, will refactor to do the rest of the code if loaded from the cache too.

But the AccountOverrides = HashMap<Pubkey, AccountSharedData> type implies that these overrides are open to be extended to any account and not just SlotHistory, in which case the assumption above might not be true any more.

Alternatively. would it make sense to tighten AccountOverrides a little bit so only explicit set of accounts can be overridden.

That was the idea, but if you think it's better to be explicit, I can certainly change up AccountOverrides to explicitly add the slot history. In that case, maybe I'm missing something, but it seems like it's in addition to the loading logic discussed earlier, correct?

I tightened up the logic even further to only add the overrides if the slot history account is present in the transaction to simulate, and added AccountOverrides as a full type

core/src/banking_stage.rs

runtime/src/bank.rs

joncinque · 2022-04-21T17:51:41Z

Thanks for all the feedback everyone! Your points should all be addressed now

behzadnouri

lgtm

jstarry · 2022-04-22T07:07:29Z

I'd like to see this get backported to both v1.9 and v1.10, any objections? cc @t-nelson

codecov · 2022-04-22T07:52:45Z

Codecov Report

Merging #24543 (5b29174) into master (3155270) will increase coverage by 12.0%.
The diff coverage is 81.0%.

@@             Coverage Diff             @@
##           master   #24543       +/-   ##
===========================================
+ Coverage    70.0%    82.0%    +12.0%     
===========================================
  Files          36      595      +559     
  Lines        2255   165000   +162745     
  Branches      322        0      -322     
===========================================
+ Hits         1580   135430   +133850     
- Misses        560    29570    +29010     
+ Partials      115        0      -115

joncinque · 2022-04-22T10:32:24Z

Per offline discussion, we'll backport to 1.10 so RPC nodes can pick it up, which are the target of the fix.

* sim: Override slot hashes during simulation * Add simulation test program * Address feedback * Add AccountOverrides explicit type * Cargo fmt (cherry picked from commit 0d51596) # Conflicts: # programs/bpf/Cargo.lock # runtime/src/bank.rs

#24589) * sim: Override slot hashes account on simulation bank (#24543) * sim: Override slot hashes during simulation * Add simulation test program * Address feedback * Add AccountOverrides explicit type * Cargo fmt (cherry picked from commit 0d51596) # Conflicts: # programs/bpf/Cargo.lock # runtime/src/bank.rs * Fix merge conflicts Co-authored-by: Jon Cinque <jon.cinque@gmail.com>

* sim: Override slot hashes during simulation * Add simulation test program * Address feedback * Add AccountOverrides explicit type * Cargo fmt

t-nelson · 2022-04-22T18:11:25Z

Per offline discussion, we'll backport to 1.10 so RPC nodes can pick it up, which are the target of the fix.

perfect. v1.9 isn't long for this world anyway

* sim: Override slot hashes during simulation * Add simulation test program * Address feedback * Add AccountOverrides explicit type * Cargo fmt (cherry picked from commit 0d51596) # Conflicts: # programs/bpf/Cargo.lock # runtime/src/accounts.rs # runtime/src/bank.rs # runtime/src/lib.rs

#24666) * sim: Override slot hashes account on simulation bank (#24543) * sim: Override slot hashes during simulation * Add simulation test program * Address feedback * Add AccountOverrides explicit type * Cargo fmt (cherry picked from commit 0d51596) # Conflicts: # programs/bpf/Cargo.lock # runtime/src/accounts.rs # runtime/src/bank.rs # runtime/src/lib.rs * Fix merge conflicts * Sync lru version for build error * cargo fmt * Clippy Co-authored-by: Jon Cinque <jon.cinque@gmail.com>

* sim: Override slot hashes during simulation * Add simulation test program * Address feedback * Add AccountOverrides explicit type * Cargo fmt

joncinque requested review from jstarry and behzadnouri April 20, 2022 22:57

This was referenced Apr 20, 2022

accounts: Load sysvar accounts from cache instead of db during tx processing #24033

Closed

simulation: Run preflight transaction on new bank #24034

Closed

jstarry reviewed Apr 21, 2022

View reviewed changes

behzadnouri reviewed Apr 21, 2022

View reviewed changes

joncinque added 4 commits April 21, 2022 18:04

sim: Override slot hashes during simulation

601bb1a

Add simulation test program

d5eaf2a

Address feedback

c03f922

Add AccountOverrides explicit type

9820a9f

joncinque force-pushed the acct-override branch from 46b291f to 9820a9f Compare April 21, 2022 16:04

Cargo fmt

5b29174

behzadnouri approved these changes Apr 21, 2022

View reviewed changes

jstarry approved these changes Apr 22, 2022

View reviewed changes

joncinque added the v1.10 label Apr 22, 2022

joncinque merged commit 0d51596 into solana-labs:master Apr 22, 2022

mergify bot mentioned this pull request Apr 22, 2022

sim: Override slot hashes account on simulation bank (backport #24543) #24589

Merged

joncinque deleted the acct-override branch April 22, 2022 10:34

joncinque added the v1.9 label Apr 25, 2022

mergify bot mentioned this pull request Apr 25, 2022

sim: Override slot hashes account on simulation bank (backport #24543) #24666

Merged

Xavier59 mentioned this pull request Aug 4, 2022

Simulation support for Solana transactions brave/brave-browser#24269

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sim: Override slot hashes account on simulation bank #24543

sim: Override slot hashes account on simulation bank #24543

joncinque commented Apr 20, 2022

jstarry left a comment

jstarry Apr 21, 2022

joncinque Apr 21, 2022

jstarry Apr 21, 2022

joncinque Apr 21, 2022

jstarry Apr 21, 2022

joncinque Apr 21, 2022

behzadnouri left a comment

behzadnouri Apr 21, 2022

behzadnouri Apr 21, 2022

joncinque Apr 21, 2022

behzadnouri Apr 21, 2022

joncinque Apr 21, 2022 •

edited

joncinque Apr 21, 2022

joncinque commented Apr 21, 2022

behzadnouri left a comment

jstarry commented Apr 22, 2022

codecov bot commented Apr 22, 2022

joncinque commented Apr 22, 2022

t-nelson commented Apr 22, 2022

sim: Override slot hashes account on simulation bank #24543

sim: Override slot hashes account on simulation bank #24543

Conversation

joncinque commented Apr 20, 2022

Problem

Summary of Changes

jstarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behzadnouri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque Apr 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque commented Apr 21, 2022

behzadnouri left a comment

Choose a reason for hiding this comment

jstarry commented Apr 22, 2022

codecov bot commented Apr 22, 2022

Codecov Report

joncinque commented Apr 22, 2022

t-nelson commented Apr 22, 2022

joncinque Apr 21, 2022 •

edited