Merge rust-bitcoin#5050: primitives: Improve casts in witness module

7695ad8 primitives: Add private cast_to_usize_if_valid function (Tobin C. Harding)
61e6e5c Check position value when decoding cursor (Tobin C. Harding)
cb88804 Remove stale comment (Tobin C. Harding)

Pull request description:

  Two patches because the reason we can improve the casts is slightly different.


ACKs for top commit:
  apoelstra:
    ACK 7695ad8; successfully ran local tests
  jrakibi:
    ACK 7695ad8


Tree-SHA512: 965384cdb923f5be0eaf2ee7792cd589771105dcf6f46f6b342ad1cc092ff39763463e90abb0d8976ff785e07cd079ba46fa854dfb122b81f250f0544401da93

Author: apoelstra

primitives/src/witness.rs

@@ -230,9 +230,7 @@ impl Witness {
 
         let mut slice = &self.content[pos..]; // Start of element.
         let element_len = compact_size::decode_unchecked(&mut slice);
-        // Compact size should always fit into a u32 because of `MAX_SIZE` in Core.
-        // ref: https://github.com/rust-bitcoin/rust-bitcoin/issues/3264
-        let end = element_len as usize;
+        let end = cast_to_usize_if_valid(element_len)?;
         Some(&slice[..end])
     }
 
@@ -266,11 +264,11 @@ fn encode_cursor(bytes: &mut [u8], start_of_indices: usize, index: usize, value:
         .copy_from_slice(&u32::to_ne_bytes(value.try_into().expect("larger than u32")));
 }
 
-// This is duplicated in `bitcoin::blockdata::witness`, if you change them do so over there also.
 #[inline]
 fn decode_cursor(bytes: &[u8], start_of_indices: usize, index: usize) -> Option<usize> {
     let start = start_of_indices + index * 4;
-    bytes.get_array::<4>(start).map(|index_bytes| u32::from_ne_bytes(*index_bytes) as usize)
+    let pos = bytes.get_array::<4>(start).map(|index_bytes| u32::from_ne_bytes(*index_bytes))?;
+    usize::try_from(pos).ok()
 }
 
 /// The encoder for the [`Witness`] type.
@@ -562,9 +560,7 @@ impl<'a> Iterator for Iter<'a> {
         let index = decode_cursor(self.inner, self.indices_start, self.current_index)?;
         let mut slice = &self.inner[index..]; // Start of element.
         let element_len = compact_size::decode_unchecked(&mut slice);
-        // Compact size should always fit into a u32 because of `MAX_SIZE` in Core.
-        // ref: https://github.com/rust-bitcoin/rust-bitcoin/issues/3264
-        let end = element_len as usize;
+        let end = cast_to_usize_if_valid(element_len)?;
         self.current_index += 1;
         Some(&slice[..end])
     }
@@ -809,6 +805,27 @@ impl<'a> Arbitrary<'a> for Witness {
     }
 }
 
+/// Cast a decoded length prefix to a `usize`.
+///
+/// This function is basically just defensive. For all sane use cases the length prefix should be
+/// less than `MAX_VEC_SIZE` (on a 32-bit machine). If the value is bigger that `u16::MAX` and we
+/// are on a 16-bit machine you'll likely hit an error later anyway, better to just check it now.
+///
+/// # 16-bits
+///
+/// The compact size may be bigger than what can be represented in a `usize` on a 16-bit machine but
+/// this shouldn't happen if we created the witness because one would get an OOM error before that.
+fn cast_to_usize_if_valid(n: u64) -> Option<usize> {
+    /// Maximum size, in bytes, of a vector we are allowed to decode.
+    const MAX_VEC_SIZE: u64 = 4_000_000;
+
+    if n > MAX_VEC_SIZE {
+        return None;
+    }
+
+    usize::try_from(n).ok()
+}
+
 #[cfg(test)]
 mod test {
     #[cfg(feature = "alloc")]