Bug 1344461 (CVE-2017-5444)

Heap buffer overflow in nsDirIndexParser::ParseData

VERIFIED FIXED in Firefox -esr45

Status

()

VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: chamal.desilva, Assigned: bagder)

Tracking

({sec-high})

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

Firefox Tracking Flags

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

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Posted file httpindex.php
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. Download and host httpindex.php file on a web server.
    Web server should support PHP.
3. Start firefox.
4. Visit httpindex.php.


Actual results:

Firefox tab crashed.

Address Sanitizer Output
---------------------------
==4312==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6030002b7ac0 at pc 0x7fea5b5e3fc5 bp 0x7fff6d37d1b0 sp 0x7fff6d37d1a8
READ of size 1 at 0x6030002b7ac0 thread T0 (Web Content)
    #0 0x7fea5b5e3fc4 in nsDirIndexParser::ParseData(nsIDirIndex*, char*) firefox/netwerk/streamconv/converters/nsDirIndexParser.cpp:233 (discriminator 1)
    #1 0x7fea5b5e3fc4 in ?? ??:0
    #2 0x7fea5b5e27bf in nsDirIndexParser::ProcessData(nsIRequest*, nsISupports*) firefox/netwerk/streamconv/converters/nsDirIndexParser.cpp:407 (discriminator 1)
    #3 0x7fea5b5e27bf in ?? ??:0
    #4 0x7fea5b5e41a3 in nsDirIndexParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) firefox/netwerk/streamconv/converters/nsDirIndexParser.cpp:350
    #5 0x7fea5b5e41a3 in ?? ??:0
    #6 0x7fea5b783b9b in mozilla::net::HttpChannelChild::DoOnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) firefox/netwerk/protocol/http/HttpChannelChild.cpp:799 (discriminator 2)
    #7 0x7fea5b783b9b in ?? ??:0
    ...............................

0x6030002b7ac0 is located 0 bytes to the right of 32-byte region [0x6030002b7aa0,0x6030002b7ac0)
allocated by thread T0 (Web Content) here:
    #0 0x4d2628 in __interceptor_malloc _asan_rtl_
    #1 0x4d2628 in ?? ??:0
    #2 0x7fea5aedd939 in nsStringBuffer::Alloc(unsigned long) firefox/xpcom/string/nsSubstring.cpp:217
    #3 0x7fea5aedd939 in ?? ??:0
    #4 0x7fea5aedbdc4 in nsACString_internal::MutatePrep(unsigned int, char**, unsigned int*) firefox/xpcom/string/nsTSubstring.cpp:172
    #5 0x7fea5aedbdc4 in ?? ??:0
    #6 0x7fea5aed1170 in nsACString_internal::SetCapacity(unsigned int, mozilla::fallible_t const&) firefox/xpcom/string/nsTSubstring.cpp:710
    #7 0x7fea5aed1170 in ?? ??:0
    #8 0x7fea5aed1501 in nsACString_internal::SetLength(unsigned int, mozilla::fallible_t const&) firefox/xpcom/string/nsTSubstring.cpp:748
    #9 0x7fea5aed1501 in ?? ??:0
    #10 0x7fea5b5e40eb in nsDirIndexParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) firefox/netwerk/streamconv/converters/nsDirIndexParser.cpp:336
    #11 0x7fea5b5e40eb in ?? ??:0
    #12 0x7fea5b783b9b in mozilla::net::HttpChannelChild::DoOnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) firefox/netwerk/protocol/http/HttpChannelChild.cpp:799 (discriminator 2)
    #13 0x7fea5b783b9b in ?? ??:0
    ...............................


Expected results:

Firefox tab should not crash due to a Heap buffer overflow.
(Reporter)

Updated

2 years ago
Attachment #8843558 - Attachment mime type: application/x-php → text/plain
(Reporter)

Updated

2 years ago
Group: firefox-core-security → core-security
Component: Untriaged → Networking
Product: Firefox → Core
(Reporter)

Comment 1

2 years ago
Cause of bug
-------------
Bug is in nsDirIndexParser::ParseData method of netwerk\streamconv\converters\nsDirIndexParser.cpp file.
This is the http-index-format content which causes this bug.
200:a a a\r\n"
201:a\r\n"
Header line has 3 tokens. But data line has only 1 token. 
This method should  validate number of tokens in data line.

