1334776 - (CVE-2017-7797) Header name interning leaks across origins

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Anne (:annevk)]

Attachment: web-platform-tests test demonstrating the issue

Apparently the way we do header interning doesn't have any kind of boundary. We should probably limit it to each header list rather than keep a global registry.

I attached a web-platform-test patch demonstrating the issue.

Chrome and Safari do not have this problem as far as I can tell.

Comment 1

[Anne (:annevk)]

FWIW, I think this is problematic even just across requests/responses, but the origin leakage makes it a little scarier.

Comment 2

[Anne (:annevk)]

Potential attack: session supercookie.

Comment 3

[Daniel Veditz [:dveditz]]

Patrick: what are the implications here?

Comment 4

[Patrick McManus [:mcmanus]]

I believe dragana was the last one to touch the js facing header interfaces, so I'll ask her to weigh in here. :annevk it might be helpful if you could describe a scenario with a little more prose too so we're sure we know what you're talking about.

a few background things that may or may not be relevant:

header names are technically case insensitive.. h2 normalizes everything to lower case on the wire.. common headers have atoms defined and are in mixed case (no matter their wire representation).. and we've had folks in the distant past file bugs when getHeader gave them different case than their server produced (probably due to the atoms, tho I might be misremembering that)

Comment 5

[Anne (:annevk)]

This is not about JavaScript APIs (although they help expose it).

The problem is that for unknown header names we store the first one we see and then later we case-insensitively match against that name *globally*. That means you can track if a user agent has already seen a certain header name used (by using a different casing and observing whether it gets normalized). This would allow you to see if a user has used a sensitive service that uses custom header names, or allows you to track a user across sites, by teaching the browser about a certain header case once and then observing if different casings get normalized to that.

What we should do instead is only store the casing for a header name for each header list and not globally. That way it only leaks where it's expected (and necessary) to leak.

The test demonstrates the global leakage.

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: bug_1334776_v1.patch

I have tried to make it less invasive possible(this code is very old), so that we can maybe uplift it.

Comment 7

[Patrick McManus [:mcmanus]]

dragana can you provide a bugzilla comment about what the patch is meant to do?

Comment 8

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #7)
> dragana can you provide a bugzilla comment about what the patch is meant to
> do?

I was avoiding comments in the code.

nsHttpAtom now holds the old nsHttpAtom and a string that is case sensitive (only for not standard headers).

So nsHttpAtom holds a pointer to a header name. (header names are store on a static structure). This is how it used to be. I left that part the same but added a nsCString which holds a string that was used to resoled the header name. So when we parse headers we call ResolveHeader with a char*. If it is a new header name the char* will be stored in a HttpHeapAtom, nsHttpAtom::_val will point to HttpHeapAtom::value and the same strings will be stored in mLocalCaseSensitiveHeader. For the first resolve request they will be the same but for the following maybe not. At the end this nsHttpAtom will be stored in nsHttpHeaderArray. For all operation we will used the old char* except when we are returning it to a script using VisitHeaders.

I thought this is less invasive, I could supply the header name(with casing) differently but that would be a bigger change and also I was afraid to miss a spot.

I also split the header name hashTable because adding a flag to that old structure is much more work and I am afraid to uplift it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm not sure we should worry too much about uplifting this, or hiding what the new setup is, really.  Ideally the design of the new setup would just be in the commit message or so.

Comment 10

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> I'm not sure we should worry too much about uplifting this, or hiding what
> the new setup is, really.  Ideally the design of the new setup would just be
> in the commit message or so.

Thinking of it a bit longer, the approach I took it is good, I find it better that the one I mentioned in comment 8 (i.e. leaving nsHttpAtom in tact and adding string to the setHeader functions).
I could make a refinement and add the cased string only if it is needed. I will update the patch.

Comment 11

[Dragana Damjanovic [:dragana]]

Attachment: bug_1334776_v2.patch

