Bug 1342661 (CVE-2017-5443)

Out of bound read in nsBinHexDecoder::DetectContentType

VERIFIED FIXED in Firefox -esr45

Status

()

defect
VERIFIED FIXED
2 years ago
8 months ago

People

(Reporter: chamal.desilva, Assigned: bagder)

Tracking

({csectype-bounds, sec-high})

51 Branch
mozilla55
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr4553+ verified, firefox51 wontfix, firefox52 wontfix, firefox-esr5253+ verified, firefox53+ verified, firefox54+ verified, firefox55+ verified)

Details

(Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

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

Updated

2 years ago
OS: Unspecified → All
Hardware: Unspecified → All
(Reporter)

Updated

2 years ago
Attachment #8841189 - Attachment mime type: application/mac-binhex40 → text/plain

Comment 2

2 years ago
Jason, can you or someone else in the networking team look at this? Thanks.
Group: firefox-core-security → core-security
Component: Untriaged → Networking
Flags: needinfo?(jduell.mcbugs)
Product: Firefox → Core

Comment 3

2 years ago
Daniel: any chance you could look at this?
Flags: needinfo?(daniel)
Group: core-security → network-core-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Posted file log.txt
Fully symbolized ASan log
My $.02 here: we should just disable this decoder or at least fuzz it extensively.
(Assignee)

Updated

2 years ago
Assignee: nobody → daniel
Flags: needinfo?(daniel)
(Assignee)

Comment 7

2 years ago
[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

2 years ago
Daniel--is there anyone (from hg blame if nothing else) who's a logical person to review your patch?
Flags: needinfo?(jduell.mcbugs) → needinfo?(daniel)

Comment 9

2 years ago
Raymond--any chance we could get someone to fuzz this decoder some time? (see comment 6)
Flags: needinfo?(rforbes)
(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.
Flags: needinfo?(twsmith)
(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.
Flags: needinfo?(twsmith)
(Assignee)

Comment 12

2 years ago
Valentin, you reviewed a smallish patch in this area recently. Would you mind giving my patch/idea above a look?
Flags: needinfo?(daniel) → needinfo?(valentin.gosu)
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.
Attachment #8843938 - Flags: review+
Flags: needinfo?(valentin.gosu)
(Assignee)

Comment 14

2 years ago
V2 of the patch with correct "r=valentin" designation
Attachment #8843938 - Attachment is obsolete: true
Attachment #8844642 - Flags: review+
(Assignee)

Comment 15

2 years ago
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.
Attachment #8844642 - Flags: sec-approval?
sec-approval+. Go ahead and check this in. We'll want this on branches as well.
Attachment #8844642 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d23fe234d199
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 19

2 years ago
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
Attachment #8844642 - Flags: approval-mozilla-release?
Attachment #8844642 - Flags: approval-mozilla-esr52?
Attachment #8844642 - Flags: approval-mozilla-esr45?
Attachment #8844642 - Flags: approval-mozilla-beta?
Attachment #8844642 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1345004
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.
Attachment #8844642 - Flags: approval-mozilla-esr52?
Attachment #8844642 - Flags: approval-mozilla-esr52+
Attachment #8844642 - Flags: approval-mozilla-esr45?
Attachment #8844642 - Flags: approval-mozilla-esr45+
Attachment #8844642 - Flags: approval-mozilla-beta?
Attachment #8844642 - Flags: approval-mozilla-beta+
Attachment #8844642 - Flags: approval-mozilla-aurora?
Attachment #8844642 - Flags: approval-mozilla-aurora+
Group: network-core-security → core-security-release
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+]
Alias: CVE-2017-5443
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).
Attachment #8844642 - Flags: approval-mozilla-release? → approval-mozilla-release-
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.
Group: core-security-release
Flags: needinfo?(rforbes)
You need to log in before you can comment on or make changes to this bug.