132 bytes, text/plain
1.01 KB, patch
|Details | Diff | Splinter Review|
5.13 KB, patch
|Details | Diff | Splinter Review|
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.
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.
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
Product: Firefox → Core
Assignee: nobody → dd.mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Created attachment 8837221 [details] [diff] [review] bug_1338876.patch
Attachment #8837221 - Flags: review?(mcmanus)
Attachment #8837221 - Flags: review?(mcmanus) → review+
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: --- → ?
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] 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.
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.
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
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.
status-firefox-esr45: affected → wontfix
tracking-firefox51: ? → ---
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+
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: --- → ?
(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.
status-firefox-esr52: affected → fixed
Created attachment 8838575 [details] [diff] [review] bug_1338876_test.patch
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+
Check in needed for the test: bug_1338876_test.patch
Flags: in-testsuite? → in-testsuite+
Whiteboard: [necko-active] → [post-critsmash-triage][necko-active]
Whiteboard: [post-critsmash-triage][necko-active] → [post-critsmash-triage][necko-active][adv-main52+]
You need to log in before you can comment on or make changes to this bug.