This method also has 3 places where it advances char* aDataStr pointer without checking whether pointer has reached null terminator.
Ex: *aDataStr++ = '\0';
(Reporter)

Updated

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

Comment 2

2 years ago
This bug can be used to read some data from memory and display on a web page.
Is it OK if I attach a proof of concept test case here?
application/http-index-forma(In reply to Chamal De Silva from comment #2)
> This bug can be used to read some data from memory and display on a web page.
> Is it OK if I attach a proof of concept test case here?

not necessary - thanks
Keywords: sec-high
jason - this is an old legacy parser for application/http-index-format .. hasn't been touched in a long time, but looks easy enough to clean up. Can you find an asignee for a sec-high
Flags: needinfo?(jduell.mcbugs)
Flags: sec-bounty?
See Also: → bug 1346419
Group: core-security → network-core-security

Comment 5

2 years ago
Hoping bagder can take this.
Flags: needinfo?(daniel)
(Assignee)

Updated

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

Comment 6

2 years ago
This bug exists already in the code imported from CVS to hg back in 2007. It seems to originate from Bug 78148 (commit 584595884a1521 on gecko-dev@github)

My proposed patch here is to simply also pass in the line length as an argument and keep track of how much there's left when the pointer is advanced, so that the code won't read or write outside of the given constraints. Instead it will return an error from the parser function, something that wasn't done before! With my patch applied, the already attached httpindex.php test case scenario (and variations of it) seems to no longer cause any problems.

I found no existing tests using the format as input.

This patch is intended to also fix Bug 1344467 which is closely related.

Jason, any suggestion on a reviewer for this?
(Assignee)

Updated

2 years ago
See Also: → bug 1344467
(Assignee)

Comment 7

2 years ago
Comment on attachment 8846576 [details] [diff] [review]
0001-bug-1344461-keep-track-of-line-length-to-not-read-be.patch

(the commit message doesn't have the correct r= at this point)
Attachment #8846576 - Flags: review?(valentin.gosu)
Comment on attachment 8846576 [details] [diff] [review]
0001-bug-1344461-keep-track-of-line-length-to-not-read-be.patch

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

::: netwerk/streamconv/converters/nsDirIndexParser.cpp
@@ +254,5 @@
>          ++aDataStr;
> +        --lineLen;
> +      }
> +      if (lineLen > 0) {
> +        *aDataStr++ = '\0';

Shouldn't there be another --lineLen; here?

@@ +429,5 @@
>              nsCOMPtr<nsIDirIndex> idx = do_CreateInstance("@mozilla.org/dirIndex;1",&rv);
>              if (NS_FAILED(rv))
>                return rv;
>              
> +            rv = ParseData(idx, ((char *)buf) + 4, lineLen);

It seems like this was supposed to be lineLen - 4.
(Assignee)

Comment 9

2 years ago
Thanks a lot for your keen eyes there Valentin, spot on in both cases!
(Assignee)

Comment 10

2 years ago
Take 2, fixed mistakes from first review.
Attachment #8846576 - Attachment is obsolete: true
Attachment #8846576 - Flags: review?(valentin.gosu)
Attachment #8847016 - Flags: review?(valentin.gosu)
Comment on attachment 8847016 [details] [diff] [review]
v2-0001-bug-1344461-keep-track-of-line-length-to-not-read.patch

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

Looks good. Thanks!
Attachment #8847016 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 12

2 years ago
Comment on attachment 8847016 [details] [diff] [review]
v2-0001-bug-1344461-keep-track-of-line-length-to-not-read.patch

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

Fairly easy.

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

The current commit message might be a bit too direct since it even speaks of "beyond eol". The commit itself does introduce a buffer range control and isn't very complicated so that fact is easily spotted.

Which older supported branches are affected by this flaw?

Everything since October 2001(!)

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?

This code area is very old and hasn't changed much in recent time so backports should be straight-forward.

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

I'd say it is low risk (based on the relative simplicity of the patch) and it is a fairly unused feature.
Attachment #8847016 - Flags: sec-approval?
sec-approval+ for checkin on March 21, which is two weeks into the current dev cycle. 
We should get patches for affected branches made and nominated (including ESR).
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox53: --- → +
tracking-firefox54: --- → +
tracking-firefox55: --- → +
tracking-firefox-esr45: --- → 53+
tracking-firefox-esr52: --- → 53+
Whiteboard: [checkin on 3/21]
Attachment #8847016 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef67e15fd88

Please request Aurora/Beta/ESR52/ESR45 approval on this as soon as possible.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(daniel)
Whiteboard: [checkin on 3/21]
(Assignee)

Comment 15

2 years ago
Comment on attachment 8847016 [details] [diff] [review]
v2-0001-bug-1344461-keep-track-of-line-length-to-not-read.patch

Approval Request Comment
[Feature/Bug causing the regression]:

This patch fixes bug 1344461 and most likely bug 1344467 and bug 1346419 at the same time. All security related bugs.

[User impact if declined]: The sec-high issue remains
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet, just landed in incoming
[Needs manual test from QE? If yes, steps to reproduce]: yes, STR exists in the bug
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: I estimate low risk
[Why is the change risky/not risky?]: it fixes a feature that is rarely used
[String changes made/needed]: none
Flags: needinfo?(daniel)
Attachment #8847016 - Flags: approval-mozilla-esr52?
Attachment #8847016 - Flags: approval-mozilla-esr45?
Attachment #8847016 - Flags: approval-mozilla-beta?
Attachment #8847016 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5ef67e15fd88
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8847016 [details] [diff] [review]
v2-0001-bug-1344461-keep-track-of-line-length-to-not-read.patch

Fix a sec-high. Aurora54+ & Beta53+.
Attachment #8847016 - Flags: approval-mozilla-beta?
Attachment #8847016 - Flags: approval-mozilla-beta+
Attachment #8847016 - Flags: approval-mozilla-aurora?
Attachment #8847016 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1346419
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1344467

Updated

2 years ago
Flags: needinfo?(jduell.mcbugs)
Depends on: 1349954
Comment on attachment 8847016 [details] [diff] [review]
v2-0001-bug-1344461-keep-track-of-line-length-to-not-read.patch

fix a read out of bounds, esr45+/esr52+
Attachment #8847016 - Flags: approval-mozilla-esr52?
Attachment #8847016 - Flags: approval-mozilla-esr52+
Attachment #8847016 - Flags: approval-mozilla-esr45?
Attachment #8847016 - Flags: approval-mozilla-esr45+
Flags: sec-bounty? → sec-bounty+
OSX 10.6 (and *only* OSX 10.6) is hitting failures in caps/tests/mochitest/test_principal_jarprefix_origin_appid_appstatus.html on the ESR45 uplift.
https://treeherder.mozilla.org/logviewer.html#?job_id=86780118&repo=mozilla-esr45

Any ideas what might be happening here, Daniel?
Flags: needinfo?(daniel)
(Assignee)

Comment 25

2 years ago
Argh. It seems so completely random to me and no, I don't have any explanation for this atm. I don't see why this change would behave differently there or why that test would be affected in the first place! I need to dig deeper.
(Assignee)

Comment 26

2 years ago
"test timed out" even, which seems even more suspicious methinks. Can we/I rerun this test a few more times to see if it this happens all the time? macos 10.6 is a bit tricky to test locally! =(
I retriggered it 5 times on opt & debug prior to comment 24.
(Assignee)

Comment 28

2 years ago
This failing test was removed in bug 1261019 and might take some effort to work out since it only happens on macos 10.6.

Can we maybe consider disabling that specific test in esr45 ? I find it unlikely that this is actually a bug in the code since it (seemingly) works on all other platforms and branches.

:baku, can you give me a thumbs up/down on that?
Flags: needinfo?(daniel) → needinfo?(amarchesini)
+1 I agree. appId is not used in ESR45 and it has been removed in m-c.
Flags: needinfo?(amarchesini)
Group: network-core-security → core-security-release
Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+]
Alias: CVE-2017-5444
Reproduced the issue on nightly 2017-03-04 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.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
status-firefox54: fixed → verified
status-firefox55: fixed → verified
status-firefox-esr45: fixed → verified
status-firefox-esr52: fixed → verified
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.