After entering an invalid password on an NTLM authenticated site, the correct password is rejected subsequently

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gary, Assigned: mayhemer)

Tracking

52 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox-esr5253+ fixed, firefox53 fixed)

Details

(Whiteboard: [necko-active][ntlm][squid])

Attachments

(5 attachments, 1 obsolete attachment)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161012090050

Steps to reproduce:

Connect to a NTLM authenticated site.
Enter an incorrect password in the user and password popup
When this fails enter the correct password.


Actual results:

Once the initial password is rejected, it is impossible to enter the correct password.

If the browser is restarted or the connection is closed by the host.  Then the correct password can be entered.

The issue appears to be that the NTLMSSP_NEGOTIATE message is being resent on the connection.





Expected results:

In the event of an NTLM authentication failure the connection needs to be closed and a new connection established.  The NTLMSSP_NEGOTIATE message should only ever be sent once on a  conection.


See the attached packet capture, once the connection timed out the correct password could be entered.
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
Whiteboard: [necko-next]
Assignee: nobody → honzab.moz
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [necko-next] → [necko-active]
This seems server specific.  I cannot reproduce locally with IIS 10.  I checked the logs, we reuse the same connection, but negotiation happened correctly twice on it, once with a wrong password, second with a correct one and result was 200 with the resource (expected).

Apparently, the pcap you've attached is from squid.
Confirmed as an issue with squid, Internet Explorer and Firefox using the native windows NTLM show the same behaviour.
Flags: needinfo?(honzab.moz)
Thanks for confirmation.

OK, since these days I'm busy, I won't have time to fix this.  

The solution here is to mark the sticky connection as DontReuse() when we get the final 407 response and have to ask the user for credentials (again).
Assignee: honzab.moz → nobody
Flags: needinfo?(honzab.moz)
Whiteboard: [necko-active] → [necko-backlog][ntlm][squid]
Turns out to be simple.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Whiteboard: [necko-backlog][ntlm][squid] → [necko-active][ntlm][squid]
two places we set DontReuse on the sticky conn:
- receive of a new challenge (identityInvalid == true returned by ChallengeReceived of the current challenge auth module) while the previous schema was conn-based
- auth cancelation (pressing cancel in the auth dialog)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95545dd0750051082fbb0d0a1991fa85d41a6943
Attachment #8809784 - Flags: review?(mcmanus)
Blocks: 734229
Have applied the patch and found the following issues in testing:

1) NTLM authenticated web site behind and NTLM authtenticated proxy

   The proxy session appears to be cleared once the web site auth is processed, get the following 
   sequence.
   1) web site auth
   2) proxy auth

2) When authenticating to an NTLM authenticated web server, no proxy
   The connection is reset immediately after the password is supplied. pressing the retry button then
   displays the web page.

On the plus side the correct password is accepted after a correct password
Flags: needinfo?(honzab.moz)
(In reply to Gary Lockyer from comment #6)
> Have applied the patch and found the following issues in testing:
> 
> 1) NTLM authenticated web site behind and NTLM authtenticated proxy
> 
>    The proxy session appears to be cleared 

Not sure I follow what you mean here exactly.  What "session appears to be cleared" exactly means?

> once the web site auth is
> processed, get the following 
>    sequence.
>    1) web site auth
>    2) proxy auth

Again, not sure I understand.  Also, please make a note next time what is the server your are using, if it's IIS (which I'm testing against, in version 10.x) or squid or something else.

Best would be to provide a life example (probably not easy) or provide a log (MOZ_LOG), as before.  Logs usually give the answer quite reliably.

> 
> 2) When authenticating to an NTLM authenticated web server, no proxy
>    The connection is reset immediately after the password is supplied.
> pressing the retry button then
>    displays the web page.

I think this is simply bug 1315332, which is a result of 486508 and its dependencies.  I'm not sure of a simple solution there.

> 
> On the plus side the correct password is accepted after a correct password

