Re-do rent collection check on rent-exempt account by ryoqun · Pull Request #11349 · solana-labs/solana

ryoqun · 2020-08-03T10:40:37Z

Problem

Once passed as a rent-exempt for this epoch, accounts can reside with a penny (= 1 lamport) for the duration of the epoch. This could be exploited to occupy huge amount of storage for accounts, leading to a DOS.

Summary of Changes

rent-collect again only on previously-rent-exempt accounts.

~~Alternatively, we can use the highest bit of the account.rent_epoch? or introduce a new bit field? This would introduce abi breakage, though...~~ => Well, it turns out the fix is pretty simple.

With this fix in effect, attackers actually must leave enough lamports for 10 * 1024 * 1024-byte MAX_PERMITTED_DATA_LENGTH accounts (= roughly 0.2 SOL (a)) to avoid it from being reclaimed after becoming non-rent-exempt.

So, it costs 10M SOL to do 1-epoch 500TB account storage DOS (b):

[1] pry(main)> 19.055441478439427 * 10 * 1024 * 1024 # <- (a)
=> 199810786.03696102
[5] pry(main)> (500 * 1024 * 1024 * 1024 * 1024) / (10 * 1024 * 1024) * (199810786.03696102 / 1000000000) # <- (b)
=> 10475839.738974622

, meaning we're safe now.

Fixes #11342

ryoqun · 2020-08-04T04:51:10Z

@t-nelson @mvines Could one of you sanity check this? Once granted, I'll do the remaining work (test fixes, gating).

Also, I guess we should fix this with reasonably quick fashion. :)

mergify · 2020-08-04T04:52:04Z

automerge label removed due to a CI failure

t-nelson

LGTM! Nice catch!

ryoqun · 2020-08-11T15:24:07Z

todo

rebase on Fix bad rent in Bank::deposit as if since epoch 0 #10468
doc update for rent_epoch + 1 invariant no longer valid and fyi

Pull request has been modified.

ryoqun · 2020-08-13T04:33:13Z

@t-nelson Thanks for early-review. I think this pr is now ready for serious review to merge. :)

ryoqun · 2020-08-13T04:35:45Z

 ## Actual processing of collecting rent

-Rent is due for one epoch's worth of time, and accounts always have `Account::rent_epoch` of `current_epoch + 1`.
+Rent is due for one epoch's worth of time, and accounts have `Account::rent_epoch` of `current_epoch` or `current_epoch + 1` depending on the rent regime.


@rwalker-apple Hi, I'm yet again tweaking the rent protocol a bit, mainly affected by this dos possibility: #11342. Could you review this in your free time?

ryoqun · 2020-08-13T04:41:03Z

                       "data": bs58::encode(expected_data).into_string(),
                       "executable": false,
-                       "rentEpoch": 1,
+                       "rentEpoch": 0,


@CriesofCarrots @jstarry Strictly speaking, we're breaking some semantics (=~ compatibility) of our rpc api a bit here according to this new rule for rentEpoch: https://github.com/solana-labs/solana/pull/11349/files#r469691300.

It's an incompatible change, but the change is soooo subtle, I think. So, I believe there should be virtually no one relying on rentEpoch being the current epoch or the next for rent-exempt accounts in any-conceivable way for the bizarre usecases.

Or, if you have something to come to mind in terms of various partner integartions, please let me know. :)

@ryoqun agreed, very unlikely this will cause any issues. I highly doubt anyone is using rentEpoch right now

I don't know of any integrations using rentEpoch; I'm comfortable with this breaking change.

ryoqun · 2020-08-13T04:47:55Z

+                    account.rent_epoch = self.epoch
+                        + if !self.enable_old_behavior && exempt {
+                            // Rent isn't collected for the next epoch
+                            // Make sure to check exempt status later in curent epoch again
+                            0
+                        } else {
+                            // Rent is collected for next epoch
+                            1
+                        };


This is the crux of this pr. The rest are just supplemental.

codecov · 2020-08-13T07:21:07Z

Codecov Report

