From 79003d0d459b43d939a1b4849ed72621a70f327f Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Tue, 14 Nov 2017 11:16:29 -0800 Subject: [PATCH] reject connection-specific headers (#173) - When receiving, return a PROTOCOL_ERROR. - When sending, return a user error about malformed headers. Closes #36 --- src/codec/error.rs | 4 ++++ src/frame/headers.rs | 7 ++++++- src/proto/streams/send.rs | 20 ++++++++++++++++++++ tests/client_request.rs | 37 +++++++++++++++++++++++++++++++++++++ tests/server.rs | 37 ++++++++++++++++++++++++++++++++++++- tests/support/frames.rs | 11 +++++++++++ 6 files changed, 114 insertions(+), 2 deletions(-) diff --git a/src/codec/error.rs b/src/codec/error.rs index e2cc705..8d5d098 100644 --- a/src/codec/error.rs +++ b/src/codec/error.rs @@ -44,6 +44,9 @@ pub enum UserError { /// /// A new connection is needed. OverflowedStreamId, + + /// Illegal headers, such as connection-specific headers. + MalformedHeaders, } // ===== impl RecvError ===== @@ -121,6 +124,7 @@ impl error::Error for UserError { Rejected => "rejected", ReleaseCapacityTooBig => "release capacity too big", OverflowedStreamId => "stream ID overflowed", + MalformedHeaders => "malformed headers", } } } diff --git a/src/frame/headers.rs b/src/frame/headers.rs index ad92fba..8cd27e2 100644 --- a/src/frame/headers.rs +++ b/src/frame/headers.rs @@ -635,7 +635,12 @@ impl HeaderBlock { // Connection level header fields are not supported and must // result in a protocol error. - if name == header::CONNECTION { + if name == header::CONNECTION + || name == header::TRANSFER_ENCODING + || name == header::UPGRADE + || name == "keep-alive" + || name == "proxy-connection" + { trace!("load_hpack; connection level header"); malformed = true; } else if name == header::TE && value != "trailers" { diff --git a/src/proto/streams/send.rs b/src/proto/streams/send.rs index 027d20c..431e3e6 100644 --- a/src/proto/streams/send.rs +++ b/src/proto/streams/send.rs @@ -1,3 +1,4 @@ +use http; use super::*; use codec::{RecvError, UserError}; use codec::UserError::*; @@ -56,6 +57,25 @@ impl Send { self.init_window_sz ); + // 8.1.2.2. Connection-Specific Header Fields + if frame.fields().contains_key(http::header::CONNECTION) + || frame.fields().contains_key(http::header::TRANSFER_ENCODING) + || frame.fields().contains_key(http::header::UPGRADE) + || frame.fields().contains_key("keep-alive") + || frame.fields().contains_key("proxy-connection") + { + debug!("illegal connection-specific headers found"); + return Err(UserError::MalformedHeaders); + } else if let Some(te) = frame.fields().get(http::header::TE) { + if te != "trailers" { + debug!("illegal connection-specific headers found"); + return Err(UserError::MalformedHeaders); + + } + } + + + let end_stream = frame.is_end_stream(); // Update the state diff --git a/tests/client_request.rs b/tests/client_request.rs index 1c1640e..9c445fb 100644 --- a/tests/client_request.rs +++ b/tests/client_request.rs @@ -281,6 +281,43 @@ fn request_without_scheme() {} #[ignore] fn request_with_h1_version() {} +#[test] +fn request_with_connection_headers() { + let _ = ::env_logger::init(); + let (io, srv) = mock::new(); + + let srv = srv.assert_client_handshake() + .unwrap() + .recv_settings() + .close(); + + let headers = vec![ + ("connection", "foo"), + ("keep-alive", "5"), + ("proxy-connection", "bar"), + ("transfer-encoding", "chunked"), + ("upgrade", "HTTP/2.0"), + ("te", "boom"), + ]; + + let client = Client::handshake(io) + .expect("handshake") + .and_then(move |(mut client, conn)| { + for (name, val) in headers { + let req = Request::builder() + .uri("https://http2.akamai.com/") + .header(name, val) + .body(()) + .unwrap(); + let err = client.send_request(req, true).expect_err(name); + + assert_eq!(err.to_string(), "user error: malformed headers"); + } + conn.unwrap() + }); + + client.join(srv).wait().expect("wait"); +} #[test] fn sending_request_on_closed_connection() { diff --git a/tests/server.rs b/tests/server.rs index 61f3eb0..fb052a6 100644 --- a/tests/server.rs +++ b/tests/server.rs @@ -110,7 +110,7 @@ fn serve_request() { fn accept_with_pending_connections_after_socket_close() {} #[test] -fn sent_invalid_authority() { +fn recv_invalid_authority() { let _ = ::env_logger::init(); let (io, client) = mock::new(); @@ -136,6 +136,41 @@ fn sent_invalid_authority() { srv.join(client).wait().expect("wait"); } +#[test] +fn recv_connection_header() { + let _ = ::env_logger::init(); + let (io, client) = mock::new(); + + let req = |id, name, val| { + frames::headers(id) + .request("GET", "https://example.com/") + .field(name, val) + .eos() + }; + + let client = client + .assert_server_handshake() + .unwrap() + .recv_settings() + .send_frame(req(1, "connection", "foo")) + .send_frame(req(3, "keep-alive", "5")) + .send_frame(req(5, "proxy-connection", "bar")) + .send_frame(req(7, "transfer-encoding", "chunked")) + .send_frame(req(9, "upgrade", "HTTP/2.0")) + .recv_frame(frames::reset(1).protocol_error()) + .recv_frame(frames::reset(3).protocol_error()) + .recv_frame(frames::reset(5).protocol_error()) + .recv_frame(frames::reset(7).protocol_error()) + .recv_frame(frames::reset(9).protocol_error()) + .close(); + + let srv = Server::handshake(io) + .expect("handshake") + .and_then(|srv| srv.into_future().unwrap()); + + srv.join(client).wait().expect("wait"); +} + #[test] fn sends_reset_cancel_when_body_is_dropped() { let _ = ::env_logger::init(); diff --git a/tests/support/frames.rs b/tests/support/frames.rs index 21f2b1b..f8ca36e 100644 --- a/tests/support/frames.rs +++ b/tests/support/frames.rs @@ -126,6 +126,17 @@ impl Mock { Mock(frame) } + pub fn field(self, key: K, value: V) -> Self + where + K: HttpTryInto, + V: HttpTryInto, + { + let (id, pseudo, mut fields) = self.into_parts(); + fields.insert(key.try_into().unwrap(), value.try_into().unwrap()); + let frame = frame::Headers::new(id, pseudo, fields); + Mock(frame) + } + pub fn eos(mut self) -> Self { self.0.set_end_stream(); self