forked/hyper
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(header): security fix for header values that include newlines

Browse Source 
Newlines in header values will now be replaced with spaces when being
written to strings or to sockets. This prevents headers that are built
from user data to smuggle unintended headers or requests/responses.

Thanks to @skylerberg for the responsible reporting of this issue, and
helping to keep us all safe!

BREAKING CHANGE: This technically will cause code that a calls
  `SetCookie.fmt_header` to panic, as it is no longer to properly write
  that method. Most people should not be doing this at all, and all
  other ways of printing headers should work just fine.

  The breaking change must occur in a patch version because of the
  security nature of the fix.
This commit is contained in:
Sean McArthur
2017-01-19 12:53:08 -08:00
parent 7d400398ab
commit 8e790831c1
3 changed files with 146 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
src/header/common/set_cookie.rs
View File

@@ -1,5 +1,5 @@
use header::{Header, Raw};
use std::fmt::{self, Display};
use std::fmt;
use std::str::from_utf8;
@@ -90,14 +90,26 @@ impl Header for SetCookie {
Err(::Error::Header)
}
}
fn fmt_header(&self, _f: &mut fmt::Formatter) -> fmt::Result {
panic!("SetCookie cannot be used with fmt_header, must use fmt_multi_header");
}
fn fmt_header(&self, f: &mut fmt::Formatter) -> fmt::Result {
for (i, cookie) in self.0.iter().enumerate() {
if i != 0 {
try!(f.write_str("\r\nSet-Cookie: "));
}
try!(Display::fmt(cookie, f));
fn fmt_multi_header(&self, f: &mut ::header::MultilineFormatter) -> fmt::Result {
for cookie in &self.0 {
try!(f.fmt_line(cookie));
}
Ok(())
}
}
#[test]
fn test_set_cookie_fmt() {
use ::header::Headers;
let mut headers = Headers::new();
headers.set(SetCookie(vec![
"foo=bar".into(),
"baz=quux".into(),
]));
assert_eq!(headers.to_string(), "Set-Cookie: foo=bar\r\nSet-Cookie: baz=quux\r\n");
}

46
src/header/internals/item.rs
View File

@@ -4,7 +4,7 @@ use std::fmt;
use std::str::from_utf8;
use super::cell::{OptCell, PtrMapCell};
use header::{Header, Raw};
use header::{Header, MultilineFormatter, Raw};
#[derive(Clone)]
@@ -90,6 +90,29 @@ impl Item {
None => parse::<H>(self.raw.as_ref().expect("item.raw must exist")).ok()
}.map(|typed| unsafe { typed.downcast_unchecked() })
}
pub fn write_h1(&self, f: &mut MultilineFormatter) -> fmt::Result {
match *self.raw {
Some(ref raw) => {
for part in raw.iter() {
match from_utf8(&part[..]) {
Ok(s) => {
try!(f.fmt_line(&s));
},
Err(_) => {
error!("raw header value is not utf8, value={:?}", part);
return Err(fmt::Error);
}
}
}
Ok(())
},
None => {
let typed = unsafe { self.typed.one() };
typed.fmt_multi_header(f)
}
}
}
}
#[inline]
@@ -99,24 +122,3 @@ fn parse<H: Header>(raw: &Raw) -> ::Result<Box<Header + Send + Sync>> {
h
})
}
impl fmt::Display for Item {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match *self.raw {
Some(ref raw) => {
for part in raw.iter() {
match from_utf8(&part[..]) {
Ok(s) => try!(f.write_str(s)),
Err(e) => {
error!("raw header value is not utf8. header={:?}, error={:?}",
part, e);
return Err(fmt::Error);
}
}
}
Ok(())
},
None => fmt::Display::fmt(&unsafe { self.typed.one() }, f)
}
}
}

109
src/header/mod.rs
View File

