1344467 - (CVE-2017-5445) Uninitialized value in nsDirIndexParser::ParseFormat

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Chamal De Silva]

Attachment: httpindex1.php

Steps to reproduce:
--------------------
1. Build firefox with valgrind using instructions mentioned in
   https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Valgrind
2. Download and copy httpindex1.php and httpindex2.php to local web server.
   Web server should support PHP.
3. Start firefox
   * Enable Multi-process option should be disabled in firefox.
     Otherwise firefox tab crash due to some other valgrind errors.
   export VALGRIND_OPTIONS="--smc-check=all-non-file --vex-iropt-register-updates=allregs-at-mem-access --show-mismatched-frees=no --read-inline-info=yes"
  ./mach run --debugger="valgrind" --debugger-args="$VALGRIND_OPTIONS"
4. Visit httpindex1.php or httpindex2.php

Valgrind will display error. Valgrind output is attached under Crash Signature.

Cause of bug
-------------
Bug is in nsDirIndexParser::ParseFormat method of netwerk\streamconv\converters\nsDirIndexParser.cpp file.
"mFormat" int array created by this method can contain uninitialized value due to 2 reasons.

1. "http-index-format" content does not contain any format values.
    ex: 200:\r\n
        201:a\r\n
   nsDirIndexParser::ParseFormat method creates an int array with 2 elements for "mFormat", even though there are no values in format line. 
2. "http-index-format" content contains invalid values. 
   ex: 200:a a a
       201:a a a
   nsDirIndexParser::ParseFormat method creates an int array with 4 elements for "mFormat", even though there are invalid values in format line.

Comment 1

[Chamal De Silva]

Attachment: httpindex2.php

Comment 2

[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 3

[Jason Duell]

Hoping bagder can take this--same code that's causing bug 1344461.

Comment 4

[Daniel Stenberg [:bagder]]

I've submitted a patch for (the very similar) bug 1344461 that I expect will fix this issue as well.

Comment 6

[Chamal De Silva]

Attachment: httpindex3.php

Comment 7

[Chamal De Silva]

Attached a slightly modified test case which reproduces this bug. 
Please check whether httpindex3.php still reproduces this bug.

Comment 8

[Daniel Stenberg [:bagder]]

Thank you!

Comment 9

[Daniel Stenberg [:bagder]]

Can you double-check that you have this commit in the Firefox build you run: https://hg.mozilla.org/mozilla-central/rev/5ef67e15fd88 (originates from bug 1344461) I can't seem to reproduce any crashes using this recipe!

If you do have that commit in your version, can you please post an updated stack trace/crash signature ?

Comment 10

[Chamal De Silva]

I checked again and commit was present.
Test case does not crash Firefox. But Valgrind shows "uninitialised value" error.
Valgrind shows this error only when "Enable multi-process Nightly" option is disabled.
But I don't think is bug is related to "Multi-process" option.

Valgrind output
---------------
==4340== Use of uninitialised value of size 8
==4340==    at 0x100DE016: nsDirIndexParser::ParseData(nsIDirIndex*, char*, int) (nsDirIndexParser.cpp:266)
==4340==    by 0x100DE904: nsDirIndexParser::ProcessData(nsIRequest*, nsISupports*) [clone .part.53] [clone .constprop.110] (nsDirIndexParser.cpp:434)
==4340==    by 0x100DEBBE: ProcessData (nsDirIndexParser.cpp:382)
==4340==    by 0x100DEBBE: nsDirIndexParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (nsDirIndexParser.cpp:377)
==4340==    by 0xFED1D1B: mozilla::net::nsStreamListenerTee::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (nsStreamListenerTee.cpp:91)
==4340==    by 0x101FC0F2: mozilla::net::nsHttpChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (nsHttpChannel.cpp:7246)
.....

Comment on new test case (httpindex3.php)
-----------------------------------------
Bug fix (Below mentioned) correctly fixes initially attached test cases (httpindex1.php and httpindex2.php).
nsDirIndexParser::ParseFormat method ->
   mFormat[num] = -1;
+  mFormat[0] = -1; // to detect zero header fields

nsDirIndexParser::ParseData mehtod ->
+  if (mFormat[0] == -1) {
+    // no known header fields is an error
+    return NS_ERROR_ILLEGAL_VALUE;
+  }

But fix can be bypassed, when first format value is valid and following format values are invalid.
ex:
<?php
header("Content-Type:application/http-index-format");
echo "200:Filename a a\r\n";
echo "201:a a a\r\n";
?>

Comment 11

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1344467-clear-the-format-type-for-unknown-types-.patch

Thanks for the additional data.

The mFormat[] array that is filled in with identifier integers could end up with uninitialized array entries if a correct header field was followed by an incorrect one. This patch now sets all types to -1 first so that a mismatch will leave it initialized to the "unknown" type.

Comment 12

[Daniel Stenberg [:bagder]]

Comment on attachment 8850534 [details] [diff] [review]
0001-bug-1344467-clear-the-format-type-for-unknown-types-.patch

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

Hard. It is easy to cause an access of uninitialized memory, but hard to take advantage of.

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

Not very clearly, no

Which older supported branches are affected by this flaw?

This bug has been around in all code 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?

The same patch should work on all branches

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

Low risk due to the small size and minimal logical change.

Comment 13

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

sec-approval+ for trunk.
Please nominate patches (this patch?) for Aurora, Beta, ESR52, and ESR45.

Comment 14

[Daniel Stenberg [:bagder]]

Comment on attachment 8850534 [details] [diff] [review]
0001-bug-1344467-clear-the-format-type-for-unknown-types-.patch

Approval Request Comment
[Feature/Bug causing the regression]: 1344467
[User impact if declined]: erratic behavior due to use of uninitialized memory, a security risk
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: bug includes STR
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: miniscule patch
[String changes made/needed]: none

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Carsten Book [:Tomcat]]

