[tor-bugs] #5536 [EFF-HTTPS Everywhere]: Incorrect use of setResponseHeader for cookie

[Tor Bug Tracker & Wiki]

#5536: Incorrect use of setResponseHeader for cookie
----------------------------------+-----------------------------------------
 Reporter:  mkaply                |          Owner:  pde
     Type:  defect                |         Status:  new
 Priority:  normal                |      Milestone:     
Component:  EFF-HTTPS Everywhere  |        Version:     
 Keywords:                        |         Parent:     
   Points:                        |   Actualpoints:     
----------------------------------+-----------------------------------------

Comment(by ben):

 Here's my analysis:

 From what I understand, \n is invalid in HTTP headers (at least logically
 - newline and folding is the same as space).
 There can be 2 HTTP headers of the same name, but that is semantically
 identical to appending the second to the first with a ", " separator (and
 it's allowed only if the ", " separator is valid for that header).

 Our problem is that HTTPS Everywhere tries to set an additional second
 cookie for https, matching the http header. The way it does that leads to
 us trying to set a header with "\n", which fails.

 We do:

 var cookie = req1.getResponseHeader("Set-Cookie");
 req2.setRequestHeader("Cookie", cookie);


 HTTPS Everywhere does:

 req.setResponseHeader("Set-Cookie", c.source + ";Secure", true);

 I think it should instead do:

 req.setResponseHeader("Set-Cookie", oldvalue + ", " + c.source +
 ";Secure", false);


  Questions:

     Given that Firefox promises to merge headers, and the API doc is
 worded so that it's smart and looks at the header type, shouldn't Firefox
 merge Cookie headers using ", " instead of "\n"?
     Otherwise, should HTTPS Everywhere be fixed to do
 req.setResponseHeader("Set-Cookie", oldvalue + ", " + c.source +
 ";Secure", false);
     I would expect that req.setResponseHeader("foo",
 req.getResponseHeader("foo", cookie)); (ditto *Request*) is effectively a
 no-op, i.e. the result is the same as the original state. Currently, it
 can mess up the header by inserting "\n", because getResponseHeader() may
 return a "\n" that setResponseHeader() cannot deal with. I think that's
 wrong.


 References:


 RFC 2616 4.2
 > Multiple message-header fields with the same field-name MAY be present
 in a message if and only if the entire field-value for that header field
 is defined as a comma-separated list [i.e., #(values)]. It MUST be
 possible to combine the multiple header fields into one "field-name:
 field-value" pair, without changing the semantics of the message, by
 appending each subsequent field-value to the first, each separated by a
 comma.

 https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIHttpChannel#setRequestHeader%28%29
 >   aMerge If true, the new header value will be merged with any existing
 values for the specified header.
 https://developer.mozilla.org/en/DOM/XMLHttpRequest#setRequestHeader%28%29
 http://www.w3.org/TR/XMLHttpRequest/#the-setrequestheader-method
 >   If header is in the author request headers list either use multiple
 headers, combine the values or use a combination of those (section 4.2,
 RFC 2616). [HTTP]

 RFC 2109 4.2.2
 >   Set- Cookie:, followed by a comma-separated list of one or more
 cookies.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5536#comment:1>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online