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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Gary Lockyer]

Attachment: invalidNTLMPassword.pcapng

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.

Comment 1

[Honza Bambas (:mayhemer)]

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.

Comment 2

[Gary Lockyer]

Confirmed as an issue with squid, Internet Explorer and Firefox using the native windows NTLM show the same behaviour.

Comment 3

[Honza Bambas (:mayhemer)]

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

Comment 4

[Honza Bambas (:mayhemer)]

Turns out to be simple.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1 (don't reuse the sticky conn on a newly received auth challange)

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

Comment 6

[Gary Lockyer]

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

Comment 7

[Honza Bambas (:mayhemer)]

(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?

Comment 8

[Honza Bambas (:mayhemer)]

Patrick, ping on review, or please suggest someone else.  Thanks.

Comment 9

[Patrick McManus [:mcmanus]]

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

Comment 10

[Honza Bambas (:mayhemer)]

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

Comment 11

[Honza Bambas (:mayhemer)]

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.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v1.1 (don't reuse the sticky conn on a newly received auth challange)

Comment 13

[Honza Bambas (:mayhemer)]

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

Comment 14

[Gary Lockyer]

Honza apologies for not responding sooner, I've been off site with limited connectivity.
I'll test the patch now.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 16

[Gary Lockyer]

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

Comment 17

[Gary Lockyer]

Attachment: Run03 https request to apache web server running NTLM auth

Comment 18

[Gary Lockyer]

Attachment: run04 https request to apache webserver using NTLM auth behind a Squid proxy also using NTLM auth

Comment 19

[Honza Bambas (:mayhemer)]

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

Comment 20

[Honza Bambas (:mayhemer)]

(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?

Comment 21

[Gary Lockyer]

Applied patch for  bug 1315332 this fixes 1, but does not fix 2

Comment 22

[Honza Bambas (:mayhemer)]

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

Comment 23

[Gary Lockyer]

Attachment: Issue 2 with patch for bug 1315332 applied

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8300b41fe20b
Don't reuse sticky connection after authentication failure. r=jduell

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8300b41fe20b

Comment 26

[Honza Bambas (:mayhemer)]

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

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Honza, is that a regression? If so, is it recent? Is it something that we would want to uplift? Thanks

Comment 28

[Honza Bambas (:mayhemer)]

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

Comment 29

[Sylvestre Ledru [:Sylvestre]]

ok, merci

Comment 30

[Honza Bambas (:mayhemer)]

uplift to esr52

Comment 31

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Sounds like this is necessary for bug 1315332 which we want to fix in esr52. (and maybe for 1346392?)

Comment 32

[Honza Bambas (:mayhemer)]

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.

Comment 33

[Julien Cristau [:jcristau]]

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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/1f30d97563c8