This one adds a string to nsHttpHeaderArray::nsEntry that holds the original header name. It is set only if the original header name does not match the nsHttpAtom string (so it wikll be empty most of the time).
nsHttpHeaderArray::SetHeader, SetEmptyHeader, SetHeaderFromNet and SetResponseHeaderFromCache must be called with the original header name if needed(if not used from the internal code, e.g. SetHeader(nsHttp::Cookie...)

Comment 12

[Anne (:annevk)]

FWIW, to fully match what Chrome (and I think Edge) are doing would be to not even have interning for "known" header names. E.g., if you use setRequestHeader() or the Headers object API in Chrome, you can use `content-type` or `CONTENT-type` and get that passed-through to the server as-is.

We obviously don't need to do that to fix this issue, but it's something to consider going forward (especially with H2 making most of this obsolete anyway due to always using lowercase header names).

Comment 13

[Dragana Damjanovic [:dragana]]

(In reply to Anne (:annevk) from comment #12)
> FWIW, to fully match what Chrome (and I think Edge) are doing would be to
> not even have interning for "known" header names. E.g., if you use
> setRequestHeader() or the Headers object API in Chrome, you can use
> `content-type` or `CONTENT-type` and get that passed-through to the server
> as-is.
> 
> We obviously don't need to do that to fix this issue, but it's something to
> consider going forward (especially with H2 making most of this obsolete
> anyway due to always using lowercase header names).

The last patch will behave the same as Chrome.

Comment 14

[Anne (:annevk)]

Cool, here's another test I landed today for that (doesn't expose the security issue, just tests casing of headers): http://w3c-test.org/XMLHttpRequest/getallresponseheaders-cl.htm.

Comment 15

[Patrick McManus [:mcmanus]]

I'm going to suggest that we comment this, both in headerArray and the merge commit as "preserve http response header case per transaction"

Comment 16

[Patrick McManus [:mcmanus]]

Comment on attachment 8836114 [details] [diff] [review]
bug_1334776_v2.patch

Review of attachment 8836114 [details] [diff] [review]:
-----------------------------------------------------------------

I like the patch - unfortunately it crashes :(

Comment 17

[Patrick McManus [:mcmanus]]

when I run the testcase anne provided 
./mach web-platform-tests --debugger=gdb testing/web-platform/tests/XMLHttpRequest/cross-origin-header-interning.htm

also have to add it to testing/web-platform/meta/MANIFEST.json

I didnt' try and debug it at all..

[Child 12076] WARNING: '!compMgr', file /home/mcmanus/src/mozilla2/wd/gecko-dev/xpcom/components/nsComponentManagerUtils.cpp, line 63
[Child 12076] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /home/mcmanus/src/mozilla2/wd/gecko-dev/toolkit/xre/nsXREDirProvider.cpp, line 1699

Thread 1 "firefox" received signal SIGILL, Illegal instruction.
0x00007fffe3406770 in mozilla::net::nsHttpHeaderArray::SetHeader (this=0x7fffc3eb3160, headerName=..., value=..., merge=false, variety=mozilla::net::nsHttpHeaderArray::eVarietyRequestOverride)
    at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpHeaderArray.cpp:33
33	    SetHeader(header, headerName, value, merge, variety);
(gdb) bt
#0  0x00007fffe3406770 in mozilla::net::nsHttpHeaderArray::SetHeader (this=0x7fffc3eb3160, headerName=..., value=..., merge=false, variety=mozilla::net::nsHttpHeaderArray::eVarietyRequestOverride)
    at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpHeaderArray.cpp:33
#1  0x00007fffe3423199 in mozilla::net::nsHttpRequestHead::SetHeader (this=0x7fffc3eb3160, h=..., v=..., m=false) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpRequestHead.cpp:143
#2  0x00007fffe335d6d2 in mozilla::net::HttpBaseChannel::SetRequestHeader (this=0x7fffc3eb3000, aHeader=..., aValue=..., aMerge=false) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/HttpBaseChannel.cpp:1827
#3  0x00007fffe6bd5eb3 in mozilla::dom::RequestHeaders::ApplyToChannel (this=0x7fffd294ce78, aHttpChannel=0x7fffc3eb3058) at /home/mcmanus/src/mozilla2/wd/gecko-dev/dom/xhr/XMLHttpRequestMainThread.cpp:4121

Comment 18

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #17)
> when I run the testcase anne provided 
> ./mach web-platform-tests --debugger=gdb
> testing/web-platform/tests/XMLHttpRequest/cross-origin-header-interning.htm
> 
> also have to add it to testing/web-platform/meta/MANIFEST.json
> 
> I didnt' try and debug it at all..
> 
> [Child 12076] WARNING: '!compMgr', file
> /home/mcmanus/src/mozilla2/wd/gecko-dev/xpcom/components/
> nsComponentManagerUtils.cpp, line 63
> [Child 12076] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result
> 0x80004005: file
> /home/mcmanus/src/mozilla2/wd/gecko-dev/toolkit/xre/nsXREDirProvider.cpp,
> line 1699
> 
> Thread 1 "firefox" received signal SIGILL, Illegal instruction.
> 0x00007fffe3406770 in mozilla::net::nsHttpHeaderArray::SetHeader
> (this=0x7fffc3eb3160, headerName=..., value=..., merge=false,
> variety=mozilla::net::nsHttpHeaderArray::eVarietyRequestOverride)
>     at
> /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/
> nsHttpHeaderArray.cpp:33
> 33	    SetHeader(header, headerName, value, merge, variety);
> (gdb) bt
> #0  0x00007fffe3406770 in mozilla::net::nsHttpHeaderArray::SetHeader
> (this=0x7fffc3eb3160, headerName=..., value=..., merge=false,
> variety=mozilla::net::nsHttpHeaderArray::eVarietyRequestOverride)
>     at
> /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/
> nsHttpHeaderArray.cpp:33
> #1  0x00007fffe3423199 in mozilla::net::nsHttpRequestHead::SetHeader
> (this=0x7fffc3eb3160, h=..., v=..., m=false) at
> /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/
> nsHttpRequestHead.cpp:143
> #2  0x00007fffe335d6d2 in mozilla::net::HttpBaseChannel::SetRequestHeader
> (this=0x7fffc3eb3000, aHeader=..., aValue=..., aMerge=false) at
> /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/
> HttpBaseChannel.cpp:1827
> #3  0x00007fffe6bd5eb3 in mozilla::dom::RequestHeaders::ApplyToChannel
> (this=0x7fffd294ce78, aHttpChannel=0x7fffc3eb3058) at
> /home/mcmanus/src/mozilla2/wd/gecko-dev/dom/xhr/XMLHttpRequestMainThread.cpp:
> 4121

I have tested it with the test, interesting, I will see what is wrong.

Comment 19

[Dragana Damjanovic [:dragana]]

I have tried to reproduce the crash, I could not.. I tried many times...

Can you reproduce it?

Comment 20

[Patrick McManus [:mcmanus]]

I just did a clean rebuild. I can't even start the browser with this patch :)

I'll try and investigate more..

Comment 21

[Patrick McManus [:mcmanus]]

this seems to be compiler specific.. this is what valgrind has to say about clang version 3.8.1-12ubuntu1 (tags/RELEASE_381/final)

gcc (Ubuntu 6.2.0-5ubuntu12) 6.2.0 20161005 WFM

going to check on clang 3.7


=30180== valgrind: Unrecognised instruction at address 0x115be7b0.
==30180==    at 0x115BE7B0: mozilla::net::nsHttpHeaderArray::SetHeader(nsACString_internal const&, nsACString_internal const&, bool, mozilla::net::nsHttpHeaderArray::HeaderVariety) (nsHttpHeaderArray.cpp:33)
==30180==    by 0x115DB1B8: mozilla::net::nsHttpRequestHead::SetHeader(nsACString_internal const&, nsACString_internal const&, bool) (nsHttpRequestHead.cpp:143)
==30180==    by 0x11515741: mozilla::net::HttpBaseChannel::SetRequestHeader(nsACString_internal const&, nsACString_internal const&, bool) (HttpBaseChannel.cpp:1827)
==30180==    by 0x14D9AB52: mozilla::dom::RequestHeaders::ApplyToChannel(nsIHttpChannel*) const (XMLHttpRequestMainThread.cpp:4117)
==30180==    by 0x14D99D04: mozilla::dom::XMLHttpRequestMainThread::InitiateFetch(nsIInputStream*, long, nsACString_internal&) (XMLHttpRequestMainThread.cpp:2568)
==30180==    by 0x14D9BE51: mozilla::dom::XMLHttpRequestMainThread::SendInternal(mozilla::dom::BodyExtractorBase const*) (XMLHttpRequestMainThread.cpp:2940)
==30180==    by 0x14DB266B: mozilla::dom::XMLHttpRequestMainThread::Send(JSContext*, mozilla::ErrorResult&) (XMLHttpRequestMainThread.h:313)
==30180==    by 0x13739C52: mozilla::dom::XMLHttpRequestBinding::send(JSContext*, JS::Handle<JSObject*>, mozilla::dom::XMLHttpRequest*, JSJitMethodCallArgs const&) (XMLHttpRequestBinding.cpp:651)
==30180==    by 0x13B85664: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (BindingUtils.cpp:2953)
==30180==    by 0x1745A8A3: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:282)
==30180==    by 0x1744B09E: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:448)
==30180==    by 0x1744B49C: InternalCall(JSContext*, js::AnyInvokeArgs const&) (Interpreter.cpp:493)

