fix(deps): replace unmaintained rust-crypto with bitcoin::hashes::sha256, bump vulnerable deps#208
Conversation
EddieHouston
force-pushed
the
fix/cargo-audit-vulnerabilities
branch
from
566076b to
45e3daa
Compare
|
Have you considered using |
Looking into it to see if we get the same hardware acceleration 👍 , looks like it may depend on the version. |
Running some benches on a branch on my fork: https://github.com/EddieHouston/electrs/actions/runs/24655997795 Looks like the version of bitcoin_hashes::sha256 is equivalent to sha2 (and both are 5x faster than what we havd in rust-crypto). Will update to use bitcoin_hashes::sha256. Thanks! |
EddieHouston
changed the title
Per reviewer feedback on PR Blockstream#208: bitcoin_hashes::sha256 is already available via the bitcoin crate re-export and provides equivalent SHA-NI hardware acceleration. Drops the direct sha2 dependency. Bench (ubuntu-latest w/ SHA-NI, 1000 tx status hashes): rust-crypto 273.8 µs sha2 54.2 µs bitcoin_hashes 53.9 µs
EddieHouston
force-pushed
the
fix/cargo-audit-vulnerabilities
branch
from
6f607e9 to
8559ab3
Compare
RCasatta
left a comment
RCasatta
left a comment
There was a problem hiding this comment.
utACK 8559ab3 code review
would have been great to introduce unit tests with hardcoded values of hash_ip_with_salt and compute_script_hash in a separate commit before any other change, so that it's confirmed the following commit with the dep change doesn't change behaviour
…ded expected values Add test_compute_script_hash_p2pkh (calls compute_script_hash directly, asserts hardcoded FullHash bytes for a P2PKH scriptPubKey) and test_hash_ip_with_salt (extracts hash_ip_with_salt to a free function, tests known salt+ip → hex). These tests establish the behavioral contract before the rust-crypto dep swap, so the following commit can prove the new implementation is bit-for-bit identical.
Yep, good call for extra safety. Will redo the commits to incorporate this. |
…256, bump vulnerable deps rust-crypto 0.2 is unmaintained (last release 2016) and has a known AES miscomputation advisory (RUSTSEC-2022-0011). Its transitive dependency rustc-serialize has a stack overflow advisory (RUSTSEC-2022-0004) and is also unmaintained. Replace the three SHA-256 call sites (compute_script_hash in schema.rs and precache.rs, get_status_hash and hash_ip_with_salt in server.rs) with bitcoin::hashes::sha256, already re-exported from the bitcoin crate — avoids adding a new top-level dependency and keeps hashing consistent with the rest of the codebase. Also bumps tokio (1.49->1.52, RUSTSEC-2025-0023) and tar (0.4.44->0.4.45, RUSTSEC-2026-0068). Resolves 11 of 18 cargo-audit findings; the remaining 7 are pinned by upstream deps (electrum-client, electrumd, minreq) and require upstream releases. Adds NIST SHA-256 test vectors (empty, 'abc') verifying the new implementation against known-good values.
EddieHouston
dismissed stale reviews from RCasatta, shesek, Randy808, and philippem
via
b095dfc
EddieHouston
force-pushed
the
fix/cargo-audit-vulnerabilities
branch
from
8559ab3 to
b095dfc
Compare
Completed commit changes. |
5 participants
Summary
rust-cryptowithbitcoin::hashes::sha256for hardware-accelerated SHA-256 (SHA-NI on x86_64, ARMv8 crypto on aarch64)tokio1.49→1.52 andtar0.4.44→0.4.45 to resolve known vulnerabilitiescargo auditfindings; remaining 7 are pinned by upstream crates (electrum-client,electrumd,minreq)Motivation
cargo auditflagged 18 vulnerabilities and 15 warnings. The most actionable wasrust-crypto, a direct dependency used only for SHA-256 hashing in three places:compute_script_hashinsrc/new_index/schema.rsandsrc/new_index/precache.rsget_status_hashandhash_ip_with_saltinsrc/electrum/server.rsrust-cryptois unmaintained (last release 2016) and has a known AES miscomputation advisory (RUSTSEC-2022-0011). Its transitive dependencyrustc-serializehas a stack overflow advisory (RUSTSEC-2022-0004) and is also unmaintained.Migrated to
bitcoin::hashes::sha256(re-exported from thebitcoincrate, already a direct dependency) per reviewer feedback. This avoids adding a new top-level dependency and keeps hashing consistent with the rest of the codebase, which already usesbitcoin::hashesforsha256d,Midstate, etc.Benchmark
Independent micro-benchmark (GitHub Actions
ubuntu-latest, CPU with SHA-NI) comparing the pre-PR implementation against two modern alternatives. Workload: N strings of the form<txid>:<height>:fed into a single SHA-256 hasher, matchingthe
get_status_hashshape.rust-crypto(pre-PR)sha2bitcoin_hashesBoth modern implementations are ~5× faster than
rust-cryptoand within noise of each other on SHA-NI hardware. Benchmark source is on branchbench/sha256-comparisonfor reproducibility.Changes
Cargo.tomlrust-crypto = "0.2"; no new direct SHA-256 dep (usesbitcoin::hashes::sha256already in-tree)src/new_index/schema.rsbitcoin::hashes::sha256APIsrc/new_index/precache.rsbitcoin::hashes::sha256APIsrc/electrum/server.rsbitcoin::hashes::sha256APICargo.lockrust-crypto/rustc-serializetree, bumptokioandtarAdvisories resolved
rust-cryptobitcoin::hashes::sha256rustc-serializerust-crypto)bytescrossbeam-channelh2hyper-utilprotobufrocksdburltokiotarRemaining (upstream-blocked)
The 7 remaining advisories cannot be resolved without upstream releases. Most are in dev-only dependencies that do not ship in the production binary;
electrum-clientis the exception — it ships when theelectrum-discoveryfeature isenabled.
ring0.16.20electrum-client,electrumdelectrum-discovery)rustls0.16.0electrum-clientelectrum-discovery)webpki0.21.4electrum-clientelectrum-discovery)rustls0.19.1electrumd→ureqidna0.2.3electrumd→ureqrustls-webpki0.101.7minreq→corepc-nodeTest plan
cargo checkpasses (default features)cargo check --features liquidpassescargo test new_index::schema::tests— 3 unit tests pass:test_sha256_empty_input— NIST test vector for SHA-256("")test_sha256_abc— NIST test vector for SHA-256("abc")test_p2pkh_script_hash— real P2PKH scriptPubKey verified against independent SHA-256 computationcargo test -p electrs