Merging #11349 into master will increase coverage by 0.0%.
The diff coverage is 97.4%.

@@           Coverage Diff           @@
##           master   #11349   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         327      327           
  Lines       75668    75701   +33     
=======================================
+ Hits        61932    61960   +28     
- Misses      13736    13741    +5

t-nelson

LGTM! Just one readability nit, should you choose

ryoqun · 2020-08-14T00:19:42Z

    sol_assert(accounts[ARGUMENT_INDEX].is_signer);
    sol_assert(accounts[ARGUMENT_INDEX].is_writable);
-    sol_assert(accounts[ARGUMENT_INDEX].rent_epoch == 1);
+    sol_assert(accounts[ARGUMENT_INDEX].rent_epoch == 0);


@jackcmay Like the RPC land, this pr changes slightly changes the semantics of rent_epoch, which is exposed to programs as well. Are you aware of any programs strictly relying on the current semantics of rent_epoch?

jackcmay · 2020-08-14T00:26:03Z

Not that I know of

…

On Thu, Aug 13, 2020 at 5:19 PM Ryo Onodera ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In programs/bpf/c/src/invoked/invoked.c <#11349 (comment)>: > @@ -36,7 +36,7 @@ extern uint64_t entrypoint(const uint8_t *input) { sol_assert(accounts[ARGUMENT_INDEX].data_len == 100); sol_assert(accounts[ARGUMENT_INDEX].is_signer); sol_assert(accounts[ARGUMENT_INDEX].is_writable); - sol_assert(accounts[ARGUMENT_INDEX].rent_epoch == 1); + sol_assert(accounts[ARGUMENT_INDEX].rent_epoch == 0); @jackcmay <https://github.com/jackcmay> Like the RPC land <https://github.com/solana-labs/solana/pull/11349/files#r469692568>, this pr changes slightly changes the semantics of rent_epoch <https://github.com/solana-labs/solana/pull/11349/files#r469691300>, which is exposed to programs as well. Are you aware of any programs strictly relying on the current semantics of rent_epoch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11349 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJSLY6Q5QQJUJCBAWT2WSC3SAR7KXANCNFSM4PTGTSPQ> .

Pull request has been modified.

ryoqun · 2020-08-14T07:49:17Z

@t-nelson Thanks for re-approving! Now waiting for last-minute local testing by me and final say from @rwalker-com / @rwalker-apple (if any).

rwalker-com · 2020-08-14T16:18:59Z

lgtm

* wip: re-do rent collection check on rent-exempt account * Let's see how the ci goes * Restore previous code * Well, almost all new changes are revertable * Update doc * Add test and gating * Fix tests * Fix tests, especially avoid to change abi... * Fix more tests... * Fix snapshot restore * Align to _new_ with better uninitialized detection (cherry picked from commit 23fa84b) # Conflicts: # core/src/rpc_subscriptions.rs # core/tests/bank_forks.rs # runtime/src/bank.rs

* wip: re-do rent collection check on rent-exempt account * Let's see how the ci goes * Restore previous code * Well, almost all new changes are revertable * Update doc * Add test and gating * Fix tests * Fix tests, especially avoid to change abi... * Fix more tests... * Fix snapshot restore * Align to _new_ with better uninitialized detection (cherry picked from commit 23fa84b) # Conflicts: # core/src/rpc_subscriptions.rs

ryoqun · 2020-08-17T05:26:18Z

Now waiting for last-minute local testing by me

This is done finally and merged this!