Comment 22

[Patrick McManus [:mcmanus]]

this also fails the "casing of known headers 2" test from http://w3c-test.org/XMLHttpRequest/getallresponseheaders-cl.htm that anne mentioned. (I didn't investigate it)

Comment 23

[Patrick McManus [:mcmanus]]

tree fails to compile (unrelated to this patch) with
Ubuntu clang version 3.7.1-3ubuntu4 (tags/RELEASE_371/final) (based on LLVM 3.7.1)
Target: x86_64-pc-linux-gnu
Thread model: posix

That's fine - just not useful as a datapoint.

Comment 24

[Anne (:annevk)]

I think this is also causing bug 1348390. Which would be http://w3c-test.org/XMLHttpRequest/getallresponseheaders.htm.

It's probably okay to disable that test, but once we land this fix we should enable it again.

Comment 25

[Dragana Damjanovic [:dragana]]

I tried to reproduce the crash, but no luck. Do you have time to investigate a bit? I looked at the code but I do not see anything suspicious.

Comment 26

[Patrick McManus [:mcmanus]]

Attachment: 0001-wip.patch

* bitrot fixed
* fixed a return value of SetHeader() (that didn't return a value but was defined as nsresult)

Comment 27

[Patrick McManus [:mcmanus]]

good news - no more sigILL. of course I might have screwed up the rebasing.

http://w3c-test.org/XMLHttpRequest/getallresponseheaders-cl.htm runs but fails

while
http://w3c-test.org/XMLHttpRequest/getallresponseheaders.htm
crashes on what looks like uaf

0x00007fffe24a2997 in std::__atomic_base<int>::fetch_add (this=0xe5e5e5e5e5e5e5dd, __i=1, __m=std::memory_order_seq_cst) at /usr/bin/../lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/bits/atomic_base.h:514
514	      { return __atomic_fetch_add(&_M_i, __i, __m); }
(gdb) bt
#0  0x00007fffe24a2997 in std::__atomic_base<int>::fetch_add (this=0xe5e5e5e5e5e5e5dd, __i=1, __m=std::memory_order_seq_cst) at /usr/bin/../lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/bits/atomic_base.h:514
#1  0x00007fffe24a28f0 in mozilla::detail::IntrinsicAddSub<int, (mozilla::MemoryOrdering)2>::add (aPtr=..., aVal=1) at ../../dist/include/mozilla/Atomics.h:254
#2  0x00007fffe24a28ca in mozilla::detail::IntrinsicIncDec<int, (mozilla::MemoryOrdering)2>::inc (aPtr=...) at ../../dist/include/mozilla/Atomics.h:286
#3  0x00007fffe249d865 in mozilla::detail::AtomicBaseIncDec<int, (mozilla::MemoryOrdering)2>::operator++ (this=0xe5e5e5e5e5e5e5dd) at ../../dist/include/mozilla/Atomics.h:609
#4  0x00007fffe248f3bf in nsStringBuffer::AddRef (this=0xe5e5e5e5e5e5e5dd) at /home/mcmanus/src/mozilla2/wd/gecko-dev/xpcom/string/nsSubstring.cpp:189
#5  0x00007fffe2492b04 in nsACString::Assign (this=0x7fffb81d59c0, aStr=..., aFallible=...) at ../../../gecko-dev/xpcom/string/nsTSubstring.cpp:452
#6  0x00007fffe24867a6 in nsACString::Assign (this=0x7fffb81d59c0, aStr=...) at ../../../gecko-dev/xpcom/string/nsTSubstring.cpp:418
#7  0x00007fffe249d107 in nsCString::operator= (this=0x7fffb81d59c0, aStr=...) at ../../../gecko-dev/xpcom/string/nsTString.h:98
#8  0x00007fffe2cbcd99 in mozilla::net::nsHttpHeaderArray::SetHeader_internal (this=0x7fffb81d5700, header=..., headerName=..., value=..., variety=mozilla::net::nsHttpHeaderArray::eVarietyResponse)
    at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpHeaderArray.cpp:111
#9  0x00007fffe2ccf9dd in mozilla::net::nsHttpHeaderArray::MergeHeader (this=0x7fffb81d5700, header=..., entry=0x7fffb81d6dc8, value=..., variety=mozilla::net::nsHttpHeaderArray::eVarietyResponse)
    at ../../../../gecko-dev/netwerk/protocol/http/nsHttpHeaderArray.h:277
#10 0x00007fffe2cbd1f1 in mozilla::net::nsHttpHeaderArray::SetHeaderFromNet (this=0x7fffb81d5700, header=..., headerNameOrg=..., value=..., response=true) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpHeaderArray.cpp:183
#11 0x00007fffe2cc28b1 in mozilla::net::nsHttpResponseHead::ParseHeaderLine_locked (this=0x7fffb81d5700, line=..., originalFromNetHeaders=true) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpResponseHead.cpp:605
#12 0x00007fffe2cc3b88 in mozilla::net::nsHttpResponseHead::ParseHeaderLine (this=0x7fffb81d5700, line=...) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpResponseHead.cpp:590
#13 0x00007fffe2ccc292 in mozilla::net::nsHttpTransaction::ParseLine (this=0x7fffbd4b6000, line=...) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpTransaction.cpp:1287

Comment 28

[Dragana Damjanovic [:dragana]]

I think I have a fix for the crash - assigning a substring to a nsAutoCString make a shared substring and I did not want that.

I have looked into tests:
http://w3c-test.org/XMLHttpRequest/getallresponseheaders-cl.htm

there is 3 tests:
The first fails because it expects content-length: 0\r\n" but we got "CONTENT-LENGTH: 0\r\n". Anne do you want us to make non-standard casing all small? I think we should not. Chrome fails in the same way as firefox with my patch.
The second passes (this one uses XHR).
The third fails. It uses fetch.:
the test sets 2 headers "THIS-is-A-test: 1" and "THIS-IS-A-TEST: 2". The fetch code transform it into all small case header names. Chrome does the same. The fetch code uses SetRequestHeader with parameter merge=false and therefore we end up having only "this-is-a-test: 2". Chrome has "this-is-a-test: 1,2". I do not know why we use merge=false.


http://w3c-test.org/XMLHttpRequest/getallresponseheaders.htm
The second test fails for 2 reasons: the first reason is the same as the first test in http://w3c-test.org/XMLHttpRequest/getallresponseheaders-cl.htm. The second reason is the order of headers. Chrome and Firefox does not order headers in the same way and the test does not match neither Firefox nor Chrome.

Comment 29

[Anne (:annevk)]

For http://w3c-test.org/XMLHttpRequest/getallresponseheaders-cl.htm:

1) The lowercasing should probably not happen in Necko. If we do this it should be in our XMLHttpRequest code and done separately from this bug.
2) Great!
3) That seems like a bug, but again, not in Necko.