after a _correct_ or _incorrect_ password?
Flags: needinfo?(honzab.moz) → needinfo?(gary)
Blocks: 1315332
Patrick, ping on review, or please suggest someone else.  Thanks.
Comment on attachment 8809784 [details] [diff] [review]
v1 (don't reuse the sticky conn on a newly received auth challange)

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5511,5 @@
> +
> +    // Prove we are between OnStartRequest and OnStopRequest, because
> +    // what we do here takes effect in OnStopRequest (not reusing the
> +    // connection for next authentication round).
> +    MOZ_ASSERT(mIsPending);

as this is an idl, you should handle the error without the assert
Attachment #8809784 - Flags: review?(mcmanus) → review+
(In reply to Patrick McManus [:mcmanus] from comment #9)
> Comment on attachment 8809784 [details] [diff] [review]
> v1 (don't reuse the sticky conn on a newly received auth challange)
> 
> Review of attachment 8809784 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +5511,5 @@
> > +
> > +    // Prove we are between OnStartRequest and OnStopRequest, because
> > +    // what we do here takes effect in OnStopRequest (not reusing the
> > +    // connection for next authentication round).
> > +    MOZ_ASSERT(mIsPending);
> 
> as this is an idl, you should handle the error without the assert

You mean to rather throw than assert?  If so, I'd still add an NS_ERROR().
Gary, it's 10 days w/o a feedback from you.  I will land the patch as is, since I believe it fixes what the description of the bug is about.
(In reply to Honza Bambas (:mayhemer) from comment #12)
> Created attachment 8814381 [details] [diff] [review]
> v1.1 (don't reuse the sticky conn on a newly received auth challange)

For try, see comment 5.  The patch change is negligible.
Keywords: checkin-needed
Honza apologies for not responding sooner, I've been off site with limited connectivity.
I'll test the patch now.
Flags: needinfo?(gary)
The patch has r=jduell. Did you get that on IRC or is this by mistake (because there is an r=mcmanus here)? Also re-add 'checkin-needed' when you resolve the needinfo. Thank you.
Flags: needinfo?(honzab.moz)
Keywords: checkin-needed
I've applied the patch to the latest nightly and can see two issues:

1) Apache web server with NTLM auth
   a) get prompted for user id and password
   b) browser displays secure connection failed, connection reset
   c) clicking Try again successfully connects to the page
   log file run03.log attached

2) Squid proxy using NTLM auth in front of Apache web server using NTLM
   a) proxy auth prompt
   b) web server auth prompt
   c) proxy auth prompt
   d) page renders
   log file run04.log attached
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #15)
> The patch has r=jduell. Did you get that on IRC or is this by mistake
> (because there is an r=mcmanus here)? Also re-add 'checkin-needed' when you
> resolve the needinfo. Thank you.

Oh!  Yes, originally got eyes from jduell in a different bug (I then split this part to this separate bug later), but here it actually got r from mcmanus, yes.  Sorry, I failed to update that.

Can you please update during the landing?  I can also provide an updated patch if needed.  Thanks.
Flags: needinfo?(honzab.moz)
Keywords: checkin-needed
(In reply to Gary Lockyer from comment #16)
> I've applied the patch to the latest nightly and can see two issues:
> 
> 1) Apache web server with NTLM auth
>    a) get prompted for user id and password
>    b) browser displays secure connection failed, connection reset
>    c) clicking Try again successfully connects to the page
>    log file run03.log attached

Bug 1315332 solves that.

> 
> 2) Squid proxy using NTLM auth in front of Apache web server using NTLM
>    a) proxy auth prompt
>    b) web server auth prompt
>    c) proxy auth prompt
>    d) page renders
>    log file run04.log attached

This is new.  Maybe try applying patch from bug 1315332 as well?
Flags: needinfo?(gary)
Applied patch for  bug 1315332 this fixes 1, but does not fix 2
Flags: needinfo?(gary)
(In reply to Gary Lockyer from comment #21)
> Applied patch for  bug 1315332 this fixes 1, but does not fix 2

Thanks.  OK, so, please either give me a log with the fix for the case 2 or please tell me how to setup squid for NTLM (simply), I'm on Fedora 24 so I can repro myself locally.

Thanks.
Flags: needinfo?(gary)
Flags: needinfo?(gary)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8300b41fe20b
Don't reuse sticky connection after authentication failure. r=jduell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8300b41fe20b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(In reply to Gary Lockyer from comment #23)
> Created attachment 8815026 [details]
> Issue 2 with patch for bug 1315332 applied

Filed and fixed bug 1320999 for that.
Honza, is that a regression? If so, is it recent? Is it something that we would want to uplift? Thanks
Flags: needinfo?(honzab.moz)
(In reply to Sylvestre Ledru [:sylvestre] from comment #27)
> Honza, is that a regression? If so, is it recent? 

It's been there since ever, no, not a regression.

> Is it something that we
> would want to uplift? Thanks

Let it ride.
Flags: needinfo?(honzab.moz)
uplift to esr52
Flags: needinfo?(honzab.moz)
Sounds like this is necessary for bug 1315332 which we want to fix in esr52. (and maybe for 1346392?)
Comment on attachment 8814381 [details] [diff] [review]
v1.1 (don't reuse the sticky conn on a newly received auth challange)

Applies cleanly on esr52.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: prerequisite to fix a major NTLM authentication bug
User impact if declined: as above
Fix Landed on Version: 53
Risk to taking this patch (and alternatives if risky): small if none
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(honzab.moz)
Attachment #8814381 - Flags: approval-mozilla-esr52?
Comment on attachment 8814381 [details] [diff] [review]
v1.1 (don't reuse the sticky conn on a newly received auth challange)

ntlm auth fix for esr52
Attachment #8814381 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.