Closed Bug 1338876 (CVE-2017-5418) Opened 7 years ago Closed 7 years ago

Out of bound access in nsHttpDigestAuth::ParseChallenge


(Core :: Networking, defect)

51 Branch
Not set



Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 + fixed
firefox-esr52 52+ fixed
firefox53 + fixed
firefox54 + fixed


(Reporter: chamal.desilva, Assigned: dragana)


(Keywords: csectype-bounds, sec-low, Whiteboard: [post-critsmash-triage][necko-active][adv-main52+])


(3 files)

Attached file 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
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

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)

Expected results:

Should not have read out of bound value.
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.
OS: Unspecified → All
Hardware: Unspecified → All
Attachment #8836446 - Attachment mime type: application/x-php → text/plain
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: nobody → dd.mozilla
Ever confirmed: true
Flags: needinfo?(dd.mozilla)
Whiteboard: [necko-active]
Attachment #8837221 - Flags: review?(mcmanus)
Attachment #8837221 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
Given that this goes back basically forever, this needs a security rating (or sec-approval if applicable) before it can land.
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.
Comment on attachment 8837221 [details] [diff] [review]

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?
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]

As a sec-low, this doesn't need sec-approval to be checked into trunk. Clearing.
Attachment #8837221 - Flags: sec-approval?
Attachment #8837221 - Flags: approval-mozilla-release?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8837221 [details] [diff] [review]

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+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> Can we land an automated test for this too?

I will add one today.
Flags: needinfo?(dd.mozilla)
Attachment #8838575 - Flags: review?(mcmanus)
Comment on attachment 8838575 [details] [diff] [review]

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

Attachment #8838575 - Flags: review?(mcmanus) → review+
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.