For 
http://w3c-test.org/XMLHttpRequest/getallresponseheaders.htm:

2) This again seems like something to be fixed outside of Necko.

Comment 30

[Dragana Damjanovic [:dragana]]

Attachment: bug_1334776_v2.patch

Comment 31

[Patrick McManus [:mcmanus]]

unfortunately http://w3c-test.org/XMLHttpRequest/getallresponseheaders.htm still crashes for me

does the safety of the assign() in ParseheaderLine depend on the actual type of the nsACstring? i.e. is a substring passed in there always dangerous? maybe the type needs to be changed.. just brainstorming - not sure.

Thread 9 "Socket Thread" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffda1da700 (LWP 6215)]
0x00007fffe22ecefb in std::__atomic_base<unsigned int>::fetch_add (this=0xe5e5e5e5e5e5e5dd, __i=1, __m=std::memory_order_relaxed) at /usr/bin/../lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/bits/atomic_base.h:514
514	      { return __atomic_fetch_add(&_M_i, __i, __m); }
(gdb) bt
#0  0x00007fffe22ecefb in std::__atomic_base<unsigned int>::fetch_add (this=0xe5e5e5e5e5e5e5dd, __i=1, __m=std::memory_order_relaxed) at /usr/bin/../lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/bits/atomic_base.h:514
#1  0x00007fffe22dea16 in nsStringBuffer::AddRef (this=0xe5e5e5e5e5e5e5dd) at /home/mcmanus/src/mozilla2/wd/gecko-dev/xpcom/string/nsSubstring.cpp:200
#2  0x00007fffe22e2144 in nsACString::Assign (this=0x7fffbdc79ec0, aStr=..., aFallible=...) at ../../../gecko-dev/xpcom/string/nsTSubstring.cpp:452
#3  0x00007fffe22d5dd6 in nsACString::Assign (this=0x7fffbdc79ec0, aStr=...) at ../../../gecko-dev/xpcom/string/nsTSubstring.cpp:418
#4  0x00007fffe22ec767 in nsCString::operator= (this=0x7fffbdc79ec0, aStr=...) at ../../../gecko-dev/xpcom/string/nsTString.h:98
#5  0x00007fffe2b1a239 in mozilla::net::nsHttpHeaderArray::SetHeader_internal (this=0x7fffbdc79e00, header=..., headerName=..., value=..., variety=mozilla::net::nsHttpHeaderArray::eVarietyResponse)
    at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpHeaderArray.cpp:111