@@ -118,6 +118,19 @@ pub trait Header: HeaderClone + Any + GetType + Send + Sync {
/// This method is not allowed to introduce an Err not produced
/// by the passed-in Formatter.
fn fmt_header(&self, f: &mut fmt::Formatter) -> fmt::Result;
/// Formats a header over multiple lines.
///
/// The main example here is `Set-Cookie`, which requires that every
/// cookie being set be specified in a separate line.
///
/// The API here is still being explored, so this is hidden by default.
/// The passed in formatter doesn't have any public methods, so it would
/// be quite difficult to depend on this externally.
#[doc(hidden)]
#[inline]
fn fmt_multi_header(&self, f: &mut MultilineFormatter) -> fmt::Result {
f.fmt_line(&FmtHeader(self))
}
}
#[doc(hidden)]
@@ -147,6 +160,74 @@ fn test_get_type() {
assert_eq!(TypeId::of::<UserAgent>(), (*agent).get_type());
}
#[doc(hidden)]
#[allow(missing_debug_implementations)]
pub struct MultilineFormatter<'a, 'b: 'a>(Multi<'a, 'b>);
enum Multi<'a, 'b: 'a> {
Line(&'a str, &'a mut fmt::Formatter<'b>),
Join(bool, &'a mut fmt::Formatter<'b>),
}
impl<'a, 'b> MultilineFormatter<'a, 'b> {
fn fmt_line(&mut self, line: &fmt::Display) -> fmt::Result {
use std::fmt::Write;
match self.0 {
Multi::Line(ref name, ref mut f) => {
try!(f.write_str(*name));
try!(f.write_str(": "));
try!(write!(NewlineReplacer(*f), "{}", line));
f.write_str("\r\n")
},
Multi::Join(ref mut first, ref mut f) => {
if !*first {
try!(f.write_str(", "));
} else {
*first = false;
}
write!(NewlineReplacer(*f), "{}", line)
}
}
}
}
// Internal helper to wrap fmt_header into a fmt::Display
struct FmtHeader<'a, H: ?Sized + 'a>(&'a H);
impl<'a, H: Header + ?Sized + 'a> fmt::Display for FmtHeader<'a, H> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
self.0.fmt_header(f)
}
}
struct ValueString<'a>(&'a Item);
impl<'a> fmt::Display for ValueString<'a> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
self.0.write_h1(&mut MultilineFormatter(Multi::Join(true, f)))
}
}
struct NewlineReplacer<'a, 'b: 'a>(&'a mut fmt::Formatter<'b>);
impl<'a, 'b> fmt::Write for NewlineReplacer<'a, 'b> {
fn write_str(&mut self, s: &str) -> fmt::Result {
let mut since = 0;
for (i, &byte) in s.as_bytes().iter().enumerate() {
if byte == b'\r' || byte == b'\n' {
try!(self.0.write_str(&s[since..i]));
try!(self.0.write_str(" "));
since = i + 1;
}
}
if since < s.len() {
self.0.write_str(&s[since..])
} else {
Ok(())
}
}
}
#[doc(hidden)]
pub trait HeaderClone {
fn clone_box(&self) -> Box<Header + Send + Sync>;
@@ -415,7 +496,7 @@ impl PartialEq for Headers {
impl fmt::Display for Headers {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
for header in self.iter() {
try!(write!(f, "{}\r\n", header));
try!(fmt::Display::fmt(&header, f));
}
Ok(())
}
@@ -469,15 +550,20 @@ impl<'a> HeaderView<'a> {
}
/// Get just the header value as a String.
///
/// This will join multiple values of this header with a `, `.
///
/// **Warning:** This may not be the format that should be used to send
/// a Request or Response.
#[inline]
pub fn value_string(&self) -> String {
(*self.1).to_string()
ValueString(self.1).to_string()
}
}
impl<'a> fmt::Display for HeaderView<'a> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}: {}", self.0, *self.1)
self.1.write_h1(&mut MultilineFormatter(Multi::Line(self.0.as_ref(), f)))
}
}
@@ -572,7 +658,7 @@ mod tests {
#[cfg(feature = "nightly")]
use test::Bencher;
// Slice.position_elem is unstable
// Slice.position_elem was unstable
fn index_of(slice: &[u8], byte: u8) -> Option<usize> {
for (index, &b) in slice.iter().enumerate() {
if b == byte {
@@ -695,9 +781,10 @@ mod tests {
#[test]
fn test_headers_to_string_raw() {
let headers = Headers::from_raw(&raw!(b"Content-Length: 10")).unwrap();
let mut headers = Headers::from_raw(&raw!(b"Content-Length: 10")).unwrap();
headers.set_raw("x-foo", vec![b"foo".to_vec(), b"bar".to_vec()]);
let s = headers.to_string();
assert_eq!(s, "Content-Length: 10\r\n");
assert_eq!(s, "Content-Length: 10\r\nx-foo: foo\r\nx-foo: bar\r\n");
}
#[test]
@@ -758,6 +845,16 @@ mod tests {
}
}
#[test]
fn test_header_view_value_string() {
let mut headers = Headers::new();
headers.set_raw("foo", vec![b"one".to_vec(), b"two".to_vec()]);
for header in headers.iter() {
assert_eq!(header.name(), "foo");
assert_eq!(header.value_string(), "one, two");
}
}
#[test]
fn test_eq() {
let mut headers1 = Headers::new();