1289085 - (CVE-2016-2827) Out-of-bounds read in mozilla::net::IsValidReferrerPolicy

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Atte Kettunen]

Attachment: firefox-heap-buffer-overflow-IsEmpty10-min.html

Tested on:

OS: Ubuntu 14.04

Firefox: ASAN build from  https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-asan/1469354528/

Repro-file as an attachment.

ASAN-trace:

==29350==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6030005a6150 at pc 0x7fc156120f40 bp 0x7ffdb447f6b0 sp 0x7ffdb447f6a8
READ of size 4 at 0x6030005a6150 thread T0 (Web Content)
    #0 0x7fc156120f3f in IsEmpty /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTSubstring.h:242:12
    #1 0x7fc156120f3f in mozilla::net::IsValidReferrerPolicy(nsAString_internal const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/net/ReferrerPolicy.h:93:0
    #2 0x7fc156143c10 in nsCSPParser::referrerDirectiveValue() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/security/nsCSPParser.cpp:917:8
    #3 0x7fc156148820 in nsCSPParser::directive() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/security/nsCSPParser.cpp:1219:3
    #4 0x7fc15614956a in nsCSPParser::policy() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/security/nsCSPParser.cpp:1258:5
    #5 0x7fc156121997 in nsCSPParser::parseContentSecurityPolicy(nsAString_internal const&, nsIURI*, bool, nsCSPContext*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/security/nsCSPParser.cpp:1301:25
    #6 0x7fc1561211bb in nsCSPContext::AppendPolicy(nsAString_internal const&, bool, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/security/nsCSPContext.cpp:386:25
    #7 0x7fc15253ba66 in nsHtml5TreeOpExecutor::AddSpeculationCSP(nsAString_internal const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:1049:8
    #8 0x7fc15253b392 in nsHtml5SpeculativeLoad::Perform(nsHtml5TreeOpExecutor*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/parser/html/nsHtml5SpeculativeLoad.cpp:31:7
    #9 0x7fc1525b43ea in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/slave/m-cen-l64-asan-000000000000000/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:396:9
    #10 0x7fc1525b916b in nsHtml5ExecutorFlusher::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/parser/html/nsHtml5StreamParser.cpp:128:9
.
.
.
0x6030005a6150 is located 8 bytes to the right of 24-byte region [0x6030005a6130,0x6030005a6148)
allocated by thread T0 (Web Content) here:
    #0 0x4b522b in __interceptor_malloc _asan_rtl_:3
    #1 0x4e2efd in moz_xmalloc /builds/slave/m-cen-l64-asan-000000000000000/build/src/memory/mozalloc/mozalloc.cpp:83:17
    #2 0x7fc15040749d in Malloc /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:178:46
    #3 0x7fc15040749d in nsTArrayInfallibleAllocator::ResultTypeProxy nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity<nsTArrayInfallibleAllocator>(unsigned long, unsigned long) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray-inl.h:136:0
    #4 0x7fc151423f7c in nsString* nsTArray_Impl<nsString, nsTArrayInfallibleAllocator>::ReplaceElementsAt<nsString, nsTArrayInfallibleAllocator>(unsigned long, unsigned long, nsString const*, unsigned long) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:1870:32
    #5 0x7fc156149562 in operator= /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:891:7
    #6 0x7fc156149562 in operator= /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:2057:0
    #7 0x7fc156149562 in nsCSPParser::policy() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/security/nsCSPParser.cpp:1257:0
    #8 0x7fc156121997 in nsCSPParser::parseContentSecurityPolicy(nsAString_internal const&, nsIURI*, bool, nsCSPContext*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/security/nsCSPParser.cpp:1301:25
.
.
.

Comment 1

[Daniel Veditz [:dveditz]]

christoph: is this only going to read bogus values into a CSP (so a lower severity csp bypass) or is it due to us being so screwed up that we're going to corrupt memory further?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> christoph: is this only going to read bogus values into a CSP (so a lower
> severity csp bypass) or is it due to us being so screwed up that we're going
> to corrupt memory further?

In this case where a CSP of 'referrer' is delivered without any valid srcs (e.g. it's not delivered like 'referrer no-referrer') our parser is reading a bogus value and tries to convert that value into a valid referrerPolicy.

If you run the test in a debug build you actually trigger an assertion (see underneath). Nevertheless, it's not handled gracefully within the CSP parser.

Program received signal SIGSEGV, Segmentation fault.
0x00007fffe67ae6f5 in nsTArray_Impl<nsString, nsTArrayInfallibleAllocator>::ElementAt (this=0x7fffffffc160, aIndex=1)
    at /home/ckerschb/moz/mc-obj-ff-dbg/dist/include/nsTArray.h:992
992	    MOZ_ASSERT(aIndex < Length(), "invalid array index");
(gdb) bt
#0  0x00007fffe67ae6f5 in nsTArray_Impl<nsString, nsTArrayInfallibleAllocator>::ElementAt (this=0x7fffffffc160, aIndex=1)
    at /home/ckerschb/moz/mc-obj-ff-dbg/dist/include/nsTArray.h:992
#1  0x00007fffe67acd67 in nsTArray_Impl<nsString, nsTArrayInfallibleAllocator>::operator[] (this=0x7fffffffc160, aIndex=1)
    at /home/ckerschb/moz/mc-obj-ff-dbg/dist/include/nsTArray.h:1027
#2  0x00007fffe8820707 in nsCSPParser::referrerDirectiveValue (this=0x7fffffffc130) at /home/ckerschb/moz/mc/dom/security/nsCSPParser.cpp:917
#3  0x00007fffe88211ac in nsCSPParser::directiveValue (this=0x7fffffffc130, outSrcs=...) at /home/ckerschb/moz/mc/dom/security/nsCSPParser.cpp:1049
#4  0x00007fffe8821bda in nsCSPParser::directive (this=0x7fffffffc130) at /home/ckerschb/moz/mc/dom/security/nsCSPParser.cpp:1219
#5  0x00007fffe8821e2c in nsCSPParser::policy (this=0x7fffffffc130) at /home/ckerschb/moz/mc/dom/security/nsCSPParser.cpp:1258
#6  0x00007fffe882211d in nsCSPParser::parseContentSecurityPolicy (aPolicyString=..., aSelfURI=0x7fffc514fd00, aReportOnly=false, aCSPContext=0x7fffc48f4ca0, 
    aDeliveredViaMetaTag=true) at /home/ckerschb/moz/mc/dom/security/nsCSPParser.cpp:1301
#7  0x00007fffe8818127 in nsCSPContext::AppendPolicy (this=0x7fffc48f4ca0, aPolicyString=..., aReportOnly=false, aDeliveredViaMetaTag=true)
    at /home/ckerschb/moz/mc/dom/security/nsCSPContext.cpp:391

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1289085_csp_referrer_directive_with_no_valid_src.patch

The problem was introduced within
> Bug 965727 - Implement referrer directive for CSP. (r=jst,ckerschb)

Bug 965727 checks that the referrer directive does not contain too many srcs but doesn't check if there are not enough [1] - really unfortunate!

While investigating the problem related to this bug I also realized that we don't handle 'sandbox', 'report-uri' and also 'require-sri' directives correctly if delivered without a src. In those cases it's not a memory-corruption problem though, because the parser iterates over the remaining srcs instead of indexing into the array (see [2] for example). I filed Bug 1290560 to fix those problems, no need to pollute this patch with those fixes. Also because this patch most likely will be upliftet.

[1] https://hg.mozilla.org/mozilla-central/diff/eabee47625c6/dom/security/nsCSPParser.cpp#l1.37
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#928

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1289085_csp_referrer_directive_with_no_valid_src_tests.patch

Comment 5

[Daniel Veditz [:dveditz]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> our parser is reading a bogus value and tries to convert that value
> into a valid referrerPolicy.

This doesn't seem particularly exploitable: either it will crash the user or the referrer policy will be invalid and ignored. In rare cases it might match a valid value and change the type of referrer that gets sent.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 8776138 [details] [diff] [review]
bug_1289085_csp_referrer_directive_with_no_valid_src.patch

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

r=dveditz if you fix the comments

::: dom/security/nsCSPParser.cpp
@@ +913,2 @@
>                   mCurDir.Length() - 1));
> +    delete aDir;

I notice that in nsCSPParser::requireSRIForDirectiveValue() the error case does not delete aDir. I think you're right here and we have a leak in the other one so maybe you can fix that in this patch.

@@ +1207,5 @@
>  
> +  // special case handling of the referrer directive (since it doesn't contain
> +  // source lists)
> +  if (cspDir->equals(nsIContentSecurityPolicy::REFERRER_DIRECTIVE)) {
> +    referrerDirectiveValue(static_cast<nsRequireSRIForDirective*>(cspDir));

casting to a nsRequireSRIForDirective* can't be right. The method's argument is a nsCSPDirective* and cspDir is already a nsCSPDirective* so I don't think you need the cast, do you?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1289085_csp_referrer_directive_with_no_valid_src.patch

(In reply to Daniel Veditz [:dveditz] from comment #6)
> > +    delete aDir;
> 
> I notice that in nsCSPParser::requireSRIForDirectiveValue() the error case
> does not delete aDir. I think you're right here and we have a leak in the
> other one so maybe you can fix that in this patch.

Yes, there is a leak. I'll fix that within Bug 1290560 as well, since requrire-sri is prefed off by default anyway.
 
> > +    referrerDirectiveValue(static_cast<nsRequireSRIForDirective*>(cspDir));
> 
> casting to a nsRequireSRIForDirective* can't be right. The method's argument
> is a nsCSPDirective* and cspDir is already a nsCSPDirective* so I don't
> think you need the cast, do you?

You are absolutely right, no need for that cast. That was a copy/paste. Thanks for catching.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7dc3515dc78
https://hg.mozilla.org/mozilla-central/rev/cf65390f5764

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8776140 [details] [diff] [review]
bug_1289085_csp_referrer_directive_with_no_valid_src_tests.patch

Approval Request Comment
A webpage sending the CSP policy of only 'referrer' (without any src after the directive) might crash the browser because Firefox is reading a value from out of bounds.

[Feature/regressing bug #]:
Bug 965727 - Implement CSP 1.1 referrer directive

[User impact if declined]:
I don't know how many sites would send a CSP policy of 'referrer' only, but the fact that a security feature might crash the browser makes me feel we should uplift the fix within this bug.

[Describe test coverage new/current, TreeHerder]:
Landed a compiled code test within this bug.

[Risks and why]:
Low, because the change only affects the CSP parser.

[String/UUID change made/needed]:
no

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8776342 [details] [diff] [review]
bug_1289085_csp_referrer_directive_with_no_valid_src.patch

See comment 9.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8776140 [details] [diff] [review]
bug_1289085_csp_referrer_directive_with_no_valid_src_tests.patch

Sec issue, early in Beta49 cycle, taking it.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Christoph, given that ESR45 is affected, should we consider uplifting this to ESR45 branch as well? If so, please nominate patches for ESR45 uplift.

Comment 13

[Carsten Book [:Tomcat]]

has conflicts uplifting to beta:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r b7dc3515dc78
grafting 357014:b7dc3515dc78 "Bug 1289085: CSP - Bail early if referrer directive has no valid src. r=dveditz"
merging dom/security/nsCSPParser.cpp
merging dom/security/nsCSPParser.h
warning: conflicts while merging dom/security/nsCSPParser.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8776140 [details] [diff] [review]
bug_1289085_csp_referrer_directive_with_no_valid_src_tests.patch

See comment 9.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8776342 [details] [diff] [review]
bug_1289085_csp_referrer_directive_with_no_valid_src.patch

See comment 9.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1289085_rebased_for_beta.patch

(In reply to Carsten Book [:Tomcat] from comment #13)
> has conflicts uplifting to beta:

Hey Carsten, I rebased the main patch for beta; the tests apply cleanly for beta. Let me know if that still doesn't work for whatever reason. Thanks!

Comment 17

[Carsten Book [:Tomcat]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)
> Created attachment 8779311 [details] [diff] [review]
> bug_1289085_rebased_for_beta.patch
> 
> (In reply to Carsten Book [:Tomcat] from comment #13)
> > has conflicts uplifting to beta:
> 
> Hey Carsten, I rebased the main patch for beta; the tests apply cleanly for
> beta. Let me know if that still doesn't work for whatever reason. Thanks!

thanks! worked perfect! landed in https://hg.mozilla.org/releases/mozilla-beta/rev/adadc9dbb648

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8776140 [details] [diff] [review]
bug_1289085_csp_referrer_directive_with_no_valid_src_tests.patch

doesn't match the esr criteria (sec-high / sec-critical), not taking it

Comment 19

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original crash using the STR and the POC from comment #0. Once I reproduced the crash, went through the following verification process:

Builds Used:
* fx51.0a1 asan, buildId: 20160908150519
* fx50.0a2 asan, buildId: 20160909072720
* fx49.0b11 asan, buildId: 20160909092719

Platform Used:
* Ubuntu 16.04.1 LTS VM

Test Cases Used:

* ensured that the original issue wasn't occuring using the STR from comment #0
* opened the poc in several regular tabs/windows and ensured fx didn't crash
* opened the poc in several container tabs/windows and ensured fx didn't crash
* opened the poc in several non-e10s tabs/windows and ensured fx didn't crash
* opened the poc in several PB tabs/windows and ensured fx didn't crash