237586 - Implement negotiateauth using SSPI for Windows

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Darin Fisher]

Implement negotiateauth using SSPI for Windows.

This is a followup to bug 17578.  In that bug, we enabled support for
Microsoft's Negotiate auth protocol using GSSAPI.  Under Windows, we need to use
SSPI instead.

Comment 1

[Darin Fisher]

Attachment: v0 patch

this is a partial patch.  some changes to configure.in are still needed, and
this patch depends on my patch for bug 241124.

Comment 2

[Darin Fisher]

Attachment: configure.in changes

Comment 3

[Darin Fisher]

Attachment: v1 patch

OK, this might work... I still don't have a good Negotiate auth test case )-:

Comment 4

[cls]

Comment on attachment 146893 [details] [diff] [review]
configure.in changes

-if test `echo "$MOZ_EXTENSIONS" | grep -c negotiateauth` -ne 0; then
+if test -n "$USE_GSSAPI" -a test `echo "$MOZ_EXTENSIONS" | grep -c
negotiateauth` -ne 0; then

That should be:
test <expr> -a <expr>
or 
test <expr> && test <expr>
.
The latter will not evaluate the second expression if the first fails.	Other
than that, it looks good.

Comment 5

[Darin Fisher]

cls: oh, right... thanks for catching that!

Comment 6

[Darin Fisher]

Attachment: revised configure.in changes

Comment 7

[Darin Fisher]

Attachment: v1.1 patch

revised patch now that the negotiateauth refactorization patch has landed on
the trunk.

Comment 8

[Darin Fisher]

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

