209116 - Digest authentication sometimes uses the wrong HTTP method to compute A2 for POST

Status: RESOLVED WORKSFORME

(Core :: Networking: HTTP, defect, P3)

Description

[Adam D. Bradley]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686) Gecko/20030521 Galeon/1.3.4 Debian/1.3.4.20030526-1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686) Gecko/20030521 Galeon/1.3.4 Debian/1.3.4.20030526-1

When performing a POST using digest authentication, it seems Mozilla uses GET
instead of POST when computing A2, causing an authentication failure (the server
responds with 401).  The correct method is used, however, after the user has
been prompted to re-enter their name/password.

Reproducible: Always

Steps to Reproduce:
1.Go to http://www.zealforyourhouse.com/authdemo.php and authenticate with user
name "demo", password "demo".
2.Click on the "Try POSTing" button.

Actual Results:  
The username/password dialog will pop up again.  Use the same username and
password, click "Ok", and you will get a "Success" page.

Expected Results:  
Should not have to pop up the username/password dialog after clicking "Try POSTing".

I have reproduced this bug in Galeon and Netscape 7.

Comment 1

[Darin Fisher]

this may already be fixed... please try a more recent nightly build, such as
today's build.  you can get it from:
 
  http://ftp.mozilla.org/pub/mozilla/nightly/latest-trunk

thx!

Comment 2

[Adam D. Bradley]

I just tried the currend i686/linux build:

    Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5a) Gecko/20030612

and it's traded one problem for another.  Now Mozilla is reusing an opaque/nonce
pair without incrementing the nonce-count value, which makes the second request
look like a replay attack (triggering a 401 error).

Comment 3

[Darin Fisher]

ok, hmm... that's bad.  taking for moz 1.5.  does the resulting 401 cause the
user to be prompted to re-enter their password, or does moz silently resend the
request with the new nonce?

Comment 4

[Adam D. Bradley]

If the server sets the "stale" flag in its response, then Mozilla automatically
tries again with the new nonce from the challenge.  If the server doesn't set
the "stale" flag, the dialog gets popped up.  Either way, nonce reuse (and
therefore pipelining of authenticated requests) is buggered.

Comment 5

[Suresh]

I couldn't duplicate this bug in a recent mozilla windows debug build. Something
got changed in the server?

Comment 6

[Adam D. Bradley]

Build 2003061208 is the latest I tried, and it still produces the problem. 
Could be already fixed in the more recent builds?  I'll give it a try...

Comment 7

[Adam D. Bradley]

Just tried build 2003070107.  The popup is suppressed, but the behavior is still
suboptimal (but not wrong per se) under the covers: Mozilla doesn't even try to
reuse the nonce from the GET, but instead just sends the POST without an
authorization header, gets back the 401, then re-tries the POST with the newly
constructed Authorization.  That's an unnecessary round-trip.

Comment 8

[Darin Fisher]

for POST requests i think the extra round trip is tolerable given that POST
requests are relatively infrequent... not that we shouldn't try to do better of
course ;-)

Comment 9

[Olivier Cahagne]

*** Bug 212848 has been marked as a duplicate of this bug. ***

Comment 10

[An-Cheng Huang]

Attachment: possible fix for the problem

Comment 11

[An-Cheng Huang]

After some testing, it turns out that mozilla does in fact generate the
authorization header for the POST request using the correct httpMethod
with the nonce from the GET.  However, the request gets rejected by the
server 401 because the nonce is stale (the nonce count is not incremented).
                                                                                
Attached is a possible one-liner fix (I've tested this with the website
provided by the reporter, and it works).  It removes the second
sessionStateGrip.swap(sessionState) in nsHttpChannel::GetCredentials().
I think this swap causes the mMetaData to be null in all newly created
auth cache entries.  In fact, I don't see the point of the first swap
there either (or, for that matter, the point of keeping the old value),
since sessionState is immediately assigned the value of sessionStateGrip
anyway.  Am I missing something here?

Comment 12

[Darin Fisher]

