From b41ee639740e6968720db29671efc366821cda5e Mon Sep 17 00:00:00 2001 From: prostomarkeloff <28061158+prostomarkeloff@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:37:12 +0300 Subject: [PATCH] fix(qpack): reject post-base references at or above Required Insert Count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `VirtualAddressSpace::post_base` computed `base + index` without `checked_add` — both are peer-controlled prefix integers (up to ~2^63), so the sum can overflow and panic — and bounded the reference against the table's total inserted count rather than the block's Required Insert Count. RFC 9204 4.5.1 requires a reference whose absolute index is >= the declared Required Insert Count to be treated as QPACK_DECOMPRESSION_FAILED. Thread the Required Insert Count through DynamicTable::decoder into post_base (checked_add plus an `abs < required` ceiling) and relative_base, so an overflowing or out-of-window post-base index returns an error instead of panicking or decoding an entry the RFC requires to be rejected. Also fix decode_post_base_name_ref_header_field, which declared a Required Insert Count (2) smaller than the absolute index its post-base reference resolved to (3) — an RFC-invalid block that only decoded because of the missing check. --- h3/src/qpack/decoder.rs | 42 ++++++++++++++++++++++++++++++++---- h3/src/qpack/dynamic.rs | 19 ++++++++++++---- h3/src/qpack/vas.rs | 48 ++++++++++++++++++++++++++++++----------- 3 files changed, 88 insertions(+), 21 deletions(-) diff --git a/h3/src/qpack/decoder.rs b/h3/src/qpack/decoder.rs index 15cf68d3..68c66aa1 100644 --- a/h3/src/qpack/decoder.rs +++ b/h3/src/qpack/decoder.rs @@ -93,7 +93,7 @@ impl Decoder { return Err(DecoderError::MissingRefs(required_ref)); } - let decoder_table = self.table.decoder(base); + let decoder_table = self.table.decoder(base, required_ref); let mut mem_size = 0; let mut fields = Vec::new(); @@ -375,7 +375,7 @@ mod tests { assert!(decoder.on_encoder_recv(&mut enc, &mut dec).is_ok()); assert_eq!( - decoder.table.decoder(1).get_relative(0), + decoder.table.decoder(1, 1).get_relative(0), Ok(&StaticTable::get(1).unwrap().with_value("serial value")) ); @@ -443,7 +443,7 @@ mod tests { assert!(decoder.on_encoder_recv(&mut enc, &mut dec).is_ok()); assert_eq!( - decoder.table.decoder(1).get_relative(0), + decoder.table.decoder(1, 1).get_relative(0), Ok(&HeaderField::new("key", "value")) ); @@ -636,6 +636,35 @@ mod tests { assert_eq!(fields, &[field(2), field(3), field(4)]) } + #[test] + fn repro_post_base_overflow() { + // Peer-controlled: Required Insert Count = 1, Base = RIC + Delta Base ~ 2^63, + // post-base index ~ 2^63 -> base + index overflows usize in VirtualAddressSpace::post_base. + let base = 1 + (1usize << 63) + 126; + let mut buf = vec![]; + HeaderPrefix::new(1, base, 4, TABLE_SIZE).encode(&mut buf); + IndexedWithPostBase((1usize << 63) + 14).encode(&mut buf); + + let mut read = Cursor::new(&buf); + let decoder = Decoder::from(build_table_with_size(4)); + let res = decoder.decode_header(&mut read); + assert!(res.is_err(), "expected an error, got {res:?}"); + } + + #[test] + fn repro_post_base_above_required_insert_count() { + // RFC 9204 4.5.1: a reference with absolute index >= Required Insert Count + // MUST be treated as QPACK_DECOMPRESSION_FAILED. RIC = 2, Base = 2, post-base 0 -> abs 3. + let mut buf = vec![]; + HeaderPrefix::new(2, 2, 4, TABLE_SIZE).encode(&mut buf); + IndexedWithPostBase(0).encode(&mut buf); + + let mut read = Cursor::new(&buf); + let decoder = Decoder::from(build_table_with_size(4)); + let res = decoder.decode_header(&mut read); + assert!(res.is_err(), "expected an error, got {res:?}"); + } + #[test] fn decode_name_ref_header_field() { let mut buf = vec![]; @@ -665,7 +694,12 @@ mod tests { #[test] fn decode_post_base_name_ref_header_field() { let mut buf = vec![]; - HeaderPrefix::new(2, 2, 4, TABLE_SIZE).encode(&mut buf); + // Required Insert Count must cover the post-base reference's absolute index + // (base 2, post-base 0 -> entry foo3). The previous value (2) declared fewer + // entries than the block referenced, which RFC 9204 4.5.1 forbids; it only + // decoded because the post-base bounds check used the table's total inserted + // count instead of the Required Insert Count. + HeaderPrefix::new(4, 2, 4, TABLE_SIZE).encode(&mut buf); LiteralWithPostBaseNameRef::new(0, "new bar3") .encode(&mut buf) .unwrap(); diff --git a/h3/src/qpack/dynamic.rs b/h3/src/qpack/dynamic.rs index 08e6d17e..ce00e068 100644 --- a/h3/src/qpack/dynamic.rs +++ b/h3/src/qpack/dynamic.rs @@ -28,11 +28,15 @@ pub enum Error { pub struct DynamicTableDecoder<'a> { table: &'a DynamicTable, base: usize, + required_ref: usize, } impl<'a> DynamicTableDecoder<'a> { pub(super) fn get_relative(&self, index: usize) -> Result<&HeaderField, Error> { - let real_index = self.table.vas.relative_base(self.base, index)?; + let real_index = self + .table + .vas + .relative_base(self.base, index, self.required_ref)?; self.table .fields .get(real_index) @@ -40,7 +44,10 @@ impl<'a> DynamicTableDecoder<'a> { } pub(super) fn get_postbase(&self, index: usize) -> Result<&HeaderField, Error> { - let real_index = self.table.vas.post_base(self.base, index)?; + let real_index = self + .table + .vas + .post_base(self.base, index, self.required_ref)?; self.table .fields .get(real_index) @@ -258,8 +265,12 @@ impl DynamicTable { DynamicTable::default() } - pub fn decoder(&self, base: usize) -> DynamicTableDecoder<'_> { - DynamicTableDecoder { table: self, base } + pub fn decoder(&self, base: usize, required_ref: usize) -> DynamicTableDecoder<'_> { + DynamicTableDecoder { + table: self, + base, + required_ref, + } } pub fn encoder(&mut self, stream_id: u64) -> DynamicTableEncoder<'_> { diff --git a/h3/src/qpack/vas.rs b/h3/src/qpack/vas.rs index 8bfe8234..73ba733d 100644 --- a/h3/src/qpack/vas.rs +++ b/h3/src/qpack/vas.rs @@ -109,19 +109,41 @@ impl VirtualAddressSpace { index != 0 && index <= self.dropped } - pub fn relative_base(&self, base: usize, index: RelativeIndex) -> Result { - if self.delta == 0 || index > base || base - index <= self.dropped { + // `required` is the Required Insert Count of the header block. Per RFC 9204 §4.5.1, + // a reference to a dynamic entry whose absolute index is beyond the entries covered by + // the Required Insert Count must be rejected (QPACK_DECOMPRESSION_FAILED), so the + // absolute index (`base - index` here, 1-based) is bounded by `required`, not by the + // table's current `inserted` count. + pub fn relative_base( + &self, + base: usize, + index: RelativeIndex, + required: usize, + ) -> Result { + if self.delta == 0 + || index > base + || base - index <= self.dropped + || base - index > required + { Err(Error::RelativeIndex(index)) } else { Ok(base - self.dropped - index - 1) } } - pub fn post_base(&self, base: usize, index: RelativeIndex) -> Result { - if self.delta == 0 || base + index >= self.inserted || base + index < self.dropped { - Err(Error::PostbaseIndex(index)) - } else { - Ok(base + index - self.dropped) + pub fn post_base( + &self, + base: usize, + index: RelativeIndex, + required: usize, + ) -> Result { + // `base + index` is attacker-controlled and can overflow `usize`, hence `checked_add`. + // The absolute index must be below the Required Insert Count (RFC 9204 §4.5.1). + match base.checked_add(index) { + Some(abs) if self.delta != 0 && abs >= self.dropped && abs < required => { + Ok(abs - self.dropped) + } + _ => Err(Error::PostbaseIndex(index)), } } @@ -150,7 +172,7 @@ mod tests { #[test] fn test_no_relative_index_when_empty() { let vas = VirtualAddressSpace::default(); - let res = vas.relative_base(0, 0); + let res = vas.relative_base(0, 0, 0); assert_eq!(res, Err(Error::RelativeIndex(0))); } @@ -170,7 +192,7 @@ mod tests { vas.add(); (1..*count).for_each(|_| { vas.add(); }); - assert_eq!(vas.relative_base(*count, count - 1), Ok(0), "{:?}", vas); + assert_eq!(vas.relative_base(*count, count - 1, *count), Ok(0), "{:?}", vas); } #[test] @@ -182,7 +204,7 @@ mod tests { (1..*count).for_each(|_| { vas.add(); }); (0..*count - 1).for_each(|_| vas.drop()); - assert_eq!(vas.relative_base(*count, count - 1), Err(Error::RelativeIndex(count - 1)), "{:?}", vas); + assert_eq!(vas.relative_base(*count, count - 1, *count), Err(Error::RelativeIndex(count - 1)), "{:?}", vas); } #[test] @@ -193,7 +215,7 @@ mod tests { (1..*count).for_each(|_| { vas.add(); }); vas.add(); - assert_eq!(vas.relative_base(*count, 0), Ok(count -1), + assert_eq!(vas.relative_base(*count, 0, *count), Ok(count -1), "{:?}", vas); } @@ -206,7 +228,7 @@ mod tests { vas.add(); (0..*count - 1).for_each(|_| { vas.drop(); }); - assert_eq!(vas.relative_base(*count, 0), Ok(0), + assert_eq!(vas.relative_base(*count, 0, *count), Ok(0), "{:?}", vas); } } @@ -228,7 +250,7 @@ mod tests { vas.add(); }); - assert_eq!(vas.post_base(4, 1), Ok(5)); + assert_eq!(vas.post_base(4, 1, 7), Ok(5)); } #[test]