backed out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1e98eaf0f70f7ce12244ac5bf9293048b507bb23&filter-searchStr=Linux+opt+Mochitests+executed+by+TaskCluster+test-linux32%2Fopt-mochitest-browser-chrome-4+tc-M(bc4) it seems that one of this 3 checkins cause times like https://treeherder.mozilla.org/logviewer.html#?job_id=86621316&repo=mozilla-inbound and seems the tests fail as example on connecting -> https://public-artifacts.taskcluster.net/B3lwHCOzQTe1j9McYz1LLg/0/public/test_info//mozilla-test-fail-screenshot_DRpsBk.png

Comment 17

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-bug-1344467-clear-the-format-type-for-unknown-typ.patch

Here's take 2. The previous version could still break out of the loop before the assignment and then cause reads of uninitialized data. I feel rather confident with this version.

There's try run going at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cbdc417d41a93eadd61c89d745c763b1a7b7fdd&selectedJob=87789473 which unfortunately has an unusually large amount of intermittents.

Comment 18

[Daniel Stenberg [:bagder]]

Attachment: v3-0001-bug-1344467-clear-the-format-type-for-unknown-typ.patch

Take 3.

Turns out fixing this bug triggered a different flow later on that caused test regressions. nsDirIndexParser::ParseData() really can't be allowed to return failure even for bad input as that then skips the necessary call to mListener->OnIndexAvailable().

This version of the patch makes changes ParseData() to still return on bad input, but return NS_OK.

Try run, with the changes visible as two different commits before I squashed them: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78085bfac800d91ea23963293ea5b256b34a5d83

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta/ESR52/ESR45 approval on this when you get a chance.

Comment 22

[Daniel Stenberg [:bagder]]

Comment on attachment 8853377 [details] [diff] [review]
v3-0001-bug-1344467-clear-the-format-type-for-unknown-typ.patch

Approval Request Comment
[Feature/Bug causing the regression]: 1344467
[User impact if declined]: security problem remains
[Is this code covered by automated tests?]: indirectly, yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: STR in bug
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: small patch in code that hasn't changed much over recent years
[String changes made/needed]: none

Comment 23

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

Comment on attachment 8853377 [details] [diff] [review]
v3-0001-bug-1344467-clear-the-format-type-for-unknown-typ.patch

Sec-high issue, let's aim this for the beta 10 build later this week, for aurora, and both ESRs.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a4fb01e49c9

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4f97cc13beb2

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d7d87adfe186
https://hg.mozilla.org/releases/mozilla-esr45/rev/38664f88d8f5