an-cheng: the swaps are important, but maybe they aren't being done right ;-)
remember also that NTLM has session state.

Comment 13

[Darin Fisher]

maybe entry->mMetaData is not always getting updated to the new value of the
session state??

Comment 14

[An-Cheng Huang]

Ok, here is what I think happens when sending a GET then a POST using digest
authentication:

// GET without Authorization sent
// 401 received
ProcessAuthentication()
    GetCredentials()
        GetAuthEntryForDomain()
        // entry == null
        // sessionStateGrip == null
        sessionState = sessionStateGrip
        // sessionStateGrip == null, sessionState == null
        DigestAuth::ChallengeReceived(..., &sessionState, ...)
            NS_IF_RELEASE(*sessionState)
        // sessionState == null
        sessionStateGrip.swap(sessionState)
        // sessionStateGrip == null, sessionState == null
        PromptForIdentity()
        sessionState = sessionStateGrip
        // sessionStateGrip == null, sessionState == null
        DigestAuth::GenerateCredentials(..., &sessionState, ...)
            // sessionState == null
            v->SetData(1);
            NS_ADDREF(*sessionState = v)
            // nonce_count is "00000001"
            // sessionState == X, value of sessionState is 1
        // sessionState == X
        sessionStateGrip.swap(sessionState)
        // sessionStateGrip == X, sessionState == null
        SetAuthEntry(..., sessionState)
        // !REUSABLE_CREDENTIALS, so creds in the entry is null
        // REUSABLE_CHALLENGE, so challenge is stored
        // metadata (sessionState) in the entry is null
                                                                                
...

// sending POST
AddAuthorizationHeaders()
    SetAuthorizationHeader()
        GetAuthEntryForPath()
        // entry != null
        // creds[0] == null, challenge != null
        sessionState = entry->mMetaData
        // sessionState == null
        DigestAuth::GenerateCredentials(..., &sessionState, ...)
            // sessionState == null
            v->SetData(1);
            NS_ADDREF(*sessionState = v)
            // nonce_count is "00000001"
            // sessionState == Y, value of sessionState is 1
        // sessionState == Y
        entry->mMetaData.swap(sessionState)
        // entry->mMetaData == Y, sessionState == null

Therefore, when POST is sent the nonce_count in the Authorization is 1,
so if the server checks the nonce_count, the request is rejected with
stale=true, which is what happens here.

If this is correct, then when a new cache entry is created in GetCredentials(),
the metadata is always null, i.e., the metadata (sessionState) returned by
GenerateCredentials() is discarded.  The metadata is only populated when
sending the next request, i.e., when the code path goes through
SetAuthorizationHeader().  In fact, although I don't know much about
NTLM auth, it would seem that this also applies to NTLM (i.e., if there is
no previous cache entry, then sessionStateGrip is null before the second
swap, and sessionState is null after the swap, so the metadata in the
newly created entry will also be null).
                                                                                
The reason that I removed the second swap in GetCredentials() is that
after the swap, sessionStateGrip is not used at all, while sessionState
is only used for the SetAuthEntry() call at the end.  So it seems reasonable
to use the metadata returned (created or updated) by GenerateCredentials()
(i.e., sessionState without the swap), instead of the swapped value.
I don't know how NTLM works, but I guess this should also be the correct
behavior for NTLM?

Comment 15

[An-Cheng Huang]

The code structure in nsHttpChannel.cpp was changed by the fix for bug 224749.
This might also have fixed the problem here.  However, the test link posted    
by the reporter is no longer valid, so at the moment I'm not able to test it...

Comment 16

[An-Cheng Huang]

I believe the patch from Bug 114451 (applied on 20040128) has also fixed this
problem since it achieves what's described in Comment 11 (i.e., not setting
mMetaData to null).

Comment 17

[Darin Fisher]

resolving WFM based on previous comment.  reporter: please confirm.

Comment 18

[Adam D. Bradley]

Tried the 2004031208 build and all seems to be working fine.  I'll leave the
test case up and in working order just in case.