1394752 - NTLM/Negotiate Auth hangs browser

Status: RESOLVED FIXED

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

Description

[Florian Bender]

I'm in a corporate environment (with a rather loose proxy). I set up Firefox to automatically login for the intranet pages at intranet.company.tld (address redacted) with the following settings:

- network.automatic-ntlm-auth.trusted-uris;.company.tld
- network.negotiate-auth.delegation-uris;.company.tld
- network.negotiate-auth.trusted-uris;.company.tld

When I visit the intranet pages, I experience multiple and multi-second hangs of the browser (including chrome). Maybe it's some synchronous IPC?

Interestingly, when I reset network.negotiate-auth.trusted-uris to an empty value, the hangs disappear reproducibly.

Fx 55.0.3, e10s running well (5/5 windows, 1/1 content processes). Issue was visible in previous Firefox versions as well.

Happy to help you out with logs etc. (esp. of the login process) if you guide me how to acquire them.

Comment 1

[Honza Bambas (:mayhemer)]

Thanks for this report and an offer to help.  I will then ask you for logs according https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging.  

Please set the log modules (MOZ_LOG) as: sync,timestamp,nsHttp:5,negotiateauth:5,NTLM:5

Then start Firefox, reproduce the problem only and close it again.  It's better to send the log file to my bugzilla email directly than to expose it here.  There might be cookies exposed in the log.

Thanks!

Comment 2

[Honza Bambas (:mayhemer)]

And yes, this could be a duplicate of bug 791008, let's verify.

Comment 3

[Daniel Holbert [:dholbert]]

This sounds worth fixing, but doesn't sound like a QF bug. --> marking as [qf-]

