From 2603d78f59d284953553b7ef48c3ea4baa085cd1 Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Thu, 19 Jan 2017 12:53:08 -0800 Subject: [PATCH] fix(header): security fix for header values that include newlines 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. --- src/header/common/set_cookie.rs | 25 +++++-- src/header/internals/item.rs | 41 ++++++------ src/header/mod.rs | 113 +++++++++++++++++++++++++++++--- 3 files changed, 145 insertions(+), 34 deletions(-) diff --git a/src/header/common/set_cookie.rs b/src/header/common/set_cookie.rs index 7f16e0ff71..6d26fcd962 100644 --- a/src/header/common/set_cookie.rs +++ b/src/header/common/set_cookie.rs @@ -1,5 +1,5 @@ use header::{Header, HeaderFormat}; -use std::fmt::{self, Display}; +use std::fmt::{self}; use std::str::from_utf8; @@ -93,15 +93,26 @@ impl Header for SetCookie { } impl HeaderFormat for SetCookie { + 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 { + println!("setcookie fmt_multi_header"); + 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"); +} diff --git a/src/header/internals/item.rs b/src/header/internals/item.rs index 60c6972325..c6baab6cf6 100644 --- a/src/header/internals/item.rs +++ b/src/header/internals/item.rs @@ -4,7 +4,7 @@ use std::fmt; use std::str::from_utf8; use super::cell::{OptCell, PtrMapCell}; -use header::{Header, HeaderFormat}; +use header::{Header, HeaderFormat, MultilineFormatter}; #[derive(Clone)] @@ -83,35 +83,38 @@ impl Item { } self.typed.get_mut(tid).map(|typed| unsafe { typed.downcast_mut_unchecked() }) } -} - -#[inline] -fn parse(raw: &Vec>) -> - ::Result> { - Header::parse_header(&raw[..]).map(|h: H| { - // FIXME: Use Type ascription - let h: Box = Box::new(h); - h - }) -} -impl fmt::Display for Item { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + 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.write_str(s)), - Err(e) => { - error!("raw header value is not utf8. header={:?}, error={:?}", - part, e); + Ok(s) => { + try!(f.fmt_line(&s)); + }, + Err(_) => { + error!("raw header value is not utf8, value={:?}", part); return Err(fmt::Error); } } } Ok(()) }, - None => fmt::Display::fmt(&unsafe { self.typed.one() }, f) + None => { + let typed = unsafe { self.typed.one() }; + typed.fmt_multi_header(f) + } } } } + +#[inline] +fn parse(raw: &Vec>) -> + ::Result> { + Header::parse_header(&raw[..]).map(|h: H| { + // FIXME: Use Type ascription + let h: Box = Box::new(h); + h + }) +} + diff --git a/src/header/mod.rs b/src/header/mod.rs index 89d7fc0eac..702629a1ed 100644 --- a/src/header/mod.rs +++ b/src/header/mod.rs @@ -132,6 +132,87 @@ pub trait HeaderFormat: fmt::Debug + HeaderClone + Any + Typeable + Send + Sync /// 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)] +#[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: HeaderFormat + ?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)] @@ -322,7 +403,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(()) } @@ -375,15 +456,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, f))) } } @@ -486,7 +572,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 { for (index, &b) in slice.iter().enumerate() { if b == byte { @@ -604,7 +690,7 @@ mod tests { } #[test] - fn test_headers_show() { + fn test_headers_fmt() { let mut headers = Headers::new(); headers.set(ContentLength(15)); headers.set(Host { hostname: "foo.bar".to_owned(), port: None }); @@ -615,10 +701,11 @@ mod tests { } #[test] - fn test_headers_show_raw() { - let headers = Headers::from_raw(&raw!(b"Content-Length: 10")).unwrap(); + fn test_headers_fmt_raw() { + 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] @@ -672,6 +759,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();