#6  0x00007fffe2b2cead in mozilla::net::nsHttpHeaderArray::MergeHeader (this=0x7fffbdc79e00, header=..., entry=0x7fffbdc8a748, value=..., variety=mozilla::net::nsHttpHeaderArray::eVarietyResponse)
    at ../../../../gecko-dev/netwerk/protocol/http/nsHttpHeaderArray.h:279
#7  0x00007fffe2b1a691 in mozilla::net::nsHttpHeaderArray::SetHeaderFromNet (this=0x7fffbdc79e00, header=..., headerNameOrg=..., value=..., response=true) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpHeaderArray.cpp:184
#8  0x00007fffe2b1fd81 in mozilla::net::nsHttpResponseHead::ParseHeaderLine_locked (this=0x7fffbdc79e00, line=..., originalFromNetHeaders=true) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpResponseHead.cpp:605
#9  0x00007fffe2b21058 in mozilla::net::nsHttpResponseHead::ParseHeaderLine (this=0x7fffbdc79e00, line=...) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpResponseHead.cpp:590
#10 0x00007fffe2b29762 in mozilla::net::nsHttpTransaction::ParseLine (this=0x7fffbdcf4400, line=...) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpTransaction.cpp:1287
#11 0x00007fffe2b29879 in mozilla::net::nsHttpTransaction::ParseLineSegment (this=0x7fffbdcf4400, segment=0x7fffbdca002b "foo-test: 3\n", '\344' <repeats 187 times>, <incomplete sequence \344>..., len=12)
    at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpTransaction.cpp:1304