cneberg (or anyone else cc'd on this bug): can you please test this out on a
win32 box?  thanks!

Comment 9

[Christopher Nebergall]

I'll give it a try.

Comment 10

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

So, does implementing negotiateauth using SSPI not have the same problems that
NTLM had? didn't that use to crash?

Comment 11

[Christopher Nebergall]

It fails, error message below.  I tried to do a little debugging on my own, but
haven't figured out the problem.   I tried changing the servicename from
HTTP@FQDN to to HTTP/FQDN because I'm guessing that is more correct for the
SSPI, but I'm not sure.  This did not fix the problem.  If you have anything
else for me to try let me know.

InitializeSecurityContext failed
rc=-2146893055

Comment 12

[Darin Fisher]

That error code is SEC_E_INVALID_HANDLE.  Hmm... I must have done something silly.

Comment 13

[Darin Fisher]

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

>+    if (inToken) {
...
>+        ctxIn = &mCtxt;
>+    }
>+    else {
...
>+        ctxIn = NULL;
>+    }
...
>+    rc = (sspi->InitializeSecurityContext)(&mCred,
>+                                           ctxIn,

in fact, i think the problem is in the above code.  i should only
assign ctxIn to the address of mCtxt if InitializeSecurityContext
has been called at least once already.

new patch coming up.

Comment 14

[Darin Fisher]

Attachment: v1.2 patch

Actually, the problem was with the Reset function.  It should not have been
destroying the CredHandle object.  I tested this patch with a mock server that
just generates a 401 response with a "WWW-Authenticate: Negotiate" header. 
SSPI generated a SPNEGO response, so I think there's a chance this patch will
actually work.	I haven't confirmed the patch beyond that though.  Again,
please help me test this.  Thanks!

Comment 15

[Christopher Nebergall]

Kerberos works if HTTP@ is changed to HTTP/ to create the service principal.  
Should DsMakeSpn be used to do do this? 

Oddities:
It doesn't use the reverse DNS name for forming the SPN like IE 6.0 normally
does (IE is somewhat erratic on what it chooses) instead it uses the same name
you use in the URL. (Maybe DsMakeSpn would fix this?)

Negotiate fails over to NTLM if Kerberos fails, so our current pref defaults are
wrong for use with the SSPI. We really need a GUI so users can set their
intranet domain, but without this the only secure default is to disable it.

In addition, the SSPI is different than GSSAPI, SSPI will only delegate if an AD
system administrator configures a special flag on the service called
OK-AS-DELEGATE.   This allows the AD sys admins to control delegation.  IE
always enables delegation to the SSPI, but it only succeeds if that flag is set.
 (I varified this in my tests). I suggest we do the same thing and always enable
delegation.

Suggested Win32 defaults

network.negotiate-auth.delegation-uris   ""
network.negotiate-auth.trusted-uris      "https://, http://"

When testing this bug I noticed that the prefs are not working correctly, when a
list of schemes is used, only the last one is matched against, the eariler ones
are ignored. 

Comment 16

[Christopher Nebergall]

Oops. Sorry, reverse the values for the prefs for default.

network.negotiate-auth.delegation-uris   "https://, http://"
network.negotiate-auth.trusted-uris      ""

Comment 17

[Christopher Nebergall]

Bug 242446 added with fix for nsHttpNegotiateAuth::MatchesBaseURI not working
correctly.

Comment 18

[Darin Fisher]

I also read somewhere that SSPI delegation requires mutual-auth.  Is that correct?

Comment 19

[Christopher Nebergall]

I'm not sure(In reply to comment #18)
> I also read somewhere that SSPI delegation requires mutual-auth.  Is that correct?

I don't know what the SSPI spec says, but delegation worked fine to the apache
server I tested with the current patch. I had no problems.  I can check against
IIS next week and see if it cares. (I realize it makes no security sense to put
the check in the server, but I'll test it anyway.)

IE always asks for mutual auth but doesn't seem to ever check the reply. So I'm
not even sure why they ask for it...

Comment 20

[Jari Ahonen]

Patch (v1.2) seems to work for me when "HTTP@" is changed to "HTTP/" in
nsHttpNegotiateAuth.cpp line 159.

Test server is Apache 2.0.49 with CVS version of mod_auth_kerb.

Comment 21

[Jari Ahonen]

(In reply to comment #15)
> It doesn't use the reverse DNS name for forming the SPN like IE 6.0 normally
> does (IE is somewhat erratic on what it chooses) instead it uses the same name
> you use in the URL. (Maybe DsMakeSpn would fix this?)

I noticed this as well. It would be nice if the Mozilla service principal was
same as used by IE. Based on my experiences I thought IE will lookup the
canonical hostname and use that as service principal hostname. Is this (how IE
will form the SPN) documented somewhere ?

Comment 22

[Darin Fisher]

Hmm... DsMakeSpn seems to only be available on Windows XP or Windows 2000
Professional, or otherwise it requires the Active Directory Client Extension.

I'm happy to add code to nsNegotiateAuthSSPI.cpp that canonicalizes the hostname
and generates a SPN from that (with HTTP/ prepended).  We could try using
DsMakeSpn if it happens to be available, but it seems clear to me that we cannot
rely on that always being present.

Comment 23

[Darin Fisher]

Attachment: v1.3 patch

OK, this patch addresses the SPN canonicalization issue.  I've written a
function that maps the "HTTP@host" SPN to "HTTP/cname".  This is only done for
the SSPI code path.

This patch also corrects an infinite loop that can occur if the server responds
to the client's authorization attempt with a brand new "WWW-Authenticate:
Negotiate NTLM" header.  This can easily happen when NTLM is used because the
server might reject the user's credentials.  We want to make sure that when
this happens we give up on Negotiate and use the second challenge.

To canonicalize the SPN, I used nsIDNSService::resolveHost.  This method blocks
the calling thread until the lookup completes, which might seem problematic for
performance; however, I doubt it will be an issue because we will most likely
already have the hostname stored in the DNS cache.  The CNAME I'm using is the
CNAME that is returned with the original DNS "A" query.  I'm not doing a
reverse lookup.  Perhaps I should since that's what the GSSAPI seems to do;
however... I'm not too worried since site admins can ensure that the CNAME
returned with "A" queries is correct.

Comment 24

[Darin Fisher]

I think this patch necessitates that we change the default trusted URIs to none.
 It is no longer sufficient to limit the default trusted URIs to https:// since
that would allow a hacker to setup a https:// site to sniff NTLM v1 hashes. 
That might be too much of a security risk.  I think the only sane thing to do
for now is to ship Moz with an empty whitelist.

At some point, we should maybe implement some UI to allow the user to configure
the whitelist.  I.e., it'd be nice if we prompted the user when a site they are
loading is not on the whitelist.  And, there should be a way for the user to
configure the whitelist without resorting to about:config.

I understand that a hidden pref to configure the whitelist is probably still
useful in a lot of cases, so there is hopefully value in having this feature in
mozilla but default disabled.

Comment 25

[Wyllys Ingersoll]

(In reply to comment #24)
> I think this patch necessitates that we change the default trusted URIs to none.
>  It is no longer sufficient to limit the default trusted URIs to https:// since
> that would allow a hacker to setup a https:// site to sniff NTLM v1 hashes. 
> That might be too much of a security risk.  I think the only sane thing to do
> for now is to ship Moz with an empty whitelist.

Without any sort of visible UI, most people will have no clue how to enable this
feature.  At this point, I think we *REALLY* need some visible configuration
knobs for this feature in the "Security" panel.

By the way - howcome "about:prefs" doesn't work in Firefox??  How is one
supposed to edit all of the hidden preferences without knowing what
they are in the first place?

> 
> At some point, we should maybe implement some UI to allow the user to configure
> the whitelist.  I.e., it'd be nice if we prompted the user when a site they are

I think by disabling the lists, you are effectively disabling this feature so
some sort of UI is almost a necessity in order to enable it again.

Comment 26

[Darin Fisher]

I'd really like to see this feature get added to FFox 1.0, and with any luck Moz
1.7.1 as well.

Comment 27

[Darin Fisher]

Attachment: changes to all.js

Comment 28

[Christopher Nebergall]

I'm on vacation now. I'll have time to look at this on Tuesday. Do we have to
change the defaults in all.js for every platform?  The security threat is only
for Windows, keeping the old default still makes sense for every other platform.
Maybe we could leave all.js blank and set the default in the C code, or else we
can start to discuss a gui layout, I'll do my best to help with it if it means
we could get in in time of the next release.

Comment 29

[Darin Fisher]

> I'm on vacation now. I'll have time to look at this on Tuesday.

No problem.  Happy 4th! ;-)


> Do we have to
> change the defaults in all.js for every platform?  The security threat is only
> for Windows, keeping the old default still makes sense for every other 
> platform.

The challenge really is that we don't even know what the underlying GSSAPI
implementation might do.  It is not inconceivable that GSSAPI may one day
implement NTLM via SPNEGO.  Perhaps it'll have its own way of knowing the user's
logon.  We shouldn't depend on the fact that GSSAPI only provides Kerberos via
SPNEGO.

This gets back to my reasons for filing the bug on implementing SPNEGO inside
Mozilla.  Clearly that has the advantage of allowing Mozilla to know what
protocols are being used since it would be responsible for selecting the protocols.

So, unless we write our own SPNEGO implementation, I think we have to assume
that all domains are untrusted by default.


> Maybe we could leave all.js blank and set the default in the C code, or else 
> we can start to discuss a gui layout, I'll do my best to help with it if it 
> means we could get in in time of the next release.

We need to get Ben Goodger involved with this soon if we want to get some UI
added to Firefox 1.0.

Comment 30

[Christopher Nebergall]

Bug 249942 created for GUI issues.  I've got a couple of ideas I'm attaching to
that bug.

Comment 31

[Christopher Nebergall]

Comment on attachment 152203 [details] [diff] [review]
v1.3 patch

SSPI works fine using both NTLM and Kerberos. 

The area I'm not sure of is your Makefile.in changes. In my first attempt I
didn't get Negotiate support functioning without FORCE_SHARED_LIBS = 1 but it
could be my build environment. I'm recompiling now and will check it in the
morning. But provided you've tried this and got it to succeed r=cneberg

Also does
SHORT_LIBNAME need defined?

Comment 32

[Christopher Nebergall]

Comment on attachment 152207 [details] [diff] [review]
changes to all.js

Looks good

Comment 33

[Darin Fisher]

> The area I'm not sure of is your Makefile.in changes. In my first attempt I
> didn't get Negotiate support functioning without FORCE_SHARED_LIBS = 1 but it
> could be my build environment. I'm recompiling now and will check it in the
> morning. But provided you've tried this and got it to succeed r=cneberg

Interesting... the patch definitely does work fine under Win2k.  I tested it
against a Win2k3 server.


> Also does
> SHORT_LIBNAME need defined?  

Good question.  I think that might only be necessary for OS/2, but perhaps I
should use it for Win32 as well since we certainly define USE_SHORT_LIBNAME in
configure.in for Win32 builds.

Comment 34

[Christopher Nebergall]

(In reply to comment #33)
> > The area I'm not sure of is your Makefile.in changes. In my first attempt I
> > didn't get Negotiate support functioning without FORCE_SHARED_LIBS = 1 but it
> > could be my build environment. I'm recompiling now and will check it in the
> > morning. But provided you've tried this and got it to succeed r=cneberg
> 
> Interesting... the patch definitely does work fine under Win2k.  I tested it
> against a Win2k3 server.

My build completed overnight and worked fine with no changes.

Comment 35

[Jari Ahonen]

Patch v1.3 built with Mozilla 1.8a1 on Windows works fine for me as well,
including the CNAME resolution.

Regarding the comment about getting this included in 1.7.1, does that mean it
would be possible to build the NegotiateAuth extension as an XPI that could be
installed on top of an existing Mozilla 1.7 installation ? I recall trying to
compile patch v1.2 with Mozilla 1.7 sources and it failed to build so I guess
the code would need to be somehow adapted to Moz 1.7.

Comment 36

[Christopher Nebergall]

(In reply to comment #35)
> Patch v1.3 built with Mozilla 1.8a1 on Windows works fine for me as well,
> including the CNAME resolution.
> 
> Regarding the comment about getting this included in 1.7.1, does that mean it
> would be possible to build the NegotiateAuth extension as an XPI that could be
> installed on top of an existing Mozilla 1.7 installation ? I recall trying to
> compile patch v1.2 with Mozilla 1.7 sources and it failed to build so I guess
> the code would need to be somehow adapted to Moz 1.7.

Not without backporting the patch.  This patch uses updates to the nsIauthModule
API not in Mozilla 1.7 or Firefox 9.1.

http://lxr.mozilla.org/mozilla/source/netwerk/base/public/nsIAuthModule.idl
http://lxr.mozilla.org/mozilla1.7/source/netwerk/base/public/nsIAuthModule.idl

Comment 37

[Darin Fisher]

I am hoping to get approval to land this patch along with the interface changes
onto the 1.7 and aviary-1.0 branches.

Comment 38

[Darin Fisher]

Comment on attachment 152203 [details] [diff] [review]
v1.3 patch

requesting approval to land this patch for 1.8a2.  it's important to maximize
field testing for this patch.

it would also be ideal to get this on the 1.7 branch since that will be a long
lived branch, which many enterprise customers will be using.

Comment 39

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 152203 [details] [diff] [review]
v1.3 patch

Agreed -- test coverage and feedback from the field for this sort of thing is
what alphas are all about. a=shaver for 1.8a2

Comment 40

[Darin Fisher]

fixed-on-trunk for mozilla 1.8 alpha 2

Comment 41

[Bill Gianopoulos [:WG9s]]

In reply to comment #25

> By the way - howcome "about:prefs" doesn't work in Firefox??  How is one
> supposed to edit all of the hidden preferences without knowing what
> they are in the first place?

because it is about:config.

Comment 42

[Mike Kaply [:mkaply]]

Comment on attachment 152203 [details] [diff] [review]
v1.3 patch

a=mkaply for 1.7.2

We want this on the stable branch for "corporate" users

Comment 43

[Darin Fisher]

fixed1.7.2 and fixed-aviary1.0, which entailed landing the patches in this bug
as well as the required changes to nsIAuthModule that occured in bug 241124.

Comment 44

[Michael Lefevre]

fixed-aviary1.0 is available as a real keyword now :)

Comment 45

[Jari Ahonen]

Now that this is fixed on 1.7 branch, should it now be possible to build a 1.7
branch build and selectively copy certain files over an existing 1.7
installation to get it to support Negotiate ? As long as the number/size of
files needed is low, this would be easier than having to install a completely
new version just to get NegotiateAuth support.

Searching for nsIAuthModule in lxr.mozilla.org would suggest that this could be
done but I'm not entirely sure of side-effects. Anyone willing to guess what
will happen ?

Comment 46

[Jari Ahonen]

(In reply to comment #45)
Answering to myself. I've been successful in getting Negotiate to work with an
already-installed 1.7.1 by replacing msgbsutl.dll, necko.dll and pipnss.dll with
versions from 1.7 branch build and installing negotiateauth.dll. Needed to run
regxpcom before Negotiate would work.

Comment 47

[Darin Fisher]

> Answering to myself. I've been successful in getting Negotiate to work with an
> already-installed 1.7.1 by replacing msgbsutl.dll, necko.dll and pipnss.dll with
> versions from 1.7 branch build and installing negotiateauth.dll. Needed to run
> regxpcom before Negotiate would work.

yes, that will work.  you might want to update necko.xpt since it makes
reference to the nsIAuthModule interface which changed in the newer 1.7 builds.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The xpinstall/packager/packages-win change that was made citing this bug uses
forward-slashes instead of backslashes.  I'm not sure if that's a problem.

Comment 49

[Darin Fisher]

Attachment: use backslashes instead

thanks dbaron!

Comment 50

[Darin Fisher]

correction to slashes checked in.

Comment 51

[Darin Fisher]

Comment on attachment 155659 [details] [diff] [review]
use backslashes instead

this doesn't appear to have caused any actual problems.

Comment 52

[Mike Kaply [:mkaply]]

Comment on attachment 155659 [details] [diff] [review]
use backslashes instead

a=mkaply

Comment 53

[Darin Fisher]

> a=mkaply

fixed1.7.3