advisory-db icon indicating copy to clipboard operation
advisory-db copied to clipboard

hpack is unmaintained and has a panic vulnerability in decoding

Open sno2 opened this issue 2 years ago • 7 comments

The hpack crate is unmaintained. The author, mlalic, has not responded to issues and PRs (including important bug fix) since its release 7 years ago. I am unable to contact the maintainer through other means due to LinkedIn requiring premium to message their account.

Furthermore, it includes 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)
     }
 }

sno2 avatar Sep 15 '23 21:09 sno2

Though at first glance the crate appears unmaintained, can you open a public GitHub issue asking about the maintenance status for confirmation?

tarcieri avatar Sep 15 '23 21:09 tarcieri

Out of the 3 issues and 2 PRs opened in the past 7 years in the hpack repository, the author has not messaged to any of them.

The same was already asked in the solicit crate by the same author 3 years ago to which they did not respond to.

I have posted a GitHub issue, but I cannot find any feedback from the author in either of these crate repositories since 7 years ago.

sno2 avatar Sep 15 '23 21:09 sno2

Per our unmaintained crates policy we like to be explicit about these inquiries.

Thanks for opening the issue.

tarcieri avatar Sep 15 '23 21:09 tarcieri

@tarcieri Am I able to file for the vulnerability with the proposed patch as a fix? Anyone is able to find vulnerable projects right now if they have access to this ticket.

sno2 avatar Sep 16 '23 15:09 sno2

Sure, that's fine

tarcieri avatar Sep 19 '23 16:09 tarcieri

@tarcieri Can the crate be considered unmaintained now? It has been over 90 days since I created the issue.

sno2 avatar Mar 06 '24 02:03 sno2

Sure!

tarcieri avatar Mar 06 '24 02:03 tarcieri