#12 0x00007fffe2b28ed8 in mozilla::net::nsHttpTransaction::ParseHead (this=0x7fffbdcf4400, buf=0x7fffbdca002b "foo-test: 3\n", '\344' <repeats 187 times>, <incomplete sequence \344>..., count=55, countRead=0x7fffda1d93d4)
    at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpTransaction.cpp:1431
#13 0x00007fffe2b26c3a in mozilla::net::nsHttpTransaction::ProcessData (this=0x7fffbdcf4400, buf=0x7fffbdca0000 "HTTP/1.1 280 HELLO\nfoo-test: 1\nfoo-test: 2\nfoo-test: 3\n", '\344' <repeats 144 times>, <incomplete sequence \344>..., count=55, 
    countRead=0x7fffda1d94b0) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpTransaction.cpp:1690
#14 0x00007fffe2b26a4d in mozilla::net::nsHttpTransaction::WritePipeSegment (stream=0x7fffc88f3958, closure=0x7fffbdcf4400, 
    buf=0x7fffbdca0000 "HTTP/1.1 280 HELLO\nfoo-test: 1\nfoo-test: 2\nfoo-test: 3\n", '\344' <repeats 144 times>, <incomplete sequence \344>..., offset=0, count=32768, countWritten=0x7fffda1d94b0)
    at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpTransaction.cpp:823
#15 0x00007fffe23cd1fc in nsPipeOutputStream::WriteSegments (this=0x7fffc88f3958, aReader=0x7fffe2b26840 <mozilla::net::nsHttpTransaction::WritePipeSegment(nsIOutputStream*, void*, char*, unsigned int, unsigned int, unsigned int*)>, aClosure=0x7fffbdcf4400, 
    aCount=32768, aWriteCount=0x7fffda1d9624) at /home/mcmanus/src/mozilla2/wd/gecko-dev/xpcom/io/nsPipe3.cpp:1817
#16 0x00007fffe2b2824e in mozilla::net::nsHttpTransaction::WriteSegments (this=0x7fffbdcf4400, writer=0x7fffc718fe28, count=32768, countWritten=0x7fffda1d9624) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpTransaction.cpp:859
#17 0x00007fffe2a7b6bd in mozilla::net::nsAHttpTransaction::WriteSegmentsAgain (this=0x7fffbdcf4400, writer=0x7fffc718fe28, count=32768, countWritten=0x7fffda1d9624, again=0x7fffda1d9623) at ../../../../gecko-dev/netwerk/protocol/http/nsAHttpTransaction.h:93
#18 0x00007fffe2ae569d in mozilla::net::nsHttpConnection::OnSocketReadable (this=0x7fffc718fe20) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpConnection.cpp:1812
#19 0x00007fffe2ae62fb in mozilla::net::nsHttpConnection::OnInputStreamReady (this=0x7fffc718fe20, in=0x7fffbded3e80) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/protocol/http/nsHttpConnection.cpp:2148
#20 0x00007fffe25befb1 in mozilla::net::nsSocketInputStream::OnSocketReady (this=0x7fffbded3e80, condition=nsresult::NS_OK) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/base/nsSocketTransport2.cpp:293
#21 0x00007fffe25c662d in mozilla::net::nsSocketTransport::OnSocketReady (this=0x7fffbded3c00, fd=0x7fffddebff70, outFlags=1) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/base/nsSocketTransport2.cpp:1996
#22 0x00007fffe25cf5e7 in mozilla::net::nsSocketTransportService::DoPollIteration (this=0x7fffdfa3d660, pollDuration=0x7fffda1d99b8) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/base/nsSocketTransportService2.cpp:1135
#23 0x00007fffe25ce98b in mozilla::net::nsSocketTransportService::Run (this=0x7fffdfa3d660) at /home/mcmanus/src/mozilla2/wd/gecko-dev/netwerk/base/nsSocketTransportService2.cpp:909
#24 0x00007fffe2433abe in nsThread::ProcessNextEvent (this=0x7ffff6bbe600, aMayWait=false, aResult=0x7fffda1d9bfe) at /home/mcmanus/src/mozilla2/wd/gecko-dev/xpcom/threads/nsThread.cpp:1269
#25 0x00007fffe2431c4c in NS_ProcessNextEvent (aThread=0x7ffff6bbe600, aMayWait=false) at /home/mcmanus/src/mozilla2/wd/gecko-dev/xpcom/threads/nsThreadUtils.cpp:389
#26 0x00007fffe2d42330 in mozilla::ipc::MessagePumpForNonMainThreads::Run (this=0x7fffddd10d00, aDelegate=0x7ffff6bb85c0) at /home/mcmanus/src/mozilla2/wd/gecko-dev/ipc/glue/MessagePump.cpp:338
#27 0x00007fffe2c831f5 in MessageLoop::RunInternal (this=0x7ffff6bb85c0) at /home/mcmanus/src/mozilla2/wd/gecko-dev/ipc/chromium/src/base/message_loop.cc:238
#28 0x00007fffe2c83175 in MessageLoop::RunHandler (this=0x7ffff6bb85c0) at /home/mcmanus/src/mozilla2/wd/gecko-dev/ipc/chromium/src/base/message_loop.cc:231
#29 0x00007fffe2c8314d in MessageLoop::Run (this=0x7ffff6bb85c0) at /home/mcmanus/src/mozilla2/wd/gecko-dev/ipc/chromium/src/base/message_loop.cc:211
#30 0x00007fffe24304d7 in nsThread::ThreadFunc (aArg=0x7fffffff9e10) at /home/mcmanus/src/mozilla2/wd/gecko-dev/xpcom/threads/nsThread.cpp:500
#31 0x00007ffff7f04335 in _pt_root (arg=0x7ffff6ba4340) at ../../../../../gecko-dev/nsprpub/pr/src/pthreads/ptthread.c:216
#32 0x00007ffff7bc16ca in start_thread (arg=0x7fffda1da700) at pthread_create.c:333
#33 0x00007ffff6e4f0af in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:105

