Last Comment Bug 342016 - Malicious auth might be able to cause overflow
: Malicious auth might be able to cause overflow
Status: RESOLVED FIXED
[sg:nse]
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Patrick McManus [:mcmanus]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-19 07:02 PDT by Daniel Veditz [:dveditz]
Modified: 2012-03-06 07:06 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Daniel Veditz [:dveditz] 2006-06-19 07:02:24 PDT
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] 2006-06-19 08:14:14 PDT
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 2006-06-19 15:32:46 PDT
We would only reach the code in question if the challenge string contained at least the text "Negotiate" (case-insensitive).
Comment 3 Darin Fisher 2006-06-19 15:36:03 PDT
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] 2006-06-26 15:40:59 PDT
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 2007-06-11 00:35:01 PDT
-> reassign to default owner
Comment 6 Josh Aas 2012-03-06 05:13:15 PST
Patrick, can you evaluate this, possibly fix it? Report is pretty old, so hopefully not still an issue.
Comment 7 Patrick McManus [:mcmanus] 2012-03-06 06:32:22 PST
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 2012-03-06 07:06:35 PST
Thanks Patrick.

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

Note You need to log in before you can comment on or make changes to this bug.