Patch bv for DoS due to malformed deserialization

[ryoqun]

Problem

There is a remotely-exploitable DoS vulnerability in validator.

This is caused with unsafes and a missing sanitization of the bv crate, which should catch the BitVec's internal invariant violation from the desrialization code path.

The deserialization of BitVec in gossip packets are under our limited_deserialize protection. Nonetheless our bincode-based limited_deserialize protection is ineffective because the memory allocation per-se isn't abnormal but its future access is by a bad pointer offset len.

Affected structs are Bloom (both master and v1.0 branches) and EpochSlots (only master).

Also, this is affects the sysvar SlotHistory, but which is usually protected by BankHash, so less severe than gossips.

Summary of Changes

• Cargo-[patch] a upstream's pending PR, thanks to @Mrmaxmeier's PR to address this.

Fixes #8873

[codecov]

Codecov Report

Merging #8882 into master will increase coverage by <.1%.
The diff coverage is 96.2%.

@@           Coverage Diff            @@
##           master   #8882     +/-   ##
========================================
+ Coverage    80.2%   80.2%   +<.1%     
========================================
  Files         267     267             
  Lines       57844   57871     +27     
========================================
+ Hits        46436   46462     +26     
- Misses      11408   11409      +1

[garious]

Can't depend on a branch like that. That'll add build nondeterminism if that user pushes more commits. That also created a security hole, as that user can now add a build.rs that runs on our CI hardware using your credentials. Also, depending on git will prevent the release automation from publishing to crates.io.

You need to fork the repo to solana-labs, create a release branch, push your fix there, tag it for release, and push the release to crates.io.

[Mrmaxmeier]

Can't depend on a branch like that. That'll add build nondeterminism if that user pushes more commits. That also created a security hole, as that user can now add a build.rs that runs on our CI hardware using your credentials.

AFAIK this is not an issue as git-dependencies are pinned to specific commits in the Cargo.lock file:

solana/Cargo.lock

Line 339 in 2d12606

source = "git+https://github.com/Mrmaxmeier/bv-rs?branch=deserialize-check-invariants#ec84d9427ebc70fecb2d1997bc93893a16fe597e"

[ryoqun]

@garious Thanks for head-ups! As @Mrmaxmeier said, I thought this is a safe; cargo fails before building even if --locked or --frozen isn't given like this for the git dependency (See below). And I locally pulled the upstrem's PR and checked commit hash and no suspicious diff, even before building locally. So thought there is no concern running in CI. Buf off course, I may be wrong. Those safe-by-default ad-hoc patching assumptions came from my experience from Ruby's bundler which heavily influenced the cargo. Maybe should I use the rev instead of branch in [patch] to be clear?

Anyway, I was careless a bit. I should have noted as such that this PR will be updated accordingly as soon as the upstream publishes a new release containing the fix. So the [patch] thing won't be merged. Sorry for misleading.

You need to fork the repo to solana-labs, create a release branch, push your fix there, tag it for release, and push the release to crates.io.

I thought forking in that way is an overkill if the upstream is responsibly and reasonably active (which is in this case, yay!). The maintainer indicated the interest to merge the PR. So the forking route will be avoided.

Build fails if the locked git revision doesn't match:

diff --git a/Cargo.lock b/Cargo.lock
index c1ff6c767..eb62a15ee 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -336,7 +336,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 [[package]]
 name = "bv"
 version = "0.11.1-alpha.0"
-source = "git+file:///home/ryoqun/work/bv-rs?branch=deserialize-check-invariants#a689add1163564464c812b9069729a3d27dcb32e"
+source = "git+file:///home/ryoqun/work/bv-rs?branch=deserialize-check-invariants#a689add1163564464c812b9069729a3d27dcb32f"
 dependencies = [
  "feature-probe 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "serde 1.0.104 (registry+https://github.com/rust-lang/crates.io-index)",

$ cargo run --bin solana -- --version
    Updating git repository `file:///home/ryoqun/work/bv-rs`
error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`

Caused by:
  failed to load source for a dependency on `bv`

Caused by:
  Unable to update file:///home/ryoqun/work/bv-rs?branch=deserialize-check-invariants#a689add1

Caused by:
  revspec 'a689add1163564464c812b9069729a3d27dcb32f' not found; class=Reference (4); code=NotFound (-3)

[mvines]

The maintainer indicated the interest to merge the PR. So the forking route will be avoided.

Sweet, that's perfect. Let's try to work upstream then! That's the best outcome

[ryoqun]

FYI: I just confimed this actually works; now our CI just refuses to build an unknown revision due to the rebased-then-force-pushed upstream PR's branch, which I re-audited as well. :)

[mvines]

Replaced by #8952