286803 - Automatic NTLM auth fails if SSPI 'negotiate' package not found

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Darin Fisher]

Automatic NTLM auth fails if SSPI 'negotiate' package not found

We have buggy code in nsNegotiateAuthSSPI.cpp's InitSSPI function that only
succeeds if SSPI provides a 'negotiate' package.  For automatic NTLM auth, we
leverage the same code that we created for negotiate auth to handle NTLM.  That
means that this code should initialize itself properly even if the OS only
provides NTLM and not negotiate auth.

Comment 1

[Darin Fisher]

This is only showing up as a bug on NT4 and other old systems.

Comment 2

[Darin Fisher]

Attachment: possible patch

This patch should do the trick, but I haven't tested it.  If someone can help
test it that would be greatly appreciated.  I lack access to servers that I
could use to test this patch.

Comment 3

[Darin Fisher]

Test build posted here: http://friedfish.homeip.net/~darinf/sspi/firefox-test.zip

Comment 4

[Darin Fisher]

Comment on attachment 177929 [details] [diff] [review]
possible patch

Thanks to Stefan Baur "www.stefanbaur.de/KSKLB" for verifying that this patch
works.

Biesi: this is a pretty straightforward patch.	it just moves the call to
QuerySecurityPackageInfo down to the initialization routine for each instance
of the nsNegotiateAuth object.	NOTE: don't be confused by the fact that this
class is named "nsNegotiateAuth".. it is actually a generic class that
interfaces with Microsoft SSPI.  Originally, it was only for the "negotiate"
auth package, but now we use it for "NTLM" as well (in order to support
automatic NTLM authentication leveraging the user's login credentials).

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

interesting... this leaks some stuff, presumably (and always did), since MSDN says:
"The caller must call the FreeContextBuffer function to free the buffer returned
in ppPackageInfo."
(http://msdn.microsoft.com/library/default.asp?url=/library/en-us/secauthn/security/querysecuritypackageinfo.asp)

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

while I'm looking at this file...

    // This should always hit our DNS cache
    nsCOMPtr<nsIDNSRecord> record;
    rv = dns->Resolve(Substring(buf, index + 1),
                      nsIDNSService::RESOLVE_CANONICAL_NAME,
                      getter_AddRefs(record));

Why's the comment true? Won't the RESOLVE_CANONICAL_NAME flag make it so that
this will almost always not be cached? (unless you hit this code more than once
for a given host). The nsHostResolver.cpp code carefully avoids hashing an entry
with and without that flag to the same key.
(Was this from before that flag, when AI_CANONNAME was always passed to
getaddrinfo?)

(I wonder if this should use iterators instead of FindChar)

Comment 7

[Darin Fisher]

yes, you are correct.  we will not benefit from a cached result the first time.
 that comment pre-dates the existance of that flag anyways.  i'll revise the
comment accordingly.

thanks for noticing that we are not free'ing that memory buffer.  i'll make that
change which is especially important since we will now be querying the package
more often.

i don't see any real advantage to using iterators at this point.

Comment 8

[Darin Fisher]

Attachment: v1.1 patch

revised patch per review comments from biesi

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 178089 [details] [diff] [review]
v1.1 patch

r=biesi

Comment 10

[Darin Fisher]

fixed-on-trunk

tomorrow's nightly builds will include this fix.