1344461 - (CVE-2017-5444) Heap buffer overflow in nsDirIndexParser::ParseData

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Chamal De Silva]

Attachment: 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.

Comment 1

[Chamal De Silva]

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';

Comment 2

[Chamal De Silva]

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?

Comment 3

[Patrick McManus [:mcmanus]]

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

Comment 4

[Patrick McManus [:mcmanus]]

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

Comment 5

[Jason Duell]

Hoping bagder can take this.

Comment 6

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1344461-keep-track-of-line-length-to-not-read-be.patch

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?

Comment 7

[Daniel Stenberg [:bagder]]

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)

Comment 8

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

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.

Comment 9

[Daniel Stenberg [:bagder]]

Thanks a lot for your keen eyes there Valentin, spot on in both cases!

Comment 10

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-bug-1344461-keep-track-of-line-length-to-not-read.patch

Take 2, fixed mistakes from first review.

Comment 11

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

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!

Comment 12

[Daniel Stenberg [:bagder]]

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.

Comment 13

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

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).

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef67e15fd88

Please request Aurora/Beta/ESR52/ESR45 approval on this as soon as possible.

Comment 15

[Daniel Stenberg [:bagder]]

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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ef67e15fd88

Comment 17

[Gerry Chang [:gchang]]

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+.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d5511eaf29c9

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1171236daacc

Comment 22

[Julien Cristau [:jcristau]]

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+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/7740cf7e121b
https://hg.mozilla.org/releases/mozilla-esr45/rev/6f14d2ef7981

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 25

[Daniel Stenberg [:bagder]]

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.

Comment 26

[Daniel Stenberg [:bagder]]

"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! =(

Comment 27

[Ryan VanderMeulen [:RyanVM]]

I retriggered it 5 times on opt & debug prior to comment 24.

Comment 28

[Daniel Stenberg [:bagder]]

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?

Comment 29

[Andrea Marchesini [:baku]]

+1 I agree. appId is not used in ESR45 and it has been removed in m-c.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Skipped on OSX 10.6.
https://hg.mozilla.org/releases/mozilla-esr45/rev/baf50ff72894

Comment 31

[Paul Silaghi, QA [:pauly]]

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.