342016 - Malicious auth might be able to cause overflow

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Daniel Veditz [:dveditz]]

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)
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Comment 1

[Daniel Veditz [:dveditz]]

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

[Darin Fisher]

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

Comment 3

[Darin Fisher]

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?

Comment 4

[Daniel Veditz [:dveditz]]

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

Comment 5

[Darin Fisher]

-> reassign to default owner

Comment 6

[Josh Aas]

Patrick, can you evaluate this, possibly fix it? Report is pretty old, so hopefully not still an issue.

Comment 7

[Patrick McManus [:mcmanus]]

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

Comment 8

[Josh Aas]

Thanks Patrick.

Opening this up as it has been resolved and backported to 1.9.2 years ago.