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

@@ -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");
+}

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<H: Header + HeaderFormat>(raw: &Vec<Vec<u8>>) ->
-        ::Result<Box<HeaderFormat + Send + Sync>> {
-    Header::parse_header(&raw[..]).map(|h: H| {
-        // FIXME: Use Type ascription
-        let h: Box<HeaderFormat + Send + Sync> = 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<H: Header + HeaderFormat>(raw: &Vec<Vec<u8>>) ->
+        ::Result<Box<HeaderFormat + Send + Sync>> {
+    Header::parse_header(&raw[..]).map(|h: H| {
+        // FIXME: Use Type ascription
+        let h: Box<HeaderFormat + Send + Sync> = Box::new(h);
+        h
+    })
+}
+

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<usize> {
         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();