Merge rust-bitcoin/rust-bitcoin#6858: base58: saturate the reserve len multiplication
What the developer wrote
Authored by Andrew Poelstra
Merge rust-bitcoin/rust-bitcoin#6858: base58: saturate the reserve len multiplication
898da6ed12fcef49180d4b622d1a0a4dd5850f3d base58: saturate the reserve len multiplication (satsfy (Renato Britto))
Pull request description:
Use a `saturating_mul()` to handle overflows on `encoded_reserve_len`.
Didn't think this was worth a test because of the input needed to overflow this isn't allocatable on 64-bit and debug already catches it, so this is just fixing the release wrap.
Per https://github.com/project-loupe/audit-rust-bitcoin/issues/60
_This finding was discovered by [Project Loupe](https://github.com/project-loupe/loupe)._
ACKs for top commit:
apoelstra:
ACK 898da6ed12fcef49180d4b622d1a0a4dd5850f3d; successfully ran local tests
Tree-SHA512: 399b3e503a800de672ce8661b014d9ea2b60081ea427701542736f1e511167195a9c38415fe028edfe556d5f7961566f52580fa61c2941abe0a1d96a3e490779
What changed, and why it matters
This commit fixes a tiny but real arithmetic bug in the library's base58 string encoder. When asked to reserve memory for an extremely large encoded string, the old code could multiply two numbers together and silently wrap around to a small value on release builds (integer overflow), causing the program to reserve far less memory than needed and then write past the end. The fix uses saturating multiplication so the value stays at the maximum safe size instead of wrapping. The practical impact is limited because the input sizes needed to trigger this are not realistically allocatable on 64-bit systems, and debug builds already panic on overflow.
No immediate action beyond applying the patch. Users on current releases should update when convenient. Developers should consider adding a regression test for the overflow edge case even though the reporter deemed it impractical, and audit similar reserve_len-style calculations elsewhere in the crate for the same pattern.
Security signals we found
Integer overflow in buffer-size calculation
Potential heap buffer overrun due to undersized allocation
Release-build-only wraparound (debug builds already panic)
Third-party security audit finding (Project Loupe)
Saturating arithmetic used as fix
Evidence from the diff
In base58/src/lib.rs, encoded_reserve_len() changed from (unencoded_len * 137).div_ceil(100) to unencoded_len.saturating_mul(137).div_ceil(100). On release builds, usize multiplication wraps on overflow, so a crafted very large unencoded_len could cause encoded_reserve_len() to return a small buffer size. Subsequent encoding could then overrun the allocated Vec. With saturating_mul(), the product clamps to usize::MAX, so div_ceil(100) still yields a huge (but not wrapped) length, and the allocation will fail safely rather than creating an undersized buffer. This is a defense-in-depth fix for an integer overflow in memory-reservation sizing.
Changed components
base58/src/lib.rsencoded_reserve_len()Base58 encoding with allocation featureInspect captured patch +1 / −1
### base58/src/lib.rs
@@ -438,7 +438,7 @@ impl fmt::Debug for Base58CkString {
#[cfg(feature = "alloc")]
const fn encoded_reserve_len(unencoded_len: usize) -> usize {
// log2(256) / log2(58) ~ 1.37 = 137 / 100
- (unencoded_len * 137).div_ceil(100)
+ unencoded_len.saturating_mul(137).div_ceil(100)
}
/// Returns the length to reserve when encoding base58 with checksumWhy this scored 37/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.