Add some missing bounds checks. (#260)

src/frame/headers.rs

@@ -153,7 +153,9 @@ impl Headers {
 
         // Read the padding length
         if flags.is_padded() {
-            // TODO: Ensure payload is sized correctly
+            if src.len() < 1 {
+                return Err(Error::MalformedMessage);
+            }
             pad = src[0] as usize;
 
             // Drop the padding
@@ -162,6 +164,9 @@ impl Headers {
 
         // Read the stream dependency
         let stream_dep = if flags.is_priority() {
+            if src.len() < 5 {
+                return Err(Error::MalformedMessage);
+            }
             let stream_dep = StreamDependency::load(&src[..5])?;
 
             if stream_dep.dependency_id() == head.stream_id() {
@@ -290,6 +295,10 @@ impl PushPromise {
 
         // Read the padding length
         if flags.is_padded() {
+            if src.len() < 1 {
+                return Err(Error::MalformedMessage);
+            }
+
             // TODO: Ensure payload is sized correctly
             pad = src[0] as usize;
 
@@ -297,6 +306,10 @@ impl PushPromise {
             let _ = src.split_to(1);
         }
 
+        if src.len() < 5 {
+            return Err(Error::MalformedMessage);
+        }
+
         let (promised_id, _) = StreamId::parse(&src[..4]);
         // Drop promised_id bytes
         let _ = src.split_to(5);

src/hpack/header.rs

@@ -1,4 +1,4 @@
-use super::DecoderError;
+use super::{DecoderError, NeedMore};
 
 use bytes::Bytes;
 use http::{Method, StatusCode};
@@ -60,6 +60,9 @@ impl Header<Option<HeaderName>> {
 
 impl Header {
     pub fn new(name: Bytes, value: Bytes) -> Result<Header, DecoderError> {
+        if name.len() == 0 {
+            return Err(DecoderError::NeedMore(NeedMore::UnexpectedEndOfStream));
+        }
         if name[0] == b':' {
             match &name[1..] {
                 b"authority" => {

src/proto/connection.rs

@@ -315,7 +315,7 @@ where
                     // but should allow continuing to process current streams
                     // until they are all EOS. Once they are, State should
                     // transition to GoAway.
-                    self.streams.recv_go_away(&frame);
+                    self.streams.recv_go_away(&frame)?;
                     self.error = Some(frame.reason());
                 },
                 Some(Ping(frame)) => {

src/proto/streams/streams.rs

@@ -310,7 +310,7 @@ where
         last_processed_id
     }
 
-    pub fn recv_go_away(&mut self, frame: &frame::GoAway) {
+    pub fn recv_go_away(&mut self, frame: &frame::GoAway) -> Result<(), RecvError> {
         let mut me = self.inner.lock().unwrap();
         let me = &mut *me;
 
@@ -322,6 +322,17 @@ where
         let last_stream_id = frame.last_stream_id();
         let err = frame.reason().into();
 
+        if last_stream_id > actions.recv.max_stream_id() {
+            // The remote endpoint sent a `GOAWAY` frame indicating a stream
+            // that we never sent, or that we have already terminated on account
+            // of previous `GOAWAY` frame. In either case, that is illegal.
+            // (When sending multiple `GOAWAY`s, "Endpoints MUST NOT increase
+            // the value they send in the last stream identifier, since the
+            // peers might already have retried unprocessed requests on another
+            // connection.")
+            return Err(RecvError::Connection(Reason::PROTOCOL_ERROR));
+        }
+
         actions.recv.go_away(last_stream_id);
 
         me.store
@@ -337,6 +348,8 @@ where
             .unwrap();
 
         actions.conn_error = Some(err);
+
+        Ok(())
     }
 
     pub fn recv_eof(&mut self) {