1338876 - (CVE-2017-5418) Out of bound access in nsHttpDigestAuth::ParseChallenge

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Chamal De Silva]

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

Comment 1

[Chamal De Silva]

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.

Comment 2

[:Gijs (he/him)]

Chamal: thanks for the clear testcase and report.

Dragana, can you or someone else on the networking team look at this? Thanks!

Comment 3

[Dragana Damjanovic [:dragana]]

Attachment: bug_1338876.patch

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Andrew McCreight [:mccr8]]

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 6

[Dragana Damjanovic [:dragana]]

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.

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Daniel Veditz [: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).

Comment 9

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

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36663347138d

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36663347138d

Comment 12

[Julien Cristau [:jcristau]]

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

fix info leak in digest auth, aurora53+, beta52+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 14

[Dragana Damjanovic [:dragana]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/1c01e2aa7bf0

Comment 16

[Dragana Damjanovic [:dragana]]

Attachment: bug_1338876_test.patch

Comment 17

[Patrick McManus [:mcmanus]]

Comment on attachment 8838575 [details] [diff] [review]
bug_1338876_test.patch

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

thanks

Comment 18

[Dragana Damjanovic [:dragana]]

Check in needed for the test: bug_1338876_test.patch

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/40a559d547319034db251ca2a98dc0a8004ad592

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/40a559d54731