Skip to content

Commit 2febe1a

Browse files
ThetaSinnerThetaSinner
committed
stream - bug fix for header semantics rule for header name case in hpack
1 parent 87c0bc0 commit 2febe1a

File tree

6 files changed

+153
-26
lines changed

6 files changed

+153
-26
lines changed

‎Cargo.toml

+2
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ threadpool = "1.6"
1919
openssl = { version = "^0.9.17", features = ["v110", "v102"] }
2020
tokio-openssl = { git = "https://github.com/ThetaSinner/tokio-openssl" }
2121

22+
regex = "0.2"
23+
2224
# http only
2325
httparse = "1.2.3"
2426

‎src/http2/error.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,8 @@ pub enum ErrorName {
121121
EvenStreamIdentiferOnClientInitiatedStream,
122122
ResetStreamFrameWithInvalidSize,
123123
WindowUpdateWouldCauseSendWindowToExceedLimit,
124-
InvalidFrameLengthForConnectionWindowUpdateFrame
124+
InvalidFrameLengthForConnectionWindowUpdateFrame,
125+
NonLowerCaseHeaderNameIsRejectedAsMalformed
125126
}
126127

127128
impl From<ErrorName> for Vec<u8> {
@@ -198,6 +199,9 @@ impl From<ErrorName> for Vec<u8> {
198199
},
199200
ErrorName::InvalidFrameLengthForConnectionWindowUpdateFrame => {
200201
"Invalid frame length for connection window update frame"
202+
},
203+
ErrorName::NonLowerCaseHeaderNameIsRejectedAsMalformed => {
204+
"Nonlower-case header name is rejected as malformed"
201205
}
202206
}.to_owned().as_bytes().to_vec()
203207
}

‎src/http2/header.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ pub enum HeaderValue {
4848

4949
#[derive(Debug)]
5050
pub struct Header {
51-
pub name: HeaderName,
51+
pub name: HeaderName,
5252
pub value: HeaderValue,
5353
allow_compression: bool
5454
}
@@ -177,16 +177,18 @@ impl fmt::Display for HeaderValue {
177177
}
178178
}
179179