Comment 32

[Patrick McManus [:mcmanus]]

Comment on attachment 8856494 [details] [diff] [review]
bug_1334776_v2.patch

Review of attachment 8856494 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpHeaderArray.cpp
@@ +415,5 @@
>  
>      // assign return values
>      if (hdr) *hdr = atom;
>      if (val) val->Assign(p, p2 - p + 1);
> +    if (headerName) headerName->Assign(sub.BeginReading(), sub.Length());

might as well put a comment on the subtle part here, given that we both missed it the first time around.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> assigning a substring to a nsAutoCString make a shared substring and I did not want that.

Uh... what do you mean?  Assigning to an nsAutoCString should generally give it its own stringbuffer, copying as needed.

Looking at the stack in comment 31, we have a freed stringbuffer pointer:

    nsStringBuffer::AddRef (this=0xe5e5e5e5e5e5e5dd)

Which string is that?  Is that the entry->headerNameOrg string or something else?

Comment 34

[Dragana Damjanovic [:dragana]]

This is all my mistake. Sorry for the noise.

103         nsEntry *entry =  mHeaders.AppendElement();
104	    if (!entry) {
105	        return NS_ERROR_OUT_OF_MEMORY;
106	    }
107	    entry->header = header;
108	    // Only same original form of a header if it is different than the header
109	    // string.
110	    if (!headerName.Equals(header.get())) {
111	        entry->headerNameOrg = headerName;

entry->headerNameOrg get the value of headerName. mData of entry->headerNameOrg string is assigned mData of headerName. This mData is freed. This is because headerName is entry->headerNameOrg, here entry is another entry (which is probably empty) and both are part of the same nsTArray<nsEntry> so probably after the new one is created the old one is reallocated. Does this make sense ?

Comment 35

[Dragana Damjanovic [:dragana]]

Attachment: bug_1334776_v2.patch

Comment 36

[Patrick McManus [:mcmanus]]

I'm sorry; I don't understand what the problem was (i.e. how we ended up in uaf)

specifically there are lots of ways to call setheader_internal, why does copying in that one case make a difference. why does the header value (instead of name) never need to be copied?

Is it somehow a re-entrant issue where entry->name was overwritten (and then deleted) on the stack and the caller is unaware?

Comment 37

[Patrick McManus [:mcmanus]]

Comment on attachment 8856573 [details] [diff] [review]
bug_1334776_v2.patch

Review of attachment 8856573 [details] [diff] [review]:
-----------------------------------------------------------------

I will admit I don't totally understand what the problem is here.

::: netwerk/protocol/http/nsHttpHeaderArray.cpp
@@ +104,5 @@
>      if (!entry) {
>          return NS_ERROR_OUT_OF_MEMORY;
>      }
>      entry->header = header;
> +    // Only same original form of a header if it is different than the header

s/same/save

@@ +105,5 @@
>          return NS_ERROR_OUT_OF_MEMORY;
>      }
>      entry->header = header;
> +    // Only same original form of a header if it is different than the header
> +    // string.

atom string

Comment 38

[Patrick McManus [:mcmanus]]

