Closed Bug 1344305 Opened 3 years ago Closed 3 years ago

Write beyond bounds caused by nsHttpNTLMAuth::GenerateCredentials()

Categories

(Core :: Networking: HTTP, defect)

51 Branch
x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 53+ wontfix
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: q1, Assigned: erahm)

Details

(Keywords: sec-high, Whiteboard: [adv-main53+][adv-esr52.1+])

Attachments

(6 files, 1 obsolete file)

Attached file bug_701_poc_1.cpp
nsHttpNTLMAuth::GenerateCredentials() (netwerk\protocol\http\nsHttpNTLMAuth.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).

This bug is almost identical to https://bugzilla.mozilla.org/show_bug.cgi?id=1344081 , except that this bug affect NTLM instead of Negotiate.

The attack can be waged only if the attacking server is listed in the preference |network.automatic-ntlm-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; 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 484, which doesn't check for overflow:

360: NS_IMETHODIMP
361: nsHttpNTLMAuth::GenerateCredentials(nsIHttpAuthenticableChannel *authChannel,
362:                                     const char      *challenge,
363:                                     bool             isProxyAuth,
364:                                     const char16_t *domain,
365:                                     const char16_t *user,
366:                                     const char16_t *pass,
367:                                     nsISupports    **sessionState,
368:                                     nsISupports    **continuationState,
369:                                     uint32_t       *aFlags,
370:                                     char           **creds)
371: 
372: {
373:     LOG(("nsHttpNTLMAuth::GenerateCredentials\n"));
374: 
375:     *creds = nullptr;
376:     *aFlags = 0;
377: 
378:     // if user or password is empty, ChallengeReceived returned
379:     // identityInvalid = false, that means we are using default user
380:     // credentials; see  nsAuthSSPI::Init method for explanation of this
381:     // condition
382:     if (!user || !pass)
383:         *aFlags = USING_INTERNAL_IDENTITY;
384: 
385:     nsresult rv;
386:     nsCOMPtr<nsIAuthModule> module = do_QueryInterface(*continuationState, &rv);
387:     NS_ENSURE_SUCCESS(rv, rv);
388: 
389:     void *inBuf, *outBuf;
390:     uint32_t inBufLen, outBufLen;
391: 
392:     // initial challenge
393:     if (PL_strcasecmp(challenge, "NTLM") == 0) {
...
469:     }
470:     else {
471:         // decode challenge; skip past "NTLM " to the start of the base64
472:         // encoded data.
473:         int len = strlen(challenge);
474:         if (len < 6)
475:             return NS_ERROR_UNEXPECTED; // bogus challenge
476:         challenge += 5;
477:         len -= 5;
478: 
479:         // strip off any padding (see bug 230351)
480:         while (challenge[len - 1] == '=')
481:           len--;
482: 
483:         // decode into the input secbuffer
484:         inBufLen = (len * 3)/4;      // sufficient size (see plbase64.h)
485:         inBuf = moz_xmalloc(inBufLen);
486:         if (!inBuf)
487:             return NS_ERROR_OUT_OF_MEMORY;
488: 
489:         if (PL_Base64Decode(challenge, len, (char *) inBuf) == nullptr) {
490:             free(inBuf);
491:             return NS_ERROR_UNEXPECTED; // improper base64 encoding
492:         }
493:     }


See https://bugzilla.mozilla.org/show_bug.cgi?id=1344081 for a detailed explanation that applies to this bug as well.

Attached is a POC that can be used in the same manner as the one attached to https://bugzilla.mozilla.org/show_bug.cgi?id=1344081 , with obvious slight alterations on where to set the BP.

Also per the other bug, I have manifested this bug only on an x64 build, though it seems likely that it could be manifested on an x86 build running on an x64 platform.
Keywords: sec-high
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
Flags: sec-bounty?
MozReview-Commit-ID: DK6yx4PAYzi
Attachment #8846762 - Flags: review?(jduell.mcbugs)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Hmm, I just noticed the following suspicious code that almost-immediately follows the bad code I reported in comment 0:

    rv = module->GetNextToken(inBuf, inBufLen, &outBuf, &outBufLen);
    if (NS_SUCCEEDED(rv)) {
        // base64 encode data in output buffer and prepend "NTLM "
        int credsLen = 5 + ((outBufLen + 2)/3)*4;
        *creds = (char *) moz_xmalloc(credsLen + 1);
        if (!*creds)
            rv = NS_ERROR_OUT_OF_MEMORY;
        else {
            memcpy(*creds, "NTLM ", 5);
            PL_Base64Encode((char *) outBuf, outBufLen, *creds + 5);
            (*creds)[credsLen] = '\0'; // null terminate
        }
        // OK, we are done with |outBuf|
        free(outBuf);
    }

The calculation for |credsLen| could overflow if anyone ever changes |nsCString| to allow lengths >= ~0xc0000000. Maybe this should be fixed while you're here?
Attachment #8846762 - Flags: review?(jduell.mcbugs) → review+
(In reply to q1 from comment #3)
> The calculation for |credsLen| could overflow if anyone ever changes
> |nsCString| to allow lengths >= ~0xc0000000. Maybe this should be fixed
> while you're here?

Good point, I'll add an additional patch.
We want to append to an existing buffer so |Base64Encode| isn't going to work.
Instead I just switched over to CheckedInt.
Attachment #8846800 - Flags: review?(jduell.mcbugs)
Comment on attachment 8846800 [details] [diff] [review]
Part 2: Update size calculation for base64 encode in nsHttpNTLMAuth::GenerateCredentials

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

::: netwerk/protocol/http/nsHttpNTLMAuth.cpp
@@ -389,5 @@
>      rv = module->GetNextToken(inBuf, inBufLen, &outBuf, &outBufLen);
>      if (NS_SUCCEEDED(rv)) {
>          // base64 encode data in output buffer and prepend "NTLM "
> -        int credsLen = 5 + ((outBufLen + 2)/3)*4;
> -        *creds = (char *) moz_xmalloc(credsLen + 1);

|moz_xmalloc| is infallible, we don't need to check the result.
Attachment #8846800 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 8846762 [details] [diff] [review]
Switch to Base64Decode in nsHttpNTLMAuth::GenerateCredentials

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Not super easy, it looks like we're updating to a new base64 interface.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

> Which older supported branches are affected by this flaw?

All.

> If not all supported branches, which bug introduced the flaw?

N/A

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This is old code, the patches should backport fine.

> How likely is this patch to cause regressions; how much testing does it need?

Low, the base64 code is used extensively.
Attachment #8846762 - Flags: sec-approval?
Comment on attachment 8846800 [details] [diff] [review]
Part 2: Update size calculation for base64 encode in nsHttpNTLMAuth::GenerateCredentials

See comment 7.
Attachment #8846800 - Flags: sec-approval?
sec-approval+ for trunk.
We'll want branch patches for affected branches, including ESR ones.
Attachment #8846762 - Flags: sec-approval? → sec-approval+
Attachment #8846800 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcc6d071833b2b41e4167734bb4fda585875bb48
Bug 1344305 - Switch to Base64Decode in nsHttpNTLMAuth::GenerateCredentials. r=jduell

https://hg.mozilla.org/integration/mozilla-inbound/rev/828637ec7e52364bbc30b07e07de1ab5c242a0d5
Bug 1344305 - Part 2: Update size calculation for base64 encode in nsHttpNTLMAuth::GenerateCredentials. r=jduell
https://hg.mozilla.org/mozilla-central/rev/dcc6d071833b
https://hg.mozilla.org/mozilla-central/rev/828637ec7e52
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: network-core-security → core-security-release
The bounty committee has decided this bug is a specific instance of bug 1344380 and are minusing this bug in favor of that more serious one.
Flags: sec-bounty? → sec-bounty-
Attachment #8846762 - Attachment is obsolete: true
Attachment #8846762 - Attachment is obsolete: false
Attachment #8849320 - Attachment description: Cleanup base64 usage in nsHttpNTLMAuth::GenerateCredentials → Cleanup base64 usage in nsHttpNTLMAuth::GenerateCredentials for aurora
Attachment #8849320 - Attachment filename: bug_1344305 → bug_1344305_aurora
Comment on attachment 8849320 [details] [diff] [review]
Cleanup base64 usage in nsHttpNTLMAuth::GenerateCredentials for aurora

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

N/A, this is old code.

> [User impact if declined]:

Possible OOB write.

> [Is this code covered by automated tests?]:

Yes.

> [Has the fix been verified in Nightly?]:

Yes.

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

No.

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

N/A

> [Is the change risky?]:

Low risk.

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

Minor change, we use a more modern base64 interface that checks for overflow.

> [String changes made/needed]:

N/A
Attachment #8849320 - Flags: approval-mozilla-aurora?
Comment on attachment 8849325 [details] [diff] [review]
Cleanup base64 usage in nsHttpNTLMAuth::GenerateCredentials for beta

> Approval Request Comment

See comment 17.
Attachment #8849325 - Flags: approval-mozilla-beta?
Comment on attachment 8849337 [details] [diff] [review]
Cleanup base64 usage in nsHttpNTLMAuth::GenerateCredentials for esr52

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

This is sec-high.
Attachment #8849337 - Flags: approval-mozilla-esr52?
Comment on attachment 8849338 [details] [diff] [review]
Cleanup base64 usage in nsHttpNTLMAuth::GenerateCredentials for esr45

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

This is sec-crit.
Attachment #8849338 - Flags: approval-mozilla-esr45?
(In reply to Eric Rahm [:erahm] from comment #20)
> Comment on attachment 8849338 [details] [diff] [review]
> Cleanup base64 usage in nsHttpNTLMAuth::GenerateCredentials for esr45
> 
> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR
> consideration:
> 
> This is sec-crit.

(sorry sec-high)
Comment on attachment 8849320 [details] [diff] [review]
Cleanup base64 usage in nsHttpNTLMAuth::GenerateCredentials for aurora

Fix a sec-high. Aurora54+ & Beta53+.
Attachment #8849320 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8849325 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Setting qe-verify- based on Eric's assessment on manual testing needs (see Comment 17) and the fact that this fix has automated coverage.
Flags: qe-verify-
Comment on attachment 8849337 [details] [diff] [review]
Cleanup base64 usage in nsHttpNTLMAuth::GenerateCredentials for esr52

base64 fix for 52.1.0esr
Attachment #8849337 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment on attachment 8849338 [details] [diff] [review]
Cleanup base64 usage in nsHttpNTLMAuth::GenerateCredentials for esr45

... and for 45.9.0esr
Attachment #8849338 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Backed out from ESR45 for bustage. Marking ESR45 as wontfix per IRC discussion with erahm.
https://hg.mozilla.org/releases/mozilla-esr45/rev/62a639250c76bfa93d88d18f8270808fe337b847
Attachment #8849338 - Attachment is obsolete: true
Whiteboard: [adv-main53+][adv-esr52.1+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.