180+
// Dear user, note that because http2 has stronger semantics for headers than hpack does. To allow for this
181+
// extra processing step we deserialise into custom headers only and will use enums later.
180182
impl header_trait::HpackHeaderTrait for Header {
181183
fn from_field_with_compression_flag(field: table::Field, allow_compression: bool) -> Self {
182-
let mut header = Header::new(field.name.as_str().into(), HeaderValue::Str(field.value));
184+
let mut header = Header::from_field(field);
183185
header.set_allow_compression(allow_compression);
184186

185187
header
186188
}
187189

188190
fn from_field(field: table::Field) -> Self {
189-
Header::new(field.name.as_str().into(), HeaderValue::Str(field.value))
191+
Header::new(HeaderName::CustomHeader(field.name), HeaderValue::Str(field.value))
190192
}
191193

192194
fn get_name(&self) -> String {

‎src/http2/stream/mod.rs

+94-15
Original file line numberDiff line numberDiff line change
@@ -159,27 +159,52 @@ impl Stream {
159159
let headers_frame = framing::headers::HeaderFrame::new(&frame.header, &mut frame.payload.into_iter());
160160
self.temp_header_block.extend(headers_frame.get_header_block_fragment());
161161

162+
let mut process_error = None;
163+
162164
// If the headers block is complete then unpack it immediately.
163165
if headers_frame.is_end_headers() {
164-
self.request.process_temp_header_block(self.temp_header_block.as_slice(), hpack_recv_context);
166+
process_error = self.request.process_temp_header_block(self.temp_header_block.as_slice(), hpack_recv_context);
165167

166168
// TODO this only removes values from the vector, it doesn't change the allocated capacity.
167169
self.temp_header_block.clear();
168170
}
169171

170-
if headers_frame.is_end_stream() {
172+
if process_error.is_some() {
173+
if let state::StreamStateName::Open(ref state) = new_state {
174+
(
175+
Some(
176+
state::StreamStateName::Closed(
177+
(
178+
state,
179+
state::StreamClosedInfo {
180+
reason: state::StreamClosedReason::ResetLocal
181+
}
182+
).into()
183+
)
184+
),
185+
process_error
186+
)
187+
}
188+
else {
189+
unreachable!("enum decomposition failed. How?");
190+
}
191+
}
192+
else if headers_frame.is_end_stream() {
171193
// This is an interesting consequence of using enums for wrapping states.
172194
// This can never fail, because we have explicitly changed state to open above.
173195
// But we still have to destructure.
174196
new_state = if let state::StreamStateName::Open(ref state) = new_state {
175-
state::StreamStateName::HalfClosedRemote(state.into())
197+
state::StreamStateName::HalfClosedRemote(state.into())
176198
}
177199
else {
178200
unreachable!("enum decomposition failed. How?");
179201
};
180-
}
181202

182-
(Some(new_state), None)
203+
(Some(new_state), None)
204+
}
205+
else {
206+
(Some(new_state), None)
207+
}
183208
},
184209
framing::FrameType::PushPromise => {
185210
// (8.2) A client cannot push. Thus, servers MUST treat the receipt of a
@@ -240,17 +265,35 @@ impl Stream {
240265
// it is this header frame which needs to be checked for end stream rather than the next one.
241266
let should_end_stream = self.should_headers_frame_end_stream();
242267

268+
let mut process_error = None;
269+
243270
if headers_frame.is_end_headers() {
244-
self.request.process_temp_header_block(self.temp_header_block.as_slice(), hpack_recv_context);
271+
process_error = self.request.process_temp_header_block(self.temp_header_block.as_slice(), hpack_recv_context);
245272

246273
// TODO this only removes values from the vector, it doesn't change the allocated capacity.
247274
self.temp_header_block.clear();
248275
}
249276

250-
// (8.1) An endpoint that receives a HEADERS frame without the END_STREAM
251-
// flag set after receiving a final (non-informational) status code MUST
252-
// treat the corresponding request or response as malformed (Section 8.1.2.6).
253-
if should_end_stream {
277+
if process_error.is_some() {
278+
(
279+
Some(
280+
state::StreamStateName::Closed(
281+
(
282+
state,
283+
state::StreamClosedInfo {
284+
reason: state::StreamClosedReason::ResetLocal
285+
}
286+
).into()
287+
)
288+
),
289+
process_error
290+
)
291+
}
292+
else if should_end_stream {
293+
// (8.1) An endpoint that receives a HEADERS frame without the END_STREAM
294+
// flag set after receiving a final (non-informational) status code MUST
295+
// treat the corresponding request or response as malformed (Section 8.1.2.6).
296+
254297
if headers_frame.is_end_stream() {
255298
(
256299
Some(
@@ -386,16 +429,34 @@ impl Stream {
386429
else {
387430
let continuation_frame = framing::continuation::ContinuationFrame::new(&frame.header, &mut frame.payload.into_iter());
388431
self.temp_header_block.extend(continuation_frame.get_header_block_fragment());
389-
432+
433+
let mut process_error = None;
390434
if continuation_frame.is_end_headers() {
391-
self.request.process_temp_header_block(self.temp_header_block.as_slice(), hpack_recv_context);
435+
process_error = self.request.process_temp_header_block(self.temp_header_block.as_slice(), hpack_recv_context);
392436

393437
// TODO this only removes values from the vector, it doesn't change the allocated capacity.
394438
self.temp_header_block.clear();
395439
}
396440

397441
// TODO handle continuation frame decode error.
398-
(None, None)
442+
if process_error.is_some() {
443+
(
444+
Some(
445+
state::StreamStateName::Closed(
446+
(
447+
state,
448+
state::StreamClosedInfo {
449+
reason: state::StreamClosedReason::ResetLocal
450+
}
451+
).into()
452+
)
453+
),
454+
process_error
455+
)
456+
}
457+
else {
458+
(None, None)
459+
}
399460
}
400461
},
401462
_ => {
@@ -444,15 +505,33 @@ impl Stream {
444505
let continuation_frame = framing::continuation::ContinuationFrame::new(&frame.header, &mut frame.payload.into_iter());
445506
self.temp_header_block.extend(continuation_frame.get_header_block_fragment());
446507

508+
let mut process_error = None;
447509
if continuation_frame.is_end_headers() {
448-
self.request.process_temp_header_block(self.temp_header_block.as_slice(), hpack_recv_context);
510+
process_error = self.request.process_temp_header_block(self.temp_header_block.as_slice(), hpack_recv_context);
449511

450512
// TODO this only removes values from the vector, it doesn't change the allocated capacity.
451513
self.temp_header_block.clear();
452514
}
453515

454516
// TODO handle continuation frame decode error.
455-
(None, None)
517+
if process_error.is_some() {
518+
(
519+
Some(
520+
state::StreamStateName::Closed(
521+
(
522+
state,
523+
state::StreamClosedInfo {
524+
reason: state::StreamClosedReason::ResetLocal
525+
}
526+
).into()
527+
)
528+
),
529+
process_error
530+
)
531+
}
532+
else {
533+
(None, None)
534+
}
456535
}
457536
},
458537
framing::FrameType::WindowUpdate => {

‎src/http2/stream/stream_request.rs

+45-7
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,14 @@
1515
// You should have received a copy of the GNU General Public License
1616
// along with Osmium. If not, see <http://www.gnu.org/licenses/>.
1717

18+
// regex
19+
use regex::Regex;
20+
1821
// osmium
1922
use http2::header;
2023
use http2::hpack::context as hpack_context;
2124
use http2::hpack::unpack as hpack_unpack;
25+
use http2::error;
2226

2327
#[derive(Debug)]
2428
pub struct StreamRequest {
@@ -36,35 +40,69 @@ impl StreamRequest {
3640
}
3741
}
3842

39-
// TODO will return an error.
40-
pub fn process_temp_header_block(&mut self, temp_header_block: &[u8], hpack_recv_context: &mut hpack_context::RecvContext) {
43+
pub fn process_temp_header_block(&mut self, temp_header_block: &[u8], hpack_recv_context: &mut hpack_context::RecvContext) -> Option<error::HttpError> {
4144
let mut decoded = hpack_unpack::UnpackedHeaders::<header::Header>::new();
4245
hpack_unpack::unpack(temp_header_block, hpack_recv_context, &mut decoded);
4346

4447
// TODO can the header block be empty? because that will break the logic below.
4548

4649
if self.headers.is_empty() {
4750
// If no request headers have been received then these are the request headers.
48-
self.headers = hpack_to_http2_headers(decoded.headers);
51+
match hpack_to_http2_headers(decoded.headers) {
52+
Ok(headers) => {
53+
self.headers = headers;
54+
},
55+
Err(e) => return Some(e)
56+
}
4957
}
5058
else if self.trailer_headers.is_none() {
5159
// If no trailer headers have been received then these are the tailer headers.
52-
self.trailer_headers = Some(hpack_to_http2_headers(decoded.headers));
60+
match hpack_to_http2_headers(decoded.headers) {
61+
Ok(headers) => {
62+
self.trailer_headers = Some(headers);
63+
},
64+
Err(e) => return Some(e)
65+
}
5366
}
5467
else {
5568
// TODO handle error. We have received all the header blocks we were expecting, but received
5669
// a request to process another.
5770
panic!("unexpected header block");
5871
}
72+
73+
None
5974
}
6075
}
6176

62-
fn hpack_to_http2_headers(hpack_headers: Vec<header::Header>) -> header::Headers {
77+
fn hpack_to_http2_headers(hpack_headers: Vec<header::Header>) -> Result<header::Headers, error::HttpError> {
6378
let mut headers = header::Headers::new();
6479

65-
for header in hpack_headers.into_iter() {
80+
// TODO move to setup
81+
let header_name_regex = Regex::new(r"^:?[!#$%&'*+\-.^_`|~0-9a-z]+$").unwrap();
82+
83+
// TODO this should be a move and not need mut?
84+
for mut header in hpack_headers.into_iter() {
85+
let new_name = match header.name {
86+
header::HeaderName::CustomHeader(name) => {
87+
// TODO this could be better, seeing as is has to be done many times
88+
if !header_name_regex.is_match(String::from(name.clone()).as_str()) {
89+
error!("Rejecting header because of bad name {:?}", name);
90+
return Err(error::HttpError::StreamError(
91+
error::ErrorCode::ProtocolError,
92+
error::ErrorName::NonLowerCaseHeaderNameIsRejectedAsMalformed
93+
));
94+
}
95+
96+
name.as_str().into()
97+
},
98+
_ => {
99+
panic!("incorrect use of hpack");
100+
}
101+
};
102+
103+
header.name = new_name;
66104
headers.push_header(header);
67105
}
68106

69-
headers
107+
Ok(headers)
70108
}

‎src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ extern crate threadpool;
3030
extern crate openssl;
3131
extern crate tokio_openssl;
3232

33+
extern crate regex;
34+
3335
pub mod http_version;
3436
pub mod http;
3537
pub mod http2;

0 commit comments

Comments
 (0)