(In reply to Patrick McManus [:mcmanus] from comment #36)
> I'm sorry; I don't understand what the problem was (i.e. how we ended up in
> uaf)
> 
> specifically there are lots of ways to call setheader_internal, why does
> copying in that one case make a difference. why does the header value
> (instead of name) never need to be copied?

dragana and I spoke out of band. the answer is that mergeHeader is special - the values if they are merged are rewritten with a comma (so the old string can be safely destroyed) and previously the name was always based on the persistent atom (not anymore - thus the problem).

Comment 39

[Patrick McManus [:mcmanus]]

Comment on attachment 8856573 [details] [diff] [review]
bug_1334776_v2.patch

Review of attachment 8856573 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpHeaderArray.h
@@ +127,5 @@
>      // Must be copy-constructable and assignable
>      struct nsEntry
>      {
>          nsHttpAtom header;
> +        nsCString headerNameOrg;

let's rename all instances of headerNameOrg to headerNameOriginal (or perhaps headerNameOrig)

I know its a pain. thanks!

Comment 40

[Dragana Damjanovic [:dragana]]

Attachment: bug_1334776_v2.patch

Comment 41

[Dragana Damjanovic [:dragana]]

Attachment: bug_1334776_v2.patch

Fix some tests.

Comment 42

[Dragana Damjanovic [:dragana]]

Attachment: bug_1334776_devtools.patch

This is a patch for devtools. With this patch all test are passing but I am afraid that there is some code that the tests do not cover.

Honza, I do not know who is the right person to review this.

Comment 43

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e6631888c7b7809e9a69bb0f04193a541945928

Comment 44

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8858309 [details] [diff] [review]
bug_1334776_devtools.patch

Review of attachment 8858309 [details] [diff] [review]:
-----------------------------------------------------------------

If I understand correctly, we should treat all headers case-insensitive => lower case, correct?

If yes, the patch looks good to me.

R+ assuming try is green.

Honza

Comment 45

[Dragana Damjanovic [:dragana]]

Attachment: bug_1334776_v2.patch

rebased

Comment 46

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf7aacaca2319d80863613729b569518aa810dea
at this one there is an error in web-platform-test, but that is my mistake. I marked that the first test in getallresponseheaders-cl.htm pass but it does not, this is explained in comment 28. I fixed it:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8d136b54bfdd8b44727848d0f83e9240e6ca41d

Comment 47

[Dragana Damjanovic [:dragana]]

(In reply to Jan Honza Odvarko [:Honza] from comment #44)
> Comment on attachment 8858309 [details] [diff] [review]
> bug_1334776_devtools.patch
> 
> Review of attachment 8858309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If I understand correctly, we should treat all headers case-insensitive =>
> lower case, correct?
> 
> If yes, the patch looks good to me.
> 
> R+ assuming try is green.
> 
> Honza

A header name will have the same form as when it is received from a server or set using setHeader. So it is easiest to treat is case-insensitive and transform i to lower case.

Comment 48

[Dragana Damjanovic [:dragana]]

Comment on attachment 8861308 [details] [diff] [review]
bug_1334776_v2.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? The patch shows what the problem is. How to exploit it described in description ,comment 1 and 2

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? yes

Which older supported branches are affected by this flaw? all, this is handle in the same way from the beginning.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I am afraid that backports are risky, I would like this parch to run a bit longer before release. We now return case-sensitive header names and I am afraid some application will break because they are expecting something different.

How likely is this patch to cause regressions; how much testing does it need?
It is possible that this will cause problems but they are hard to test, we will maybe break some sites.

Comment 49

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8861308 [details] [diff] [review]
bug_1334776_v2.patch

As a sec-low rated issue, this doesn't need sec-approval to be checked in. I'm clearing the flag.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Go ahead and check into trunk.

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9bf8e9f799c3
https://hg.mozilla.org/mozilla-central/rev/32aa533e3335

Comment 51

[Ryan VanderMeulen [:RyanVM]]

Sounds like this is a wontfix for 54 then based on comment 48. Given that this is a fingerprinting issue, I'm leaving esr52 set to affected for possible backport there in a future release (maybe 52.3 or 52.4?).

Comment 52

[Al Billings [:abillings - ex-MoCo]]

As a sec-low, we wouldn't normally take this on ESR52 without a reason.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Arthur, is this something that would be useful to backport to ESR52 for Tor?

Comment 54

[Arthur Edelstein [:arthur]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #53)
> Arthur, is this something that would be useful to backport to ESR52 for Tor?

Yes, I think it would be. Thanks for bringing it to our attention.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Thanks for the reply, Arthur. Dragana, can you please nominate this for ESR52 approval? Thanks :)

Comment 56

[Dragana Damjanovic [:dragana]]

This patch changes some header casing. I am afraid that some application will depend on old casing. This patch is on beta and nightly, it is still not on release. Pushing this to esr52 is connected with some risk. I will double check with Patrick.

Comment 57

[Patrick McManus [:mcmanus]]

I don't think "useful" is the right metric for backporting. Hopefully all patches are useful to someone :)

the risk here is in compat.. it doesn't seem to meet esr criteria and hasn't seen the broadest release audience yet.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

Sounds like the earliest we'd want to consider this then would be for 52.4, i.e. the next cycle. Which would already be halfway through ESR52's lifespan. Let's just wontfix unless people strongly want it (and I assume the Tor folks can just include it locally in their builds).