Malicious auth might be able to cause overflow

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: dveditz, Assigned: mcmanus)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse])

(Reporter)

Description

11 years ago
From jackerror via email:
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

/source/extensions/negotiateauth/nsHttpNegotiateAuth.cpp#251	:

200 NS_IMETHODIMP
201 nsHttpNegotiateAuth::GenerateCredentials(nsIHttpChannel *httpChannel,
202                                          const char *challenge,
203                                          PRBool isProxyAuth,
204                                          const PRUnichar *domain,
205                                          const PRUnichar *username,
206                                          const PRUnichar *password,
207                                          nsISupports **sessionState,
208                                          nsISupports **continuationState,
209                                          char **creds)
210 {
211     // ChallengeReceived must have been called previously.
212     nsIAuthModule *module = (nsIAuthModule *) *continuationState;
213     NS_ENSURE_TRUE(module, NS_ERROR_NOT_INITIALIZED);
214 
215     LOG(("nsHttpNegotiateAuth::GenerateCredentials() [challenge=%s]\n", challenge));
216 
217     NS_ASSERTION(creds, "null param");
[...]
234     unsigned int len = strlen(challenge);
235 
236     void *inToken, *outToken;
237     PRUint32 inTokenLen, outTokenLen;
238 
239     if (len > kNegotiateLen) {
240         challenge += kNegotiateLen;
241         while (*challenge == ' ')
242             challenge++;
243         len = strlen(challenge);
244 
245         inTokenLen = (len * 3)/4;


.: if strlen(challenge) <= 1, inTokenLen will be null :.


246         inToken = malloc(inTokenLen);

.: thus we have a malloc(0) here :.

247         if (!inToken)
248             return (NS_ERROR_OUT_OF_MEMORY);
249 
250         // strip off any padding (see bug 230351)
251         while (challenge[len - 1] == '=')
252             len--;

.: Here if challenge [-1] == '=' (It is easily possible for an attacker to
   fill the heap to have such a character at challenge [-1]), len will
   become negative. This is dangerous because it is converted to an
   unsigned int in the call to PL_Base64Decode :.

257         if (PL_Base64Decode(challenge, len, (char *) inToken) == NULL) {
258             free(inToken);
259             return(NS_ERROR_UNEXPECTED);
260         }
270     nsresult rv = module->GetNextToken(inToken, inTokenLen, &outToken, &outTokenLen);


I didn't investigated further to know if this is remotelly triggerable.
(I think it is by sending specially crafted AUTH header from the HTTP server)
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
(Reporter)

Comment 1

11 years ago
mozilla/extensions/negotiateauth was moved to mozilla/extensions/auth, but the code in question appears unchanged apart from minor line-number drift. I don't see anything that prevents an empty challenge from getting this far.

Comment 2

11 years ago
We would only reach the code in question if the challenge string contained at least the text "Negotiate" (case-insensitive).

Comment 3

11 years ago
In which case, challenge[-1] would be a space or the character 'e'.  I don't see how it could be a "=" character.  Am I missing something?
(Reporter)

Updated

11 years ago
Whiteboard: [sg:investigate]
(Reporter)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 4

11 years ago
Reopening: the code may not be exploitable but it's still definitely wrong if it's possible to malloc(0) and challenge[-1]. Should bail out if len <= 1 (or equivalently, if len == 0 or inTokenLen == 0).
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Whiteboard: [sg:investigate] → [sg:nse]

Comment 5

10 years ago
-> reassign to default owner
Assignee: darin.moz → nobody
Status: REOPENED → NEW

Comment 6

5 years ago
Patrick, can you evaluate this, possibly fix it? Report is pretty old, so hopefully not still an issue.
Assignee: nobody → mcmanus
(Assignee)

Comment 7

5 years ago
I think there has been a change here in the last 18 months that makes this safe thanks to :jimm in bug 520607. The padding check has been moved up before the allocation.

the code now looks like:

  if (len > kNegotiateLen) {
        challenge += kNegotiateLen;
        while (*challenge == ' ')
            challenge++;
        len = strlen(challenge);

        // strip off any padding (see bug 230351)
        while (challenge[len - 1] == '=')
            len--;

        inTokenLen = (len * 3)/4;
        inToken = malloc(inTokenLen);
        if (!inToken)
            return (NS_ERROR_OUT_OF_MEMORY);

        //
        // Decode the response that followed the "Negotiate" token
        //
        if (PL_Base64Decode(challenge, len, (char *) inToken) == NULL) {
            free(inToken);
            return(NS_ERROR_UNEXPECTED);
        }

...

so even at len == 0, challenge[len] now deref's part of kNegotiateLen or the spacing after it which is safe enough instead of the newly allocated shorter buffer.. in the case of len {0,1} we do malloc(0) but that is not used anywhere except inside base64 decode which does not produce any output with those length. in the case of len 2 we do malloc(1) and in the case of len 3 we do malloc(2), in both those cases decode outputs 1 and 2 chars respectively - so ts ok. http://mxr.mozilla.org/mozilla-central/source/nsprpub/lib/libc/src/base64.c#324
Status: NEW → RESOLVED
Last Resolved: 11 years ago5 years ago
Resolution: --- → FIXED

Comment 8

5 years ago
Thanks Patrick.

Opening this up as it has been resolved and backported to 1.9.2 years ago.
Group: core-security
You need to log in before you can comment on or make changes to this bug.