(The QF effort is focused on high-usage websites and high-value benchmarks, and it can't cover all performance problems in Firefox.)

Comment 4

[Florian Bender]

Honza, I'm off for the next two seeks so it'll take a while before I can profile. Ni? me as reminder.

Comment 5

[Firefox Bug Husbandry Bot]

Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 6

[Honza Bambas (:mayhemer)]

Florian, still there?  To move forward it would be great to obtain the log as state in comment 1.  W/o it we can only guess what to do and hence de-prioritize this bug.

I suspect we need to implement async credentials processing also for the NTLM module (impl nsHttpNTLMAuth::GenerateCredentialsAsync.)  But I don't want to shoot into darkness.

Thanks!

Comment 7

[Florian Bender]

Yes, sorry, I did not forget about this bug, but was derailed by higher priority work. Plus, my computer crashed, so I need to wait to get it back. I hope to provide the logs by the end of this month.

Keeping ni?.

Comment 8

[Honza Bambas (:mayhemer)]

No time to work on this soon.

Comment 9

[Valentin Gosu [:valentin]]

This is blocking socket process, being the last consumer of DeprecatedSyncResolve.

Comment 10

[Valentin Gosu [:valentin]]

I need to look into how difficult it is to refactor this.

Comment 11

[Valentin Gosu [:valentin]]

It's not very clear why this bug was happening, but there are two possibilities:

1. It was happening because of the call to DeprecatedSyncResolve on the main thread.
2. It was happening because of the call to AcquireCredentialsHandleW was taking too long (also on the main thread)

For both of these, the solution would be to implement GenerateCredentialsAsync for nsHttpNTLMAuth.
The simple way to implement it would be to dispatch a task to the background thread that resolves the cname for the host, then calls nsAuthSSPI::Init on the same background thread.

The main problem with this is there is no code coverage for this.
Since this is using the windows SSPI functions, can't really hit this in automation. I've looked into creating a kerberos server in a VM, but it's not trivial. Another approach would be to have a dummy shim for the SecurityFunctionTableW structure for testing.

Comment 12

[Valentin Gosu [:valentin]]

Attachment: Bug 1394752 - Make NTLM/Negotiate auth async to avoid blocking the main thread r=#necko

Comment 13

[Valentin Gosu [:valentin]]

Attachment: WIP: Bug 1394752 - make pref static r=#necko

Depends on D142161

Comment 14

[Florian Bender]

Do you still need logs from me?

Comment 15

[Kershaw Chang [:kershaw]]

(In reply to Florian Bender from comment #14)

Do you still need logs from me?

Yes, I think so. Could you send the log to necko at mozilla dot com?
Thanks.

Comment 16

[Florian Bender]

Copy I'll see what I can do, will take a few days.

Comment 17

[BugBot [:suhaib / :marco]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit BugBot documentation.

Comment 18

[Valentin Gosu [:valentin]]

Attachment: Bug 1394752 - Use generic NTLM on windows xpcshell tests to avoid test failures r=#necko

On windows the sys-ntlm module is used, instead of the generic impl.
That can make the tests fail if we have a different expectation.

Comment 19

[Valentin Gosu [:valentin]]

Attachment: Bug 1394752 - Test that Negotiate auth builds the SPN from the canonical name r=#necko

nsAuthSSPI::InitAsync resolves the canonical name of the host so that the
SPN is built from the CNAME target rather than the name in the URI. Nothing
covered that: every existing Negotiate test uses localhost, which
nsHostResolver satisfies without reaching a resolver thread, so the lookup
completes re-entrantly inside AsyncResolveNative and the asynchronous path
is never taken.

Resolve the host through a TRR server instead. That forces a real async
resolution and lets the name be a CNAME for a different host, so the test
fails if the canonical name is dropped or if the lookup result never makes
it back to the channel.

Windows only, since nsAuthSSPI is the only nsIAuthModule that implements
initAsync.

Comment 20

[Valentin Gosu [:valentin]]

Attachment: Bug 1394752 - Test cancelling a channel while async auth init is in flight r=#necko

Deferring nsIAuthModule::init to a background thread means a channel can be
cancelled while the init is still running. Cancel() runs on the main thread
and races the background thread that finishes the init and goes on to
generate the credentials, both of which touch GetNextTokenCompleteEvent.

test_bug1683176.js already cancels during Negotiate credential generation,
but it resolves localhost, so the init completes re-entrantly and the
cancel can only ever land after it. Delay the DoH response instead: the
transaction is suspended for authentication while the canonical name lookup
is still outstanding, so the cancel is guaranteed to land mid-init. The test
asserts the server never sees an authenticated retry, which is what
distinguishes a cancel that took effect from one that arrived too late.

Comment 21

[Valentin Gosu [:valentin]]

Attachment: Bug 1394752 - Test cancelling a channel during async NTLM credential generation r=#necko

nsHttpNTLMAuth::GenerateCredentialsAsync runs GenerateCredentials on a
background thread, so Cancel() can run on the main thread concurrently with
the background thread filling in NTLMGetNextTokenCompleteEvent. Whichever
side wins, the callback has to run exactly once and the channel has to end
up aborted.

test_bug1683176.js covers this for Negotiate but there was no equivalent for
NTLM, which is the path that just gained a background thread.

Uses a content principal because the generic NTLM module reports the
identity as invalid and the channel prompts, which is blocked for
non-web-content triggered loads.

Comment 22

[Valentin Gosu [:valentin]]

Attachment: Bug 1394752 - Add gtests for the nsIAuthModule::initAsync contract r=#necko

There is no way to drive initAsync from JS with a mock module, because
nsIAuthModule::CreateInstance is a hardcoded factory rather than a component
manager lookup, so cover the contract directly.

• Modules without an async init must report NS_ERROR_NOT_IMPLEMENTED.
  NegotiateAuthState::InitAsync and GetNextTokenRunnable::Run both fall back
  to the synchronous Init on exactly that result, so a module that grows a
  partial implementation without returning it would silently skip
  initialization.

• nsAuthSSPI must complete a Kerberos init off the main thread. localhost
  is answered without reaching a resolver thread, so before InitAsync passed
  an event target to AsyncResolveNative this ran re-entrantly on the caller's
  thread; the test fails (on the MOZ_ASSERT in OnLookupComplete) if that
  regresses.

• NTLM skips the canonical name lookup, so its init still reports completion
  before returning.

• Empty and malformed service names are rejected without invoking the
  callback.

Comment 23

[Valentin Gosu [:valentin]]

Attachment: Bug 1394752 - Test native NTLM proxy and web auth r=#necko

Every existing NTLM test either sets network.auth.force-generic-ntlm or runs
somewhere sys-ntlm is unavailable, so nsHttpNTLMAuth::mUseNative and the
native module itself had no coverage at all. That matters here because
nsAuthSSPI is now constructed on the main thread in ChallengeReceived and
then has Init and GetNextToken called on a background thread, which is why it
needed threadsafe refcounting.

Trusting localhost for automatic NTLM means the native module uses default
credentials and reports the identity as valid, so the channel never prompts.
The test fails if anything asks for a prompt, which is what keeps it honest:
the internal module always prompts, so a silent fallback to it is caught
rather than passing quietly.

The sequence stops after the type 1 message. Answering a type 2 challenge
makes SSPI derive a response from whoever is logged in, and on a machine with
no credentials for the domain named in the challenge it fails with
SEC_E_NO_CREDENTIALS -- which is why the other proxy tests force the generic
module. The type 3 step stays covered there.

Comment 24

[Valentin Gosu [:valentin]]

Attachment: Bug 1394752 - Test native NTLM against a server that validates the response r=#necko

The other native NTLM tests accept any well-formed type 3 message, so a bug
that computed the NTLMv2 proof wrongly would still pass them. This one has
the server recompute the proof from a password it knows and compare:

NTHash = MD4(UTF16LE(password))
NTLMv2Hash = HMAC-MD5(NTHash, UTF16LE(uppercase(user) + domain))
NTProofStr = HMAC-MD5(NTLMv2Hash, serverChallenge + blob)

The server runs on the node harness, issues a real type 2 with a fresh random
challenge per connection, and returns 200 only when the proof matches. A
second task points it at a different password and expects 403, so a validator
that accepted everything cannot masquerade as a working one.

MD4 is implemented in JS because OpenSSL 3 moved it to the legacy provider
and node cannot supply it. It is checked against three known answers,
including the NT hash of the password from MS-NLMP, so a broken digest shows
up as a failed vector rather than as an unexplained rejection.

Uses explicit credentials for the same reason as the full exchange test:
default credentials give SEC_E_NO_CREDENTIALS on a machine that is not
domain joined.

Comment 25

[Valentin Gosu [:valentin]]

Attachment: Bug 1394752 - Remove the scriptable synchronous nsIDNSService::resolve r=#necko

Nothing calls it. There is no JS caller anywhere in the tree, and the only two
implementations were nsDNSService::Resolve, which unwrapped the origin
attributes and forwarded straight to ResolveNative, and
ChildDNSService::Resolve, which returned NS_ERROR_NOT_AVAILABLE with the
comment that sync IPDL is evil. DNSServiceWrapper forwards via
NS_FORWARD_NSIDNSSERVICE and both implementors declare their methods with
NS_DECL_NSIDNSSERVICE, so removing the method from the interface is enough.

resolveNative stays. It has three callers, and they all pass RESOLVE_OFFLINE or
run off the main thread, so they consult literals and the cache rather than
blocking: nsCORSListenerProxy and two in DnsAndConnectSocket. Its documentation
was attached to the scriptable method, so it moves and is corrected to describe
the RESOLVE_OFFLINE exemption that ResolveNative actually implements.

The uuid is deliberately not bumped: the interface is builtinclass and internal,
and binary XPCOM consumers have not existed since Firefox 57.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e410de302947
https://hg.mozilla.org/integration/autoland/rev/876b42ba609b
Use generic NTLM on windows xpcshell tests to avoid test failures r=necko-reviewers,kershaw
https://github.com/mozilla-firefox/firefox/commit/1cea5b89b166
https://hg.mozilla.org/integration/autoland/rev/588ed54bb13e
Make NTLM/Negotiate auth async to avoid blocking the main thread r=necko-reviewers,kershaw
https://github.com/mozilla-firefox/firefox/commit/bf0a9bf15cf3
https://hg.mozilla.org/integration/autoland/rev/f6ac49b649fb
Test that Negotiate auth builds the SPN from the canonical name r=necko-reviewers,kershaw
https://github.com/mozilla-firefox/firefox/commit/cda80310f472
https://hg.mozilla.org/integration/autoland/rev/28c149b17367
Test cancelling a channel while async auth init is in flight r=necko-reviewers,kershaw
https://github.com/mozilla-firefox/firefox/commit/37e674c96f75
https://hg.mozilla.org/integration/autoland/rev/30ea1007227a
Test cancelling a channel during async NTLM credential generation r=necko-reviewers,kershaw
https://github.com/mozilla-firefox/firefox/commit/9e047566f3e9
https://hg.mozilla.org/integration/autoland/rev/d4d395dae5d8
Add gtests for the nsIAuthModule::initAsync contract r=necko-reviewers,kershaw
https://github.com/mozilla-firefox/firefox/commit/fe91f5cf9454
https://hg.mozilla.org/integration/autoland/rev/74584fca31f6
Test native NTLM proxy and web auth r=necko-reviewers,kershaw
https://github.com/mozilla-firefox/firefox/commit/8d447f79c396
https://hg.mozilla.org/integration/autoland/rev/eded1691d088
Test native NTLM against a server that validates the response r=necko-reviewers,kershaw
https://github.com/mozilla-firefox/firefox/commit/565d55563c2b
https://hg.mozilla.org/integration/autoland/rev/e6cdd2a3ede9
Remove the scriptable synchronous nsIDNSService::resolve r=necko-reviewers,jesup

Comment 27

[Silaghi Andreea]

https://hg.mozilla.org/mozilla-central/rev/876b42ba609b
https://hg.mozilla.org/mozilla-central/rev/588ed54bb13e
https://hg.mozilla.org/mozilla-central/rev/f6ac49b649fb
https://hg.mozilla.org/mozilla-central/rev/28c149b17367
https://hg.mozilla.org/mozilla-central/rev/30ea1007227a
https://hg.mozilla.org/mozilla-central/rev/d4d395dae5d8
https://hg.mozilla.org/mozilla-central/rev/74584fca31f6
https://hg.mozilla.org/mozilla-central/rev/eded1691d088
https://hg.mozilla.org/mozilla-central/rev/e6cdd2a3ede9

Comment 28

[Donal Meehan [:dmeehan]]

Did you want to nominate this for the Fx160 relnotes? If so, set the relnote-firefox flag to "?"
https://wiki.mozilla.org/Release_Management/Release_Notes_Nomination

Possible wording:

Fixed Firefox freezing for several seconds when signing in to intranet sites that use NTLM or Kerberos (Negotiate) authentication.

Comment 30

[Valentin Gosu [:valentin]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Fixes a longstanding issue in Firefox where the thread is blocked waiting for DNS resolution.
[Affects Firefox for Android]: No
[Suggested wording]: Fixed Firefox freezing for several seconds when signing in to intranet sites that use NTLM or Kerberos (Negotiate) authentication.
[Links (documentation, blog post, etc)]:

Comment 31

[Donal Meehan [:dmeehan]]

Thanks, added to the Fx160 nightly release notes, please allow 30 minutes for the site to update.