Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic vulnerability in decoding #11

Open
sno2 opened this issue Mar 11, 2024 · 0 comments
Open

Panic vulnerability in decoding #11

sno2 opened this issue Mar 11, 2024 · 0 comments

Comments

@sno2
Copy link

sno2 commented Mar 11, 2024

hpack has a panic vulnerability in the Decoder struct because it does not validate the buffer length is long enough to parse an integer in the update_max_dynamic_size function after seeing a SizeUpdate field. Here is a minimal reproduction:

use hpack::Decoder;

pub fn main() {
  let input = &[0x3f];
  let mut decoder = Decoder::new();
  let _ = decoder.decode(input);
}

All users who try to decode untrusted input using the Decoder are vulnerable to this exploit. A patched version of the crate is available on crates.io under the name hpack-patched. See Cargo's documentation on overriding dependencies for more information.


The following Git patch fixes the issue:

diff --git a/src/decoder.rs b/src/decoder.rs
index dc4f0c2..55fff45 100644
--- a/src/decoder.rs
+++ b/src/decoder.rs
@@ -359,7 +359,7 @@ impl<'a> Decoder<'a> {
                 },
                 FieldRepresentation::SizeUpdate => {
                     // Handle the dynamic table size update...
-                    self.update_max_dynamic_size(buffer_leftover)
+                    self.update_max_dynamic_size(buffer_leftover)?
                 }
             };
 
@@ -445,19 +445,16 @@ impl<'a> Decoder<'a> {
     /// size of the underlying dynamic table, possibly causing a number of
     /// headers to be evicted from it.
     ///
-    /// Assumes that the first byte in the given buffer `buf` is the first
-    /// octet in the `SizeUpdate` block.
-    ///
     /// Returns the number of octets consumed from the given buffer.
-    fn update_max_dynamic_size(&mut self, buf: &[u8]) -> usize {
-        let (new_size, consumed) = decode_integer(buf, 5).ok().unwrap();
+    fn update_max_dynamic_size(&mut self, buf: &[u8]) -> Result<usize, DecoderError> {
+        let (new_size, consumed) = decode_integer(buf, 5)?;
         self.header_table.dynamic_table.set_max_table_size(new_size);
 
         info!("Decoder changed max table size from {} to {}",
               self.header_table.dynamic_table.get_size(),
               new_size);
 
-        consumed
+        Ok(consumed)
     }
 }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant