Closed
Bug 1309438
Opened 5 years ago
Closed 4 years ago
After entering an invalid password on an NTLM authenticated site, the correct password is rejected subsequently
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: gary, Assigned: mayhemer)
References
Details
(Whiteboard: [necko-active][ntlm][squid])
Attachments
(5 files, 1 obsolete file)
54.88 KB,
application/octet-stream
|
Details | |
8.51 KB,
patch
|
mayhemer
:
review+
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
2.26 MB,
text/x-log
|
Details | |
2.41 MB,
text/x-log
|
Details | |
2.69 MB,
text/x-log
|
Details |
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.
![]() |
Assignee | |
Updated•4 years ago
|
Assignee: nobody → honzab.moz
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [necko-next] → [necko-active]
![]() |
Assignee | |
Comment 1•4 years ago
|
||
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.
Reporter | ||
Comment 2•4 years ago
|
||
Confirmed as an issue with squid, Internet Explorer and Firefox using the native windows NTLM show the same behaviour.
Flags: needinfo?(honzab.moz)
![]() |
Assignee | |
Comment 3•4 years ago
|
||
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]
![]() |
Assignee | |
Comment 4•4 years ago
|
||
Turns out to be simple.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Whiteboard: [necko-backlog][ntlm][squid] → [necko-active][ntlm][squid]
![]() |
Assignee | |
Comment 5•4 years ago
|
||
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)
Reporter | ||
Comment 6•4 years ago
|
||
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)
![]() |
Assignee | |
Comment 7•4 years ago
|
||
(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)
![]() |
Assignee | |
Comment 8•4 years ago
|
||
Patrick, ping on review, or please suggest someone else. Thanks.
Comment 9•4 years ago
|
||
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+
![]() |
Assignee | |
Comment 10•4 years ago
|
||
(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().
![]() |
Assignee | |
Comment 11•4 years ago
|
||
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.
![]() |
Assignee | |
Comment 12•4 years ago
|
||
Attachment #8809784 -
Attachment is obsolete: true
Attachment #8814381 -
Flags: review+
![]() |
Assignee | |
Comment 13•4 years ago
|
||
(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
![]() |
Assignee | |
Updated•4 years ago
|
status-firefox52:
--- → affected
status-firefox53:
--- → affected
Reporter | ||
Comment 14•4 years ago
|
||
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
Reporter | ||
Comment 16•4 years ago
|
||
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
Reporter | ||
Comment 17•4 years ago
|
||
Reporter | ||
Comment 18•4 years ago
|
||
![]() |
Assignee | |
Comment 19•4 years ago
|
||
(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
![]() |
Assignee | |
Comment 20•4 years ago
|
||
(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)
Reporter | ||
Comment 21•4 years ago
|
||
Applied patch for bug 1315332 this fixes 1, but does not fix 2
Flags: needinfo?(gary)
![]() |
Assignee | |
Comment 22•4 years ago
|
||
(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)
Reporter | ||
Comment 23•4 years ago
|
||
Flags: needinfo?(gary)
Comment 24•4 years ago
|
||
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
Comment 25•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8300b41fe20b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
![]() |
Assignee | |
Comment 26•4 years ago
|
||
(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•4 years ago
|
||
Honza, is that a regression? If so, is it recent? Is it something that we would want to uplift? Thanks
Flags: needinfo?(honzab.moz)
![]() |
Assignee | |
Comment 28•4 years ago
|
||
(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)
Comment 29•4 years ago
|
||
ok, merci
Sounds like this is necessary for bug 1315332 which we want to fix in esr52. (and maybe for 1346392?)
tracking-thunderbird_esr52:
--- → ?
![]() |
Assignee | |
Comment 32•4 years ago
|
||
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?
Updated•4 years ago
|
status-firefox-esr52:
--- → affected
tracking-firefox-esr52:
--- → ?
tracking-thunderbird_esr52:
? → ---
Comment 33•4 years ago
|
||
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+
Comment 34•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/1f30d97563c8
Updated•4 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•