Bug 1344467 (CVE-2017-5445)

Uninitialized value in nsDirIndexParser::ParseFormat

RESOLVED FIXED in Firefox -esr45

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: chamal.desilva, Assigned: bagder)

Tracking

({sec-moderate})

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

Firefox Tracking Flags

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

Details

(Whiteboard: [necko-active][adv-main53+][adv-esr52.1+][adv-esr45.9+], crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

2 years ago
Posted file httpindex2.php
(Reporter)

Updated

2 years ago
Group: firefox-core-security → core-security
Component: Untriaged → Networking
Product: Firefox → Core
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)
Keywords: sec-high
Flags: sec-bounty?
Group: core-security → network-core-security

Comment 3

2 years ago
Hoping bagder can take this--same code that's causing bug 1344461.
Flags: needinfo?(daniel)
(Assignee)

Updated

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

Comment 4

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

Updated

2 years ago
See Also: → bug 1344461
Whiteboard: [necko-active]
(Assignee)

Updated

2 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1344461
(Reporter)

Comment 6

2 years ago
Posted file httpindex3.php
(Reporter)

Comment 7

2 years ago
Attached a slightly modified test case which reproduces this bug. 
Please check whether httpindex3.php still reproduces this bug.
Flags: needinfo?(daniel)
(Assignee)

Comment 8

2 years ago
Thank you!
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(daniel)
Resolution: DUPLICATE → ---
(Assignee)

Comment 9

2 years ago
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 ?
Flags: needinfo?(chamalsl)
(Reporter)

Comment 10

2 years ago
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";
?>
Flags: needinfo?(chamalsl)
(Assignee)

Comment 11

2 years ago
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.
Attachment #8850534 - Flags: feedback?(chamalsl)
(Reporter)

Updated

2 years ago
Attachment #8850534 - Flags: feedback?(chamal.desilva) → feedback+
(Assignee)

Updated

2 years ago
Attachment #8850534 - Flags: review?(valentin.gosu)
Attachment #8850534 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 12

2 years ago
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.
Attachment #8850534 - Flags: sec-approval?
sec-approval+ for trunk.
Please nominate patches (this patch?) for Aurora, Beta, ESR52, and ESR45.
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+
Attachment #8850534 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 14

2 years ago
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
Attachment #8850534 - Flags: approval-mozilla-esr52?
Attachment #8850534 - Flags: approval-mozilla-esr45?
Attachment #8850534 - Flags: approval-mozilla-beta?
Attachment #8850534 - Flags: approval-mozilla-aurora?

Updated

2 years ago
Flags: needinfo?(jduell.mcbugs)
(Assignee)

Comment 17

2 years ago
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.
Attachment #8850534 - Attachment is obsolete: true
Attachment #8850534 - Flags: approval-mozilla-esr52?
Attachment #8850534 - Flags: approval-mozilla-esr45?
Attachment #8850534 - Flags: approval-mozilla-beta?
Attachment #8850534 - Flags: approval-mozilla-aurora?
Flags: needinfo?(daniel)
Attachment #8853311 - Flags: review+
(Assignee)

Comment 18

2 years ago
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
Attachment #8853311 - Attachment is obsolete: true
Attachment #8853377 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a3c28a335237ef6a890d973659996da7b6aecfdf
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please request Aurora/Beta/ESR52/ESR45 approval on this when you get a chance.
Flags: needinfo?(daniel)
(Assignee)

Comment 22

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