Write beyond bounds caused by nsHttpNegotiateAuth::GenerateCredentials()

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: q1, Assigned: erahm)

Tracking

({sec-moderate})

51 Branch
mozilla55
x86_64
All
Points:
---
Bug Flags:
sec-bounty -
qe-verify ?

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main53+][adv-esr52.1+][post-critsmash-triage])

Attachments

(3 attachments)

Posted file bug_699_poc_1.cpp
nsHttpNegotiateAuth::GenerateCredentials() (extensions\auth\nsHttpNegotiateAuth.cpp) can cause an integer overflow. When it does, it decodes attacker-provided Base64 data into binary form and writes it beyond the end of a heap object. The attacker can control both what data is written and how much (with 3-byte granularity).

The attack can be waged only if the attacking server is listed in the preference |network.negotiate-auth.trusted-uris|. However (1) if the server is listed without the "https" prefix, an attacker could poison the relevant DNS to redirect FF to her own server [1]; and (2) a "trusted" server could still be corrupted, and could therefore use this bug to compromise any computers using FF to login to it.

The bug is in line 524, which doesn't check for overflow:

472: NS_IMETHODIMP
473: nsHttpNegotiateAuth::GenerateCredentials(nsIHttpAuthenticableChannel *authChannel,
474:                                          const char *challenge,
475:                                          bool isProxyAuth,
476:                                          const char16_t *domain,
477:                                          const char16_t *username,
478:                                          const char16_t *password,
479:                                          nsISupports **sessionState,
480:                                          nsISupports **continuationState,
481:                                          uint32_t *flags,
482:                                          char **creds)
483: {
...
509:     unsigned int len = strlen(challenge);
510: 
511:     void *inToken, *outToken;
512:     uint32_t inTokenLen, outTokenLen;
513: 
514:     if (len > kNegotiateLen) {
515:         challenge += kNegotiateLen;
516:         while (*challenge == ' ')
517:             challenge++;
518:         len = strlen(challenge);
519: 
520:         // strip off any padding (see bug 230351)
521:         while (challenge[len - 1] == '=')
522:             len--;
523: 
524:         inTokenLen = (len * 3)/4;
525:         inToken = malloc(inTokenLen);
526:         if (!inToken)
527:             return (NS_ERROR_OUT_OF_MEMORY);
528: 
529:         //
530:         // Decode the response that followed the "Negotiate" token
531:         //
532:         if (PL_Base64Decode(challenge, len, (char *) inToken) == nullptr) {
533:             free(inToken);
534:             return(NS_ERROR_UNEXPECTED);
535:         }
536:     }

The code then calls PL_Base64Decode(), which overruns |inToken|.

Attached is a POC that acts as an attacking webserver. Build it in VS 2015 and run it. Then start FF 51.0.1, add the webserver's URL to |network.negotiate-auth.trusted-uris|, attach the debugger to FF, and set a BP on GenerateCredentials (). Connect FF to the "webserver" on port 27015. After some seconds, the BP will trigger. Step to line 524 and notice that the overflow causes |len| to become |0|. Examine the memory at |inToken| and step through line 532, and notice how simulated attack code and data is decoded into the memory beyond |inToken|'s end.

The POC works by sending a challenge string that is long enough to cause line 524 to overflow, but whose initial section contains Base64-encoded attack code and data followed by an illegal Base64 character (\xff). The illegal character causes PL_Base64Decode () to stop decoding, which limits the amount of data written into the heap to a quantity determined -- with 3 byte granularity -- by the attacker.

BTW, there is a similar bug in nsHttpNTLMAuth::GenerateCredentials(), which I will file separately.

[1] For this reason, it is arguably a bug that FF doesn't at least warn the user of an attempt to connect to a non-SSL server. I will file this bug separately.
Flags: sec-bounty?
I have manifested this bug only on an x64 build. It might be possible to manifest on an x86 build if run on an x64 OS (so that the user space is large enough to accommodate the challenge string of length ~ 0x55555560 bytes).
OS: Unspecified → All
Hardware: Unspecified → x86_64
(In reply to q1 from comment #0)
> BTW, there is a similar bug in nsHttpNTLMAuth::GenerateCredentials(), which
> I will file separately.

https://bugzilla.mozilla.org/show_bug.cgi?id=1344305
(In reply to q1 from comment #0)

> [1] For this reason, it is arguably a bug that FF doesn't at least warn the
> user of an attempt to connect to a non-SSL server. I will file this bug
> separately.

https://bugzilla.mozilla.org/show_bug.cgi?id=1344317
Calling this sec-moderate because most users don't have a setting for that pref, and if they did it's probably a trusted work server.
Keywords: sec-moderate
Similar to bug 1344305, we could use |mozilla::Base64Decode| [1] instead which checks for overflow.

[1] http://searchfox.org/mozilla-central/rev/7cb75d87753de9103253e34bc85592e26378f506/xpcom/io/Base64.h#34-36
Group: core-security → network-core-security
MozReview-Commit-ID: 4A5tEV5Fb8
Attachment #8846757 - Flags: review?(jduell.mcbugs)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Attachment #8846757 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/feacb48b3361a027c1fbc3c40983a46095c4dda6
Bug 1344081 - Switch to Base64Decode in nsHttpNegotiateAuth::GenerateCredentials. r=jduell
https://hg.mozilla.org/mozilla-central/rev/feacb48b3361
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I'd appreciate it if the NSS team would get informed about bugs in NSS.
Group: network-core-security → core-security-release
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #9)
> I'd appreciate it if the NSS team would get informed about bugs in NSS.

Didn't you? Ron filed bug 1344380 on the NSS Base64 problem soon after this one.
The bounty committee has decided this bug is a specific instance of bug 1344380 (even though it was patched elsewhere) and are minusing this bug in favor of that more serious one.
Flags: sec-bounty? → sec-bounty-
Doesn't seem worth backporting all the way to ESR45 (happy to be told otherwise if you feel differently, though!), but maybe we should at least do so for Aurora/Beta/ESR52?
Comment on attachment 8846757 [details] [diff] [review]
Switch to Base64Decode in nsHttpNegotiateAuth::GenerateCredentials

Approval Request Comment
[Feature/Bug causing the regression]:

This is old code.

[User impact if declined]:

Possible OOB write from user provided buffer.

[Is this code covered by automated tests?]:

Probably not specifically, a pref needs to be set.

[Has the fix been verified in Nightly?]:

Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 

Yes, see comment 0.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

Low risk.

[Why is the change risky/not risky?]:

Just updates to a more modern base64 decoder.

[String changes made/needed]:

N/A
Flags: needinfo?(erahm)
Attachment #8846757 - Flags: approval-mozilla-aurora?
Comment on attachment 8850129 [details] [diff] [review]
Switch to Base64Decode in nsHttpNegotiateAuth::GenerateCredentials for beta

See comment 13.
Attachment #8850129 - Flags: approval-mozilla-beta?
Comment on attachment 8850129 [details] [diff] [review]
Switch to Base64Decode in nsHttpNegotiateAuth::GenerateCredentials for beta

> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
> User impact if declined: 

Possible OOB write.

> Fix Landed on Version:

55.

> Risk to taking this patch (and alternatives if risky): 

Low risk.

> String or UUID changes made by this patch: 

N/A
Attachment #8850129 - Flags: approval-mozilla-esr52?
Track 53+/54+/55+ as security issue.
Comment on attachment 8846757 [details] [diff] [review]
Switch to Base64Decode in nsHttpNegotiateAuth::GenerateCredentials

Fix a security issue. Aurora54+ & Beta53+.
Attachment #8846757 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8850129 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Per Comment 13, it seems that manual testing might be of value here, but based on the PoC and the information available in Comment 0, I don't see what steps Manual QA is supposed to be following to reproduce and verify this bug. 

Eric, could you please elaborate?
Flags: qe-verify?
Flags: needinfo?(erahm)
Comment on attachment 8850129 [details] [diff] [review]
Switch to Base64Decode in nsHttpNegotiateAuth::GenerateCredentials for beta

base64 fix for esr52
Attachment #8850129 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #21)
> Eric, could you please elaborate?

From comment #0:

> Attached is a POC that acts as an attacking webserver. Build it in VS 2015
> and run it. Then start FF 51.0.1, add the webserver's URL to
> |network.negotiate-auth.trusted-uris|, attach the debugger to FF, and set a
> BP on GenerateCredentials (). Connect FF to the "webserver" on port 27015.
> After some seconds, the BP will trigger. Step to line 524 and notice that
> the overflow causes |len| to become |0|. Examine the memory at |inToken| and
> step through line 532, and notice how simulated attack code and data is
> decoded into the memory beyond |inToken|'s end.

This is somewhat more manual than usual.
Flags: needinfo?(erahm)
Whiteboard: [adv-main53+][adv-esr52.1+]
(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #21)
> Per Comment 13, it seems that manual testing might be of value here, but
> based on the PoC and the information available in Comment 0, I don't see
> what steps Manual QA is supposed to be following to reproduce and verify
> this bug. 
> 


The moderate severity of this bug probably does not merit the time it would take to test. Up to you, Andrei.
Whiteboard: [adv-main53+][adv-esr52.1+] → [adv-main53+][adv-esr52.1+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.