snapshots security may need to be improved

[sakridge]

Problem

snapshots are untrusted data, but the snapshot ingestion is not secure in a few ways.

Proposed Solution

Audit snapshot ingestion path for security issues:

• Incorrect accounts view due to 0-lamport accounts
• Deserialize hardening (capped allocation sizes for vecs/hashmaps)
• Checks for incorrect sizes around AppendVec mmaps, such that sizes of mmap data can contain all accounts and matches file size and all account data doesn't overflow mmap size.
• other?

Towards #6727, related to #6936

[ryoqun]

@sakridge I've started looking into this. Additional findings so far (might be wrong; so reviews are welcome!):

• Untaring snapshot.tar.bz2 doesn't create a sparse file; So it will be quite easy to fill up disk space with a small-sized crafted tar file, and throttle disk write throughput with meaningless zero-fill io reqs.
• Doesn't check unused area in AppendVec is zero-filled; Fill it random data then validators will waste CPU when trying bzipping it next time creating newer snapshot. Or, fill the area with some legally troublesome data and make victim validators distribute it indefinitely.
• snapshot.tar.bz2 can contain any unrelated files. So could exhaust inodes depending on the FS the accounts dir is residing on. (done Limit extracted data size from genesis.tar.bz2 and snapshot.tar.bz2 #8427).

[sakridge]

@sakridge I've started looking into this. Additional findings so far (might be wrong; so reviews are welcome!):

• Untaring snapshot.tar.bz2 doesn't create a sparse file; So it will be quite easy to fill up disk space with a small-sized crafted tar file, and throttle disk write throughput with meaningless zero-fill io reqs.
• Doesn't check unused area in AppendVec is zero-filled; Fill it random data then validators will waste CPU when trying bzipping it next time creating newer snapshot. Or, fill the area with some legally troublesome data and make victim validators distribute it indefinitely.
• snapshot.tar.bz2 can contain any unrelated files. So could exhaust inodes depending on the FS the accounts dir is residing on.

Yea. These seem like good issues, essentially we should try to detect a zip bomb and not unzip it if it is. The data zero-ing one is maybe lower priority, but probably a good idea since it is probably inexpensive.

[ryoqun]

Another ones:

• Snapshot can contain sysvar accounts without affecting bank_hash
• accounts.executable and accounts.rent_epoch is not included, could freely mutate them?

(Once, I've finished searching remaining problems, I'll triage them and create a compiled list)

[ryoqun]

Moar:

• Snapshot transfer is unprotected (= plain old HTTP 1.1), possible MITM attacks? (Maybe sign the snapshot tar ball with Validator's pubkey?) (Ultimately, snapshot verifcation Add snapshot booting proposal #6936 solves this, but this makes network intermediaries be able to meddle with poor validators?) Maybe targeted attacks can be conjured up?

[sakridge]

Another ones:

• Snapshot can contain sysvar accounts without affecting bank_hash
• accounts.executable and accounts.rent_epoch is not included, could freely mutate them?

(Once, I've finished searching remaining problems, I'll triage them and create a compiled list)

Do you mean system accounts? Those should come from the validator software itself, but we should definitely check that they cannot be shielded by a snapshot version.

We should add the executable and rent_epoch there as well.

[ryoqun]

• how are status_cache and other bank struct fields protected??

[ryoqun]

• [zip bomb] Also limit appendvec files to be used because it consumes file descriptors one by one due to mmap. ;)

[t-nelson]

@TristanDebrunner do you have 👀 on this?

[TristanDebrunner]

@TristanDebrunner do you have 👀 on this?

Yep, thanks @t-nelson!

[ryoqun]

Moar:

• Snapshot transfer is unprotected (= plain old HTTP 1.1), possible MITM attacks? (Maybe sign the snapshot tar ball with Validator's pubkey?) (Ultimately, snapshot verifcation Add snapshot booting proposal #6936 solves this, but this makes network intermediaries be able to meddle with poor validators?) Maybe targeted attacks can be conjured up?

To elaborate on this a bit, even if a validator knows priori a set of trusted validators, currently the validator is forced to do the full round of un-.tar.bz2-ing large legimately-looking file and restoring the process full state and running a SPV against the cluster just to find that the file may be tampered by any malicious party of the network intermediaries of the whole spectrum from L1- to L7- layers.

In other words, unless snapshot itself is signed, validators are exposed the wasted computing resource, potential vulnerabilities in the snapshot sanitization (which we'll try very hard to get done right), wasted lamports for extra TXes and general DOS attack when restarting.

To sign a snapshot, accompany it with a .asc PGP file or add a custom HTTP header?

[ryoqun]

Also, this is also affected as one of API endpoints by #5778.

I dunno current behavior for our HTTP server code for classic slow/stale malicious clients and too many concurrent requests etc. I'm pretty sure there are many doable common attack.

For the particularity of snapshots, it's also worth nothing that any stale clients may make validator hold indefinitely old file descriptors for unlinked snapshot files, which may cause disk-full, if the attack done persistently enough.

Also, because snapshot archive files tend to be large, it's quite easy overwhelm the validator's outgoing bandwidth with just ab, which could impair staked validator's leader functioning.

[ryoqun]

• overflow of various variables. write_version, signature_count, ...

[ryoqun]

• Address this: Do periodic inbound cleaning for rooted slots #8436 (comment):

Well, I just noticed that when snapshot generation is disabled, this new compaction and existing purge_zero_lamports aren't run never... This could exhaust system resources..

[ryoqun]

• normalize 0-lamport account in AppendVec with Account::default(); otherwise, myriad of funny tricks are possible. (done Sanitize zero lamport accounts in append vecs #9083)

[ryoqun]

• Reject any symlinks to snapshots (especially, /path/to/validator-keypair.json)!! (done via Strictly validate the contents of snapshot/genesis #8959)

[ryoqun]

• Audit the tar crate more thoroughly. (from Strictly validate the contents of snapshot/genesis #8959).

[ryoqun]

https://github.com/solana-labs/solana/pull/9219/files#r403398028

(By the way, I noticed generate_index is quadratic in that regard while working on this. but this is a different issue...)

• mitigate this?

[ryoqun]

• check the permission of files in the archive; rwxrwxrwx isn't something we want in it...

[ryoqun]

• hash account.owner... similar to snapshots security may need to be improved #7167 (comment) (done by Include account.owner into account hash #9917)