Bug 1338876 (CVE-2017-5418)

Out of bound access in nsHttpDigestAuth::ParseChallenge

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: chamal.desilva, Assigned: dragana)

Tracking

({csectype-bounds, sec-low})

51 Branch
mozilla54
csectype-bounds, sec-low
Points:
---
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox51 wontfix, firefox52+ fixed, firefox-esr5252+ fixed, firefox53+ fixed, firefox54+ fixed)

Details

(Whiteboard: [post-critsmash-triage][necko-active][adv-main52+])

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8836446 [details]
auth.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 valgrind using instructions mentioned in
   https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Valgrind
2. Download and copy auth.php to local web server.
3. Start firefox
   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 http://127.0.0.1/auth.php


Actual results:

Valgrind reports this error.

==3063== Conditional jump or move depends on uninitialised value(s)
==3063==    at 0x101B8F28: mozilla::net::nsHttpDigestAuth::ParseChallenge(char const*, nsACString_internal&, nsACString_internal&, nsACString_internal&, nsACString_internal&, bool*, unsigned short*, unsigned short*) [clone .part.144] (nsHttpDigestAuth.cpp:577)
==3063==    by 0x101B958E: ParseChallenge (nsHttpDigestAuth.cpp:163)
==3063==    by 0x101B958E: mozilla::net::nsHttpDigestAuth::ChallengeReceived(nsIHttpAuthenticableChannel*, char const*, bool, nsISupports**, nsISupports**, bool*) (nsHttpDigestAuth.cpp:152)
==3063==    by 0x1016B6B9: mozilla::net::nsHttpChannelAuthProvider::GetCredentialsForChallenge(char const*, char const*, bool, nsIHttpAuthenticator*, nsCString&) (nsHttpChannelAuthProvider.cpp:790)
==3063==    by 0x1016C3DE: mozilla::net::nsHttpChannelAuthProvider::GetCredentials(char const*, bool, nsCString&) (nsHttpChannelAuthProvider.cpp:618)
==3063==    by 0x1016C72F: mozilla::net::nsHttpChannelAuthProvider::ProcessAuthentication(unsigned int, bool) (nsHttpChannelAuthProvider.cpp:198)
==3063==    by 0x101F01C5: mozilla::net::nsHttpChannel::ContinueProcessResponse2(nsresult) [clone .part.419] [clone .constprop.487] (nsHttpChannel.cpp:2209)
==3063==    by 0x101F091F: ContinueProcessResponse2 (nsCOMPtr.h:294)
==3063==    by 0x101F091F: mozilla::net::nsHttpChannel::ContinueProcessResponse1() (nsHttpChannel.cpp:2089)
==3063==    by 0x101F0B6E: mozilla::net::nsHttpChannel::ProcessResponse() (nsHttpChannel.cpp:2009)
==3063==    by 0x101F13A5: mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) (nsHttpChannel.cpp:6523)
==3063==    by 0xFEA068D: nsInputStreamPump::OnStateStart() (nsInputStreamPump.cpp:524)
==3063==    by 0xFEA118F: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:426)
==3063==    by 0xFDFB013: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:96)
==3063== 


Expected results:

Should not have read out of bound value.
(Reporter)

Comment 1

2 years ago
Cause of bug
------------

Bug is in below mentioned line of nsHttpDigestAuth::ParseChallenge method in netwerk\protocol\http\nsHttpDigestAuth.cpp file.

const char *p = challenge + 7;

This method assumes it will received a value that starts with "Digest " for challenge variable, which has at least 7 characters.
But that assumption fails when server sends below header.

header ('WWW-Authenticate:Digest'); //Note no space after "Digest".

So adding 7 to challenge variable overflows.
(Reporter)

Updated

2 years ago
OS: Unspecified → All
Hardware: Unspecified → All

Updated

2 years ago
Attachment #8836446 - Attachment mime type: application/x-php → text/plain

Comment 2

2 years ago
Chamal: thanks for the clear testcase and report.

Dragana, can you or someone else on the networking team look at this? Thanks!
Group: firefox-core-security → core-security
Component: Untriaged → Networking
Flags: needinfo?(dd.mozilla)
Product: Firefox → Core
(Assignee)

Updated

2 years ago
Assignee: nobody → dd.mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(dd.mozilla)
Whiteboard: [necko-active]
(Assignee)

Comment 3