* Re-do rent collection check on rent-exempt account (#11349) * wip: re-do rent collection check on rent-exempt account * Let's see how the ci goes * Restore previous code * Well, almost all new changes are revertable * Update doc * Add test and gating * Fix tests * Fix tests, especially avoid to change abi... * Fix more tests... * Fix snapshot restore * Align to _new_ with better uninitialized detection (cherry picked from commit 23fa84b) # Conflicts: # core/src/rpc_subscriptions.rs * Fix conflicts Co-authored-by: Ryo Onodera <ryoqun@gmail.com>

* Re-do rent collection check on rent-exempt account (#11349) * wip: re-do rent collection check on rent-exempt account * Let's see how the ci goes * Restore previous code * Well, almost all new changes are revertable * Update doc * Add test and gating * Fix tests * Fix tests, especially avoid to change abi... * Fix more tests... * Fix snapshot restore * Align to _new_ with better uninitialized detection (cherry picked from commit 23fa84b) # Conflicts: # core/src/rpc_subscriptions.rs # core/tests/bank_forks.rs # runtime/src/bank.rs * Fix conflicts * Add missing comment Co-authored-by: Ryo Onodera <ryoqun@gmail.com>

ryoqun commented Aug 3, 2020

View reviewed changes

Comment thread runtime/src/rent_collector.rs Outdated

ryoqun commented Aug 3, 2020

View reviewed changes

Comment thread runtime/src/rent_collector.rs Outdated

ryoqun commented Aug 3, 2020

View reviewed changes

Comment thread runtime/src/rent_collector.rs Outdated

ryoqun added automerge Merge this Pull Request automatically once CI passes v1.2 security Pull requests that address a security vulnerability labels Aug 4, 2020

mergify Bot removed the automerge Merge this Pull Request automatically once CI passes label Aug 4, 2020

mvines added the v1.3 label Aug 6, 2020

t-nelson previously approved these changes Aug 7, 2020

View reviewed changes

ryoqun added 4 commits August 12, 2020 01:12

wip: re-do rent collection check on rent-exempt account

1d0e8eb

Let's see how the ci goes

8c6c447

Restore previous code

ae90f3f

Well, almost all new changes are revertable

2be9db9

ryoqun force-pushed the account-storage-exhaustion branch from 90b8c1b to 2be9db9 Compare August 11, 2020 16:21

ryoqun added 4 commits August 12, 2020 17:10

Update doc

69f0e9c

Add test and gating

884b5aa

Fix tests

3b987d2

Fix tests, especially avoid to change abi...

8eca8d3

ryoqun marked this pull request as ready for review August 13, 2020 04:31

ryoqun changed the title ~~wip: re-do rent collection check on rent-exempt account~~ Re-do rent collection check on rent-exempt account Aug 13, 2020

ryoqun requested a review from t-nelson August 13, 2020 04:32

ryoqun commented Aug 13, 2020

View reviewed changes

Fix more tests...

12a9bd5

Fix snapshot restore

7dd0f5f

t-nelson previously approved these changes Aug 13, 2020

View reviewed changes

Comment thread runtime/src/rent_collector.rs Outdated

ryoqun commented Aug 14, 2020

View reviewed changes

Align to _new_ with better uninitialized detection

047b43a

t-nelson self-requested a review August 14, 2020 07:44

t-nelson approved these changes Aug 14, 2020

View reviewed changes

ryoqun merged commit 23fa84b into solana-labs:master Aug 17, 2020

mergify Bot mentioned this pull request Aug 17, 2020

Re-do rent collection check on rent-exempt account (bp #11349) #11654

Merged

mergify Bot mentioned this pull request Aug 17, 2020

Re-do rent collection check on rent-exempt account (bp #11349) #11655

Merged

Conversation

ryoqun commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryoqun commented Aug 4, 2020

Uh oh!

mergify Bot commented Aug 4, 2020

Uh oh!

t-nelson left a comment

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryoqun commented Aug 13, 2020

Uh oh!

ryoqun Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoqun Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoqun Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

jstarry Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

ryoqun Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

t-nelson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryoqun Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

jackcmay commented Aug 14, 2020 via email

Uh oh!

ryoqun commented Aug 14, 2020

Uh oh!

rwalker-com commented Aug 14, 2020

Uh oh!

ryoqun commented Aug 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ryoqun commented Aug 3, 2020 •

edited

Loading

ryoqun commented Aug 11, 2020 •

edited

Loading

ryoqun Aug 13, 2020 •

edited

Loading

ryoqun Aug 13, 2020 •

edited

Loading

ryoqun Aug 13, 2020 •

edited

Loading

codecov Bot commented Aug 13, 2020 •

edited

Loading