Merge rust-bitcoin/rust-bitcoin#6879: primitives: Fix Witness::get index overflow
What the developer wrote
Authored by Andrew Poelstra
Merge rust-bitcoin/rust-bitcoin#6879: primitives: Fix Witness::get index overflow
ca6bf03d0b41521327e9041104612aaf538ff703 primitives: Test Witness::get index overflow (Jamil Lambert, PhD)
fb0bbc1dd77da590f0ab94b166b41dd942a6410e primitives: Fix Witness::get index overflow (Jamil Lambert, PhD)
Pull request description:
`Witness::get` passes a caller-controlled index to `decode_cursor`, which computes `start_of_indices + index * 4` with unchecked arithmetic. A large index such as `usize::MAX / 4 + 1` overflows: with overflow checks off it wraps back to element 0's offset and returns the first element instead of `None`, and with them on it panics.
Use checked arithmetic so an out-of-range index returns `None`.
Closes project-loupe/audit-rust-bitcoin#159
ACKs for top commit:
apoelstra:
ACK ca6bf03d0b41521327e9041104612aaf538ff703; successfully ran local tests
tcharding:
ACK ca6bf03d0b41521327e9041104612aaf538ff703
satsfy:
tACK ca6bf03d0b41521327e9041104612aaf538ff703
Tree-SHA512: f6538c5d214b8fa935ce14896d31e33eab6e640160160e152a76be486b62971f042edfd83eb62f5d9ff6ce6ab59f516cef6c676ce7e39259ac5af72d4049ee51
What changed, and why it matters
This commit fixes a bug in how the Bitcoin library reads items from a transaction witness. A function called Witness::get takes an index number from the caller and uses it to calculate a memory offset. With a very large index, the arithmetic could overflow, either returning the wrong witness item or causing the program to panic. The fix uses checked arithmetic so that any out-of-range index safely returns None. The bug was found during an external security audit.
Upgrade to a version of rust-bitcoin that includes this commit. If upgrading is not possible, avoid passing untrusted or unvalidated indices to Witness::get. No immediate incident response is required unless the application exposes witness indexing to untrusted input.
Security signals we found
Integer overflow in index calculation
Caller-controlled index used without bounds checking
Potential panic or incorrect data return from API
Fix uses checked arithmetic
Regression test added for overflow index
Linked to external audit finding
Evidence from the diff
In primitives/src/witness.rs, decode_cursor computed start_of_indices + index * 4 using unchecked arithmetic. A caller-controlled index such as usize::MAX / 4 + 1 could overflow, wrapping back to element 0’s offset in release builds or panicking in debug builds. The patch replaces the unchecked addition and multiplication with checked_add and checked_mul, so an out-of-range index short-circuits to None. A regression test is included.
Changed components
primitives/src/witness.rsWitness::getdecode_cursorInspect captured patch +9 / −1
### primitives/src/witness.rs
@@ -359,7 +359,7 @@ fn encode_cursor(bytes: &mut [u8], start_of_indices: usize, index: usize, value:
#[inline]
fn decode_cursor(bytes: &[u8], start_of_indices: usize, index: usize) -> Option<usize> {
- let start = start_of_indices + index * 4;
+ let start = start_of_indices.checked_add(index.checked_mul(4)?)?;
let pos = bytes.get_array::<4>(start).map(|index_bytes| u32::from_ne_bytes(*index_bytes))?;
usize::try_from(pos).ok()
}
@@ -1192,6 +1192,14 @@ mod test {
assert_eq!(witness.last(), Some(element_2));
}
+ #[test]
+ fn get_rejects_index_arithmetic_overflow() {
+ let witness = Witness::from([[0x42u8]]);
+ let wrapping_index = usize::MAX / 4 + 1;
+
+ assert_eq!(witness.get(wrapping_index), None);
+ }
+
#[test]
fn exact_sized_iterator() {
let arbitrary_element = [1_u8, 2, 3];Why this scored 51/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.