Bug 1334776 (CVE-2017-7797)

Header name interning leaks across origins

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: annevk, Assigned: dragana)

Tracking

({csectype-disclosure, privacy, sec-low})

unspecified
mozilla55
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [necko-active][fingerprinting][adv-main55+][post-critsmash-triage])

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
FWIW, I think this is problematic even just across requests/responses, but the origin leakage makes it a little scarier.
(Reporter)

Comment 2

2 years ago
Potential attack: session supercookie.
Group: core-security → network-core-security
Attachment #8831420 - Attachment is patch: true
Patrick: what are the implications here?
Flags: needinfo?(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)
Flags: needinfo?(mcmanus) → needinfo?(dd.mozilla)
(Reporter)

Comment 5

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(dd.mozilla)
(Assignee)

Updated

2 years ago
Whiteboard: [necko-active]
(Assignee)

Comment 6

2 years ago
Posted patch bug_1334776_v1.patch (obsolete) — Splinter Review
I have tried to make it less invasive possible(this code is very old), so that we can maybe uplift it.
Attachment #8835557 - Flags: review?(mcmanus)
dragana can you provide a bugzilla comment about what the patch is meant to do?
(Assignee)

Comment 8

2 years ago
(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.
Whiteboard: [necko-active] → [necko-active][fingerprinting]
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.
(Assignee)

Comment 10

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8835557 - Flags: review?(mcmanus)
(Assignee)

Comment 11

2 years ago
Posted patch bug_1334776_v2.patch (obsolete) — Splinter Review
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...)
Attachment #8835557 - Attachment is obsolete: true
Attachment #8836114 - Flags: review?(mcmanus)
(Reporter)

Comment 12

2 years ago
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).
(Assignee)

Comment 13

2 years ago
(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.
(Reporter)

Comment 14

2 years ago
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.
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 on attachment 8836114 [details] [diff] [review]
bug_1334776_v2.patch

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

I like the patch - unfortunately it crashes :(
Attachment #8836114 - Flags: review?(mcmanus) → review-
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
(Assignee)

Comment 18

2 years ago
(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.
(Assignee)

Comment 19

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

Can you reproduce it?
(Assignee)

Updated

2 years ago
Flags: needinfo?(mcmanus)
I just did a clean rebuild. I can't even start the browser with this patch :)

I'll try and investigate more..
Flags: needinfo?(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)
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)
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.
(Reporter)

Comment 24

2 years ago
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.
(Assignee)

Comment 25

2 years ago
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.
Posted patch 0001-wip.patch (obsolete) — Splinter Review
* bitrot fixed
* fixed a return value of SetHeader() (that didn't return a value but was defined as nsresult)
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
(Assignee)

Comment 28

2 years ago
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.
(Reporter)

Comment 29

2 years ago
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.
(Assignee)

Comment 30

2 years ago
Posted patch bug_1334776_v2.patch (obsolete) — Splinter Review
Attachment #8836114 - Attachment is obsolete: true
Attachment #8854485 - Attachment is obsolete: true
Attachment #8856494 - Flags: review?(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 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.
Attachment #8856494 - Flags: review?(mcmanus) → review-
> 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?
(Assignee)

Comment 34

2 years ago
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 ?
(Assignee)

Comment 35

2 years ago
Posted patch bug_1334776_v2.patch (obsolete) — Splinter Review
Attachment #8856494 - Attachment is obsolete: true
Attachment #8856573 - Flags: review?(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 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
Attachment #8856573 - Flags: review?(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 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!
Attachment #8856573 - Flags: review+
(Assignee)

Comment 40

2 years ago
Posted patch bug_1334776_v2.patch (obsolete) — Splinter Review
Attachment #8856573 - Attachment is obsolete: true
Attachment #8857011 - Flags: review+
(Assignee)

Comment 41

2 years ago
Posted patch bug_1334776_v2.patch (obsolete) — Splinter Review
Fix some tests.
Attachment #8857011 - Attachment is obsolete: true
Attachment #8858298 - Flags: review+
(Assignee)

Comment 42

2 years ago
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.
Attachment #8858309 - Flags: review?(odvarko)
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
Attachment #8858309 - Flags: review?(odvarko) → review+
(Assignee)

Comment 45

2 years ago
rebased
Attachment #8858298 - Attachment is obsolete: true
Attachment #8861308 - Flags: review+
(Assignee)

Comment 46

2 years ago
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
(Assignee)

Comment 47

2 years ago
(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.
(Assignee)

Comment 48

2 years ago
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.
Attachment #8861308 - Flags: sec-approval?
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.
Attachment #8861308 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/9bf8e9f799c3
https://hg.mozilla.org/mozilla-central/rev/32aa533e3335
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?).
Group: network-core-security → core-security-release
As a sec-low, we wouldn't normally take this on ESR52 without a reason.
Whiteboard: [necko-active][fingerprinting] → [necko-active][fingerprinting][adv-main55+]
Arthur, is this something that would be useful to backport to ESR52 for Tor?
Flags: needinfo?(arthuredelstein)
Alias: CVE-2017-7797
(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.
Flags: needinfo?(arthuredelstein)
Thanks for the reply, Arthur. Dragana, can you please nominate this for ESR52 approval? Thanks :)
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 56

2 years ago
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.
Flags: needinfo?(dd.mozilla) → needinfo?(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.
Flags: needinfo?(mcmanus)
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).
Flags: qe-verify-
Whiteboard: [necko-active][fingerprinting][adv-main55+] → [necko-active][fingerprinting][adv-main55+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.