2 years ago
Created attachment 8837221 [details] [diff] [review]
bug_1338876.patch
Attachment #8837221 - Flags: review?(mcmanus)
Attachment #8837221 - Flags: review?(mcmanus) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Given that this goes back basically forever, this needs a security rating (or sec-approval if applicable) before it can land.
https://wiki.mozilla.org/Security/Bug_Approval_Process
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr45: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
tracking-firefox-esr45: --- → ?
tracking-firefox-esr52: --- → ?
Keywords: checkin-needed
Keywords: csectype-bounds
I'm not sure what the rating on this should be. It looks like maybe the code ends up skipping a null terminator and ends up reading a lot of random junk? So then the main danger is information leakage.
(Assignee)

Comment 6

2 years ago
Comment on attachment 8837221 [details] [diff] [review]
bug_1338876.patch

Asking for uplift, for release too.


Approval Request Comment
[Feature/Bug causing the regression]: this code is really old.
[User impact if declined]:security impact, information leakage as explained above.
[Is this code covered by automated tests?]:yes and no. The code path is but this special case not.
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: There are steps to reproduce 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 really small. This code path will be call only if the challenge starts with 'digest' so setting pointer to challenge + 6 should always be correct. The following code removes white spaces if any present. So the behaviour is the same as before the patch.
[String changes made/needed]:none

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch shows what the problem is.
We move a pointer after a null terminator and the following code will read anything that is after that position.

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

Which older supported branches are affected by this flaw?
all
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?
No risk, the code is really old, so it will apply to older branches without problems.
How likely is this patch to cause regressions; how much testing does it need?
unlikely, it will not cause regression, also i tested it manually.
Attachment #8837221 - Flags: sec-approval?
Attachment #8837221 - Flags: approval-mozilla-release?
Attachment #8837221 - Flags: approval-mozilla-beta?
Attachment #8837221 - Flags: approval-mozilla-aurora?
Dan, what do you think the rating on this should be? AFAICT the function is going to be searching through memory past the end of the array, but it only looks for specific substrings like "realm", and then copies stuff trailing that, so you can't really extract arbitrary information, as far as I can tell. I'm not sure if this information is passed directly back to content in any way.
Flags: needinfo?(dveditz)
The routine will bail out if the things it finds don't match all the expected keywords. If they do it looks like it will create a response and may send the values back to the server. It might match if you have an old challenge string in memory and get lucky enough to find it, but challenge strings don't contain private information since anyone can load the server and get prompted for auth (unless maybe the server itself is inaccessible to the attacker, such as on an intranet).
Group: core-security → network-core-security
Flags: needinfo?(dveditz)
Keywords: sec-low
Comment on attachment 8837221 [details] [diff] [review]
bug_1338876.patch

As a sec-low, this doesn't need sec-approval to be checked into trunk. Clearing.
Attachment #8837221 - Flags: sec-approval?
https://hg.mozilla.org/integration/mozilla-inbound/rev/36663347138d
status-firefox-esr45: affected → wontfix
tracking-firefox51: ? → ---
Attachment #8837221 - Flags: approval-mozilla-release?
https://hg.mozilla.org/mozilla-central/rev/36663347138d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
tracking-firefox52: ? → +
tracking-firefox53: ? → +
tracking-firefox54: ? → +
tracking-firefox-esr45: ? → ---
tracking-firefox-esr52: ? → ---
Comment on attachment 8837221 [details] [diff] [review]
bug_1338876.patch

fix info leak in digest auth, aurora53+, beta52+
Attachment #8837221 - Flags: approval-mozilla-beta?
Attachment #8837221 - Flags: approval-mozilla-beta+
Attachment #8837221 - Flags: approval-mozilla-aurora?
Attachment #8837221 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/197dde7656f8
https://hg.mozilla.org/releases/mozilla-beta/rev/1c01e2aa7bf0

Can we land an automated test for this too?
status-firefox52: affected → fixed
status-firefox53: affected → fixed
tracking-firefox-esr52: --- → ?
Flags: needinfo?(dd.mozilla)
Flags: in-testsuite?
tracking-firefox-esr52: ? → 52+
(Assignee)

Comment 14

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/197dde7656f8
> https://hg.mozilla.org/releases/mozilla-beta/rev/1c01e2aa7bf0
> 
> Can we land an automated test for this too?

I will add one today.
(Assignee)

Comment 16

2 years ago
Created attachment 8838575 [details] [diff] [review]
bug_1338876_test.patch
Flags: needinfo?(dd.mozilla)
Attachment #8838575 - Flags: review?(mcmanus)
Comment on attachment 8838575 [details] [diff] [review]
bug_1338876_test.patch

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

thanks
Attachment #8838575 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 18

2 years ago
Check in needed for the test: bug_1338876_test.patch
Keywords: checkin-needed
Group: network-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [necko-active] → [post-critsmash-triage][necko-active]
Whiteboard: [post-critsmash-triage][necko-active] → [post-critsmash-triage][necko-active][adv-main52+]
Alias: CVE-2017-5418
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.