1342661 - (CVE-2017-5443) Out of bound read in nsBinHexDecoder::DetectContentType

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Chamal De Silva]

Attachment: binhex.hqx

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

1. Build Firefox with Address Sanitizer.
   I built with options and instructions  mentioned in "Manual Build" section in
   https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer
2. Start firefox.
3. Open attached binhex.hqx.



Actual results:

Address Sanitizer will display this error.

==28086==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000232040 at pc 0x00000044b58e bp 0x7fff4a98c270 sp 0x7fff4a98ba20
READ of size 10 at 0x602000232040 thread T0 (Web Content)
    #0 0x44b58d in __interceptor_strrchr.part.28 clang/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:499
    #1 0x44b58d in ?? ??:0
    #2 0x7f984665893e in mozilla::net::nsBinHexDecoder::DetectContentType(nsIRequest*, nsCString const&) /firefox/netwerk/streamconv/converters/nsBinHexDecoder.cpp:486 (discriminator 2)
    #3 0x7f984665893e in ?? ??:0
    #4 0x7f9846657dd9 in mozilla::net::nsBinHexDecoder::ProcessNextState(nsIRequest*, nsISupports*) /firefox/netwerk/streamconv/converters/nsBinHexDecoder.cpp:185
    #5 0x7f9846657dd9 in ?? ??:0
    #6 0x7f984665731d in mozilla::net::nsBinHexDecoder::ProcessNextChunk(nsIRequest*, nsISupports*, unsigned int) /firefox/netwerk/streamconv/converters/nsBinHexDecoder.cpp:408
    #7 0x7f984665731d in ?? ??:0
    #8 0x7f9846656b24 in mozilla::net::nsBinHexDecoder::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /firefox/netwerk/streamconv/converters/nsBinHexDecoder.cpp:141
    ...................


0x602000232040 is located 0 bytes to the right of 16-byte region [0x602000232030,0x602000232040)
allocated by thread T0 (Web Content) here:
    #0 0x4d2628 in __interceptor_malloc _asan_rtl_
    #1 0x4d2628 in ?? ??:0
    #2 0x7f9845f57709 in nsStringBuffer::Alloc(unsigned long) /firefox/xpcom/string/nsSubstring.cpp:217
    #3 0x7f9845f57709 in ?? ??:0
    #4 0x7f9845f55b94 in nsACString_internal::MutatePrep(unsigned int, char**, unsigned int*) /firefox/xpcom/string/nsTSubstring.cpp:158
    #5 0x7f9845f55b94 in ?? ??:0
    #6 0x7f9845f4ae40 in nsACString_internal::SetCapacity(unsigned int, mozilla::fallible_t const&) /firefox/xpcom/string/nsTSubstring.cpp:696
    #7 0x7f9845f4ae40 in ?? ??:0
    #8 0x7f9845f5eb2c in nsACString_internal::SetCapacity(unsigned int) /firefox/xpcom/string/nsTSubstring.cpp:675
    #9 0x7f9845f5eb2c in ?? ??:0
    #10 0x7f9845f4a741 in nsACString_internal::SetLength(unsigned int) /firefox/xpcom/string/nsTSubstring.cpp:727
    #11 0x7f9845f4a741 in ?? ??:0
    #12 0x7f9846657ce6 in mozilla::net::nsBinHexDecoder::ProcessNextState(nsIRequest*, nsISupports*) /firefox/netwerk/streamconv/converters/nsBinHexDecoder.cpp:170
    #13 0x7f9846657ce6 in ?? ??:0
    #14 0x7f984665731d in mozilla::net::nsBinHexDecoder::ProcessNextChunk(nsIRequest*, nsISupports*, unsigned int) /firefox/netwerk/streamconv/converters/nsBinHexDecoder.cpp:408
    #15 0x7f984665731d in ?? ??:0
    #16 0x7f9846656b24 in mozilla::net::nsBinHexDecoder::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /firefox/netwerk/streamconv/converters/nsBinHexDecoder.cpp:141
    ...................


Expected results:

Should not read out of bound data .

Comment 1

[Chamal De Silva]

Cause of Bug
------------
Bug is present in below mentioned lines of ProcessNextState method in netwerk\streamconv\converters\nsBinHexDecoder.cpp file.

mName.BeginWriting()[mCount] = c;
if (++mCount > mName.Length())
{
  ...
  DetectContentType(aRequest, mName);

Above if condition should also check whether "++mCount" is equal to mName.Length(). Otherwise "mName.BeginWriting()[mCount] = c;" will write a character to string terminating null value.

Comment 2

[:Gijs (he/him)]

Jason, can you or someone else in the networking team look at this? Thanks.

Comment 3

[Jason Duell]

Daniel: any chance you could look at this?

Comment 4

[Tyson Smith [:tsmith]]

Attachment: valgrind_log.txt

Comment 5

[Tyson Smith [:tsmith]]

Attachment: log.txt

Fully symbolized ASan log

Comment 6

[Eric Rahm [:erahm]]

My $.02 here: we should just disable this decoder or at least fuzz it extensively.

Comment 7

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1342661-out-of-bounds-check-before-assigning-r.patch

[This problem seems to have existed since at least this commit: https://hg.mozilla.org/releases/mozilla-1.9.1/rev/194b881050ac (August 2009) - which modified the range check from the previous version, but it may have existed before that. I didn't research the previous logic.]

The question I've not really worked out for myself is weather we should error out completely on this condition or if we only should ignore the specific bad write. The patch I'm attaching here does the latter since it is the least invasive change that should still avoid the bad assign.

If I got this right, the trailing-zero overwrite is more or less done on purpose since the binhex format is supposed to end the file name with a zero byte so the original code from before 2009 that didn't use a nsCString string had to zero terminate the string. The mistake was to assume that there would always be a zero byte in the source.

Thoughts?

Comment 8

[Jason Duell]

Daniel--is there anyone (from hg blame if nothing else) who's a logical person to review your patch?

Comment 9

[Jason Duell]

Raymond--any chance we could get someone to fuzz this decoder some time? (see comment 6)

Comment 10

[Eric Rahm [:erahm]]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #9)
> Raymond--any chance we could get someone to fuzz this decoder some time?
> (see comment 6)

I think Tyson is interested in this, I'm not sure if he's actively pursuing it.

Comment 11

[Tyson Smith [:tsmith]]

(In reply to Eric Rahm [:erahm] from comment #10)
> (In reply to Jason Duell [:jduell] (needinfo me) from comment #9)
> > Raymond--any chance we could get someone to fuzz this decoder some time?
> > (see comment 6)
> 
> I think Tyson is interested in this, I'm not sure if he's actively pursuing
> it.

Sure I can put up a dumb fuzzer tonight targeting "application/mac-binhex40". Should I add any other content types to the list? Keep in mind this will be very dumb for the time being.

Comment 12

[Daniel Stenberg [:bagder]]

Valentin, you reviewed a smallish patch in this area recently. Would you mind giving my patch/idea above a look?

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8843938 [details] [diff] [review]
0001-bug-1342661-out-of-bounds-check-before-assigning-r.patch

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

The patch looks good. Thanks for fixing this.
It's funny that this code was dependent on a non-zero uninitialized value for so long. But then again mac-binhex40 probably isn't that common.

Comment 14

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-bug-1342661-boundary-check-before-assigning-r-val.patch

V2 of the patch with correct "r=valentin" designation

Comment 15

[Daniel Stenberg [:bagder]]

Comment on attachment 8844642 [details] [diff] [review]
v2-0001-bug-1342661-boundary-check-before-assigning-r-val.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It easy to figure out how to do an out of bounds write, making an exploit out if it is slightly trickier but probably not terribly complicated.

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

It is *only* an added boundary check so even with no added comments it may trigger some people's interest.

Which older supported branches are affected by this flaw?

All. The bug has been around since a long time, but happens easier since bug 1276625 was fixed in version 49.

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?

The same patch is likely to work on all branches.

How likely is this patch to cause regressions; how much testing does it need?

I estimate a very low risk. It is only a single added boundary check.

Comment 16

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

sec-approval+. Go ahead and check this in. We'll want this on branches as well.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d23fe234d19944959226301061b89e5192f71b3f

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d23fe234d199

Comment 19

[Daniel Stenberg [:bagder]]

Comment on attachment 8844642 [details] [diff] [review]
v2-0001-bug-1342661-boundary-check-before-assigning-r-val.patch

Approval Request Comment
[Feature/Bug causing the regression]: 1342661
[User impact if declined]: this is a security issue, so makes the users less secure if declined
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: there's a repro in the bug
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: the change is minimal
[String changes made/needed]: none

Comment 21

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

Comment on attachment 8844642 [details] [diff] [review]
v2-0001-bug-1342661-boundary-check-before-assigning-r-val.patch

Sec-high, approved for landing on all branches except release.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7744be94bea
https://hg.mozilla.org/releases/mozilla-beta/rev/f6bc529e2121

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/6daaaff9f1f6
https://hg.mozilla.org/releases/mozilla-esr45/rev/d449995ef7d9

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8844642 [details] [diff] [review]
v2-0001-bug-1342661-boundary-check-before-assigning-r-val.patch

This is already on 53 (beta and release branches).

Comment 25

[Paul Silaghi, QA [:pauly]]

Reproduced the issue on nightly 2017-02-25 asan build, Ubuntu 14.04 x64.
Verified fixed Fx 53b12, 54.0a2 (2017-04-12), 55.0a1 (2017-04-12), esr52.1.0, esr45.9.0 asan builds.