Closed Bug 520607 Opened 11 years ago Closed 11 years ago

Remove use of "ntlm" auth module and replace with use of "sys-ntlm".

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta4-fixed

People

(Reporter: jimm, Assigned: jimm)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(2 files, 7 obsolete files)

See bug 487872 for details. A windows security patch added reflection attack coverage for applications that use the SSPI credential api when authenticating through NTLM. (Users who are are using the updated nsAuthSSPI for single sign-on through network.automatic-ntlm-auth preferences have this coverage.)  We also need to be using SSPI for NTLM authentication in cases where the server host is not trusted, which requires dropping use of the "ntlm" (nsNTLMAuthModule) auth module and instead relying on "sys-ntlm" (nsAuthSSPI).

http://msdn.microsoft.com/en-us/library/aa374731%28VS.85%29.aspx#sspi_functions
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: P3 → P2
Flags: wanted1.9.2?
Attached patch rfc patch v.1 (obsolete) — Splinter Review
This is rough draft of what might accomplish this. However, there are some outstanding questions - 

1) NTLM weak password hash issues. The patch includes some comments by Darin back in 2003 about why we didn't turn this on for all web sites that issue an NTLM challenge. We want to use AuthSSPI now to gain reflection attack coverage, but that may then expose the user to the weak hash risk which was the reason for not turning broad support on in the first place.

2) Use of nsHttpNTLMAuth logic for platforms that don't have built in SSPI? Are we going to continue to support our internal version on platform like Mac or should we disable it.
Attached patch sys-ntlm patch v.1 (obsolete) — Splinter Review
Attachment #408427 - Attachment is obsolete: true
Comment on attachment 410847 [details] [diff] [review]
sys-ntlm patch v.1

This adds a pref for fallback to the generic ntlm implementation we have. On windows this is set to false, making sspi the default.
Attachment #410847 - Flags: review?(wtc)
Attachment #410847 - Flags: review?(wtc)
The patch in bug 230351 has a flaw - inBufLen (passed into GetNextToken/InitializeSecurityContext) is set improperly in cases where we trim off '=' from the encoded challenge. This in turn can cause failures when connecting to IIS servers. I found this while testing this patch. (Chrome's code is ok, their base64 decoder returns an output length.)

This may also be the cause of a number of NTLM bugs we currently have open.
Attachment #410847 - Attachment is obsolete: true
Looks like clearing the buffer before decoding addresses the problem as well.
(In reply to comment #5)
> Looks like clearing the buffer before decoding addresses the problem as well.

Actually, I take that back. The length on the decoded challenge buffer must be accurate. Looks like we're going to need a new version of PL_Base64Decode that returns a length of the decoded buffer.

Also, Chrome might have the same problem, they appear to calculate a results buffer similarly, and it doesn't look like the return length from Base64Decode is accurate in all cases.
Attached patch sys ntlm patch v.2 (obsolete) — Splinter Review
It wasn't the buffer length math, we were changing the length after we calculated inBufLen!

wtc, this addresses everything on trunk for the browser. I'll work up separate patches for mailnews and other branches to address the problem with the patch in bug 230351.
Attachment #411003 - Flags: review?(wtc)
Attached patch sys ntlm patch v.3 (obsolete) — Splinter Review
Same patch with an overhaul of the comments and logging. I've also moved the main pref up to the auth branch, which seemed a better fit.
Attachment #411003 - Attachment is obsolete: true
Attachment #411020 - Flags: review?(wtc)
Attachment #411003 - Flags: review?(wtc)
Attached patch sys ntlm patch v.4 (obsolete) — Splinter Review
One last update after some more testing, this fixes a problem with the last patch where if network.auth.use-generic-ntlm was true, we weren't prompting for credentials.
Blocks: 423758
Blocks: 468334
Comment on attachment 411020 [details] [diff] [review]
sys ntlm patch v.3

[sorry for the bug spam, moving this request to the final patch.]
Attachment #411020 - Flags: review?(wtc)
Attachment #411060 - Flags: review?(wtc)
Attachment #411020 - Attachment is obsolete: true
wtc, any chance for a review before the freeze? (or any suggestions on who else could review).
(In reply to comment #1)
>
> 2) Use of nsHttpNTLMAuth logic for platforms that don't have built in SSPI? Are
> we going to continue to support our internal version on platform like Mac or
> should we disable it.

On non-Windows platforms, we have to continue to support our
internal version of NTLM.  Until someone adds NTLM to gssapi,
our internal version of NTLM is the only NTLM implementation
we have for non-Windows platforms.
(In reply to comment #6)
> Looks like we're going to need a new version of PL_Base64Decode that
> returns a length of the decoded buffer.

After studying the code in question, I agree that the API
of PL_Base64Decode is error-prone as it puts the burden of
calculating the output size on the caller.

I will check Chromium's copy of this code.  Thank you for
catching this bug.
Attachment #411060 - Flags: review?(wtc) → review+
Comment on attachment 411060 [details] [diff] [review]
sys ntlm patch v.4

Sorry about the delay in review.  Unless you know a Mozilla
developer who knows NTLM, I suggest that you ask biesi or bz
(the owner and peer of the network module) for a second review.

In the interest of time, I'm giving you r=wtc with required and
suggested changes.

Required changes:

1. Please remove the network.auth.use-generic-ntlm preference and
the UseGenericNTLM function.  On Windows, we should always use
sys-ntlm; don't provide a preference to override this.  On non-Windows
platforms, we can only use our generic ntlm module.

2. Some of your new code uses Mozilla's standard indentation level
of 2, but the network code uses an indentation level of 4.  Please
follow the local convention.  Specifically, the code with wrong
indentation level is the code that sets |ai| and |pai| and the code
that logs the return value |rc| of InitializeSecurityContext, both
in extensions/auth/nsAuthSSPI.cpp.

Suggested changes:

Make as many of the following suggested changes as you see fit.
You can make these changes in a follow-up patch.

In extensions/auth/nsAuthSSPI.cpp:

>+#ifdef DEBUG_jimm
>+char* DbgLogLine(char* psz, const BYTE* pbuf, int cb) {

In general I don't like checking in DEBUG_user code.  I'll
allow it here if you think it's useful.

This function should take the size of the |psz| output buffer
as an argument, and use snprintf instead of sprintf.

>+    const BYTE* p = pbuf;
>+	  int i;

The indentation of 'int i' looks wrong.  This could be due to
the use of TABs.  I suggest changing the TABs to spaces.
There is another occurrence of 'int i' in the DbgLogToken
function that should also be fixed.

>+    char* psz = new char[cch+10];

Nit: add a space before and after the '+' sign.

>+    // If we're configured for SPNEGO (Negotiate), Kerberos, or NTLM, it's critical 
>+    // that the caller supply a service name to be used. (For NTLM changes, see bug
>+    // 487872.)

Since "SPNEGO (Negotiate), Kerberos, or NTLM" covers all the auth schemes
this module supports, this comment can omit the list and just say:
  The caller must supply a service name to be used. (For why we now require
  a service name for NTLM, see bug 487872.)

Re: the term "single sign-on"

I kind of prefer the more accurate term "default credentials" or
"cached credentials".

>+    if (domain && username && password) {
>+      ai.Domain = (unsigned short *)domain;
>+      ai.DomainLength = wcslen(domain);
>+      ai.User = (unsigned short *)username;
>+      ai.UserLength = wcslen(username);
>+      ai.Password = (unsigned short *)password;
>+      ai.PasswordLength = wcslen(password);
>+      ai.Flags = SEC_WINNT_AUTH_IDENTITY_UNICODE;
>+      pai = &ai;
>+    }

Fix the indentation (a required change noted above).

We may want to allow |domain| to be NULL.

The (unsigned short *) casts should be removed.
PRUnichar should be equivalent to unsigned short.

>+#ifdef PR_LOGGING
>+        if (rc == SEC_E_OK)
>+          LOG(("InitializeSecurityContext: succeeded.\n"));
>+        else
>+          LOG(("InitializeSecurityContext: continue.\n"));
>+#endif

I think you can remove #ifdef PR_LOGGING.  Hopefully
the compiler can optimize away an if-else statement
with empty bodies.

Fix the indentation (a required change noted above).

In extensions/auth/nsAuthSSPI.h:

>-    nsAuthSSPI(pType package = PACKAGE_TYPE_NEGOTIATE);
>+    nsAuthSSPI(pType package = PACKAGE_TYPE_NTLM);

Why did you change the default value for the |package|
argument?  I don't know if we ever rely on the default
value, but the Negotiate scheme is more secure than the
NTLM scheme, so PACKAGE_TYPE_NEGOTIATE is a more
appropriate default.

In modules/libpref/src/init/all.js:

>+pref("network.auth.use-generic-ntlm", false);

Remove this preference (a required change noted above).

>-// This preference controls whether or not the LM hash will be included in
>-// response to a NTLM challenge.  By default, this is disabled since servers
>+// This preference controls whether or not the LM hash will be included in response
>+// to a generic NTLM challenge.  By default, this is disabled since servers
> // should almost never need the LM hash, and the LM hash is what makes NTLM
> // authentication less secure.  See bug 250691 for further details.
> // NOTE: automatic-ntlm-auth which leverages the OS-provided NTLM
> //       implementation will not be affected by this preference.

This change is not necessary because the NOTE at the end of the comment
block already explains that this preference doesn't affect sys-ntlm.

In netwerk/protocol/http/src/nsHttpNTLMAuth.cpp:

>+static const char kUseGeneric[]  = "network.auth.use-generic-ntlm";

Remove this.

>+UseGenericNTLM()

Remove this function.  Update its caller as if this function
were the PR_FALSE constant.

Re: the term "internal ntlm auth module"

You call it "internal" in some places and "generic" in others.
Would be nice to be consistent.

>+        // strip off any padding (see bug 230351)
>+        while (challenge[len - 1] == '=')
>+          len--;

We should check that len % 1 != 1 after removing the trailing '='
characters.
(In reply to comment #6)
>
> Also, Chrome might have the same problem, they appear to calculate a results
> buffer similarly, and it doesn't look like the return length from Base64Decode
> is accurate in all cases.

Here is the Chrome code:

    // Decode |auth_data_| into the input buffer.
    int len = auth_data_.length();

    // Strip off any padding.
    // (See https://bugzilla.mozilla.org/show_bug.cgi?id=230351.)
    //
    // Our base64 decoder requires that the length be a multiple of 4.
    while (len > 0 && len % 4 != 0 && auth_data_[len - 1] == '=')
      len--;
    auth_data_.erase(len);

    if (!Base64Decode(auth_data_, &decoded_auth_data))
      return std::string();  // Improper base64 encoding
    in_buf_len = decoded_auth_data.length();
    in_buf = decoded_auth_data.data();

Chrome only strips the extraneous '=' characters and keeps the
'=' characters used by a correct base64 encoder to pad the
output size to a multiple of 4.

Base64Decode returns the output size via the length of the
decoded_auth_data, a std::string.  Unless the modp_b64_decode
function has a bug, Base64Decode should return an accurate
output length in all cases.  Did you find a bug?
(In reply to comment #14)
> (From update of attachment 411060 [details] [diff] [review])
> 1. Please remove the network.auth.use-generic-ntlm preference and
> the UseGenericNTLM function.  On Windows, we should always use
> sys-ntlm; don't provide a preference to override this.  On non-Windows
> platforms, we can only use our generic ntlm module.

I debated shutting it off entirely. I'm wondering though if giving people the option of falling back if say, something on their network has been tweaked to work with our internal implementation but doesn't work with Microsoft's without changes. If you feel shutting it off entirely is low risk, sounds good with me, I'll take this out.
  
> In extensions/auth/nsAuthSSPI.cpp:
> 
> >+#ifdef DEBUG_jimm
> >+char* DbgLogLine(char* psz, const BYTE* pbuf, int cb) {

This was really for my personal debugging. It's saved here and I have it locally so I think I'll just pull it. It wasn't meant to be part of the review.

> >+#ifdef PR_LOGGING
> >+        if (rc == SEC_E_OK)
> >+          LOG(("InitializeSecurityContext: succeeded.\n"));
> >+        else
> >+          LOG(("InitializeSecurityContext: continue.\n"));
> >+#endif
> 
> I think you can remove #ifdef PR_LOGGING.  Hopefully
> the compiler can optimize away an if-else statement
> with empty bodies.

This is a semi-used convention in network code, was just fallowing suit. Will remove.

Will update the rest and repost.
(In reply to comment #15)
> 
> Chrome only strips the extraneous '=' characters and keeps the
> '=' characters used by a correct base64 encoder to pad the
> output size to a multiple of 4.
> 
> Base64Decode returns the output size via the length of the
> decoded_auth_data, a std::string.  Unless the modp_b64_decode
> function has a bug, Base64Decode should return an accurate
> output length in all cases.  Did you find a bug?

No I didn't, should have been more clear. the base 64 encoding wasn't the problem, it was our placement of the stripping code.
(In reply to comment #16)
>
> > I think you can remove #ifdef PR_LOGGING.  Hopefully
> > the compiler can optimize away an if-else statement
> > with empty bodies.
> 
> This is a semi-used convention in network code, was just fallowing suit. Will
> remove.

If it's a convention in network code, please follow suit.

Re: the network.auth.use-generic-ntlm preference: I see.
I can see its value as a debugging tool for NTLM bugs
when we switch over to sys-ntlm.  If we keep it, please
file a bug to have it removed when your new code has
been tested for a while.
(In reply to comment #14)
> >+        // strip off any padding (see bug 230351)
> >+        while (challenge[len - 1] == '=')
> >+          len--;
> 
> We should check that len % 1 != 1 after removing the trailing '='
> characters.

What were you checking for here? Did you mean len % 2? After we strip off the padding though len % 2 will fail in some cases.
Attached patch sys ntlm patch v.5 (obsolete) — Splinter Review
Attachment #411060 - Attachment is obsolete: true
Attachment #412982 - Flags: superreview?(bzbarsky)
Comment on attachment 412982 [details] [diff] [review]
sys ntlm patch v.5

I really don't feel qualified to review this.  :( Jason, do you?
Attachment #412982 - Flags: review?(jduell.mcbugs)
Comment on attachment 412982 [details] [diff] [review]
sys ntlm patch v.5

bz: OK, I will review patch v.5 carefully tomorrow.  Then
one of you just need to do a superficial/rubberstamp review.
(In reply to comment #22)
> (From update of attachment 412982 [details] [diff] [review])
> bz: OK, I will review patch v.5 carefully tomorrow.  Then
> one of you just need to do a superficial/rubberstamp review.

The only review comments not addressed were the castings for SEC_WINNT_AUTH_IDENTITY, which didn't like the incoming const variables, and that comment of yours about len. Other than that, this is compat with your previous review.
Comment on attachment 412982 [details] [diff] [review]
sys ntlm patch v.5

r=biesi pending a reply to the linux question below

+++ b/extensions/auth/nsAuthSSPI.cpp
+    SEC_WINNT_AUTH_IDENTITY_W ai, *pai = nsnull;

please make that two declarations. I find mixing pointer and non-pointer declarations in the same statement hard to read.

+            ai.Domain = (PRUnichar *)domain;

here and for the other members please use C++-style casts (const_cast<PRUnichar*>(domain))

+++ b/netwerk/protocol/http/src/nsHttpNTLMAuth.cpp
+#ifdef XP_WIN
+        if (!useGeneric && !module) {
+            LOG(("No native ntlm auth modules available.\n"));

why the ifdef? Shouldn't the pref behave the same way on windows and non-windows? And if it shouldn't, why bother defining it in prefs.js?

-        // if the continuationState is null, then we may want to try using
-        // the SSPI NTLM module.  however, we need to take care to only use
-        // that module when speaking to a trusted host.  because the SSPI
-        // may send a weak LMv1 hash of the user's password, we cannot just
-        // send it to any server.

why did you delete this comment? It seems still correct and useful.

+                module = do_CreateInstance(NS_AUTH_MODULE_CONTRACTID_PREFIX "sys-ntlm");
+                *identityInvalid = PR_TRUE;

Have you tested this on Linux?
Attachment #412982 - Flags: review+
Contrary to comment 14, we do have an implementation of sys-ntlm on non-Windows platforms. See:
http://mxr.mozilla.org/mozilla-central/source/extensions/auth/nsAuthSambaNTLM.cpp
and
bug 362435
(In reply to comment #24)
> +++ b/netwerk/protocol/http/src/nsHttpNTLMAuth.cpp
> +#ifdef XP_WIN
> +        if (!useGeneric && !module) {
> +            LOG(("No native ntlm auth modules available.\n"));
> 
> why the ifdef? Shouldn't the pref behave the same way on windows and
> non-windows? And if it shouldn't, why bother defining it in prefs.js?

On windows, unless specifically requested through prefs, we want to fail if sys-ntlm fails. We do not want to fall back the internal ntlm module at all.

> 
> +                module = do_CreateInstance(NS_AUTH_MODULE_CONTRACTID_PREFIX
> "sys-ntlm");
> +                *identityInvalid = PR_TRUE;
> 
> Have you tested this on Linux?

No I haven't. I didn't realize we had a sys-ntlm module for linux. Since the linux module doesn't support domain/user/pass input, an #ifdef XP_WIN around the second part where we set *identityInvalid to true would address this.
(In reply to comment #26)
> (In reply to comment #24)
> > +++ b/netwerk/protocol/http/src/nsHttpNTLMAuth.cpp
> > +#ifdef XP_WIN
> > +        if (!useGeneric && !module) {
> > +            LOG(("No native ntlm auth modules available.\n"));
> > 
> > why the ifdef? Shouldn't the pref behave the same way on windows and
> > non-windows? And if it shouldn't, why bother defining it in prefs.js?
> 
> On windows, unless specifically requested through prefs, we want to fail if
> sys-ntlm fails. We do not want to fall back the internal ntlm module at all.

I know. My question wasn't why the block was there, my question was why you put it inside an ifdef.

> No I haven't. I didn't realize we had a sys-ntlm module for linux. Since the
> linux module doesn't support domain/user/pass input, an #ifdef XP_WIN around
> the second part where we set *identityInvalid to true would address this.

Yeah, that seems best. Perhaps it'd be better to fix nsAuthSamba too at some point to fix the problem on linux as well, though.
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #24)
> > > +++ b/netwerk/protocol/http/src/nsHttpNTLMAuth.cpp
> > > +#ifdef XP_WIN
> > > +        if (!useGeneric && !module) {
> > > +            LOG(("No native ntlm auth modules available.\n"));
> > > 
> > > why the ifdef? Shouldn't the pref behave the same way on windows and
> > > non-windows? And if it shouldn't, why bother defining it in prefs.js?
> > 
> > On windows, unless specifically requested through prefs, we want to fail if
> > sys-ntlm fails. We do not want to fall back the internal ntlm module at all.
> 
> I know. My question wasn't why the block was there, my question was why you put
> it inside an ifdef.

For windows this is absolute, we do not fall back. For other platforms, I assume we want to fall back to the internal impl.. (Unless we want to make the decision absolute on all platforms? But then ntlm is going to break for anyone who doesn't have os support, and they'll likely have a hard time figuring out about the pref.

I'm attaching a new patch that sets the pref to false for everyone, so we run through this regardless on all platforms.

> > No I haven't. I didn't realize we had a sys-ntlm module for linux. Since the
> > linux module doesn't support domain/user/pass input, an #ifdef XP_WIN around
> > the second part where we set *identityInvalid to true would address this.
> 
> Yeah, that seems best. Perhaps it'd be better to fix nsAuthSamba too at some
> point to fix the problem on linux as well, though.

I'll file a bug. Does that module work on osx as well?
Attached patch sys ntlm patch v.6 (obsolete) — Splinter Review
Attachment #412982 - Attachment is obsolete: true
Attachment #413206 - Flags: superreview?(bzbarsky)
Attachment #413206 - Flags: review?(cbiesinger)
Attachment #412982 - Flags: superreview?(bzbarsky)
Attachment #412982 - Flags: review?(jduell.mcbugs)
(In reply to comment #24)
> -        // if the continuationState is null, then we may want to try using
> -        // the SSPI NTLM module.  however, we need to take care to only use
> -        // that module when speaking to a trusted host.  because the SSPI
> -        // may send a weak LMv1 hash of the user's password, we cannot just
> -        // send it to any server.
> 
> why did you delete this comment? It seems still correct and useful.

We are now using NTLM with both default credentials and user supplied credentials by default. Plus, generally, I don't think many servers still use LMv1, most should have upgraded.
(In reply to comment #28)

Oh, I see that I didn't really understand the purpose of the pref. It is in fact a decision whether to try the default-credentials. At first it seemed to me like a decision whether to fall back to generic NTLM.


> > Yeah, that seems best. Perhaps it'd be better to fix nsAuthSamba too at some
> > point to fix the problem on linux as well, though.
> 
> I'll file a bug. Does that module work on osx as well?

If you have a samba server installed with ntlm_auth in your path...
Comment on attachment 413206 [details] [diff] [review]
sys ntlm patch v.6

+#ifdef XP_WIN
+            else {
+                // Try to use native NTLM and prompt the user for their domain,
+                // username, and password. (only supported by windows nsAuthSSPI module.)
+                module = do_CreateInstance(NS_AUTH_MODULE_CONTRACTID_PREFIX "sys-ntlm");
+                *identityInvalid = PR_TRUE;
+            }
+#endif
 #ifdef PR_LOGGING
             if (!module)
-                LOG(("failed to load sys-ntlm module\n"));
+                LOG(("Failed to initialize a native sys-ntlm auth module.\n"));
 #endif

That log output is, I think, not entirely helpful on Linux because you don't know if it failed because we didn't try or if it failed because sys-ntlm isn't available...
Attachment #413206 - Flags: review?(cbiesinger) → review+
(In reply to comment #30)
> (In reply to comment #24)
> > -        // if the continuationState is null, then we may want to try using
> > -        // the SSPI NTLM module.  however, we need to take care to only use
> > -        // that module when speaking to a trusted host.  because the SSPI
> > -        // may send a weak LMv1 hash of the user's password, we cannot just
> > -        // send it to any server.
> > 
> > why did you delete this comment? It seems still correct and useful.
> 
> We are now using NTLM with both default credentials and user supplied
> credentials by default. Plus, generally, I don't think many servers still use
> LMv1, most should have upgraded.

The point of the comment is that you don't want to send the current user's password to the server without prompting when it's encrypted weakly. The point being, the server can then decrypt the password. So it doesn't help you at all that most servers have disabled LMv1, because the attacker's server wouldn't have it disabled.

Basically this comment explains why the UseDefaultCredentials function exists, so I think it should stay.
(In reply to comment #33)
> (In reply to comment #30)
> The point of the comment is that you don't want to send the current user's
> password to the server without prompting when it's encrypted weakly. The point
> being, the server can then decrypt the password. So it doesn't help you at all
> that most servers have disabled LMv1, because the attacker's server wouldn't
> have it disabled.
> 
> Basically this comment explains why the UseDefaultCredentials function exists,
> so I think it should stay.

Ok, I'll put something in about this.
Changes:
- touched up logging output
- added comment about the risks of LMv1 w/user supplied credentials
- changed the name of the pref from "use-generic-ntlm" to "force-generic-ntlm" so it better represent what it does. updated the pref comment.

I'll file some bugs off here in bit about removing the pref down the road (wtc), fixing samba (biesi), and addressing the mailnews trim token code that landed with bug 230351.
Attachment #413206 - Attachment is obsolete: true
Attachment #413374 - Flags: superreview?(bzbarsky)
Attachment #413206 - Flags: superreview?(bzbarsky)
Comment on attachment 413374 [details] [diff] [review]
sys ntlm patch v.7

Jim: I will review your patch v.7 before noon US Pacific Time.
Feel free to check it in if you get the necessary superreviews.
Comment on attachment 413374 [details] [diff] [review]
sys ntlm patch v.7

sr=bzbarsky

Please file a followup bug to create a sane api wrapping the insanity that is PL_Base64Decode?
Attachment #413374 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #37)
> (From update of attachment 413374 [details] [diff] [review])
> sr=bzbarsky
> 
> Please file a followup bug to create a sane api wrapping the insanity that is
> PL_Base64Decode?

will do.

pushed to try one final time, will land this here in a bit.
Comment on attachment 413374 [details] [diff] [review]
sys ntlm patch v.7

r=wtc.  I have some suggested changes.  In the interest of time,
you can make the more complicated changes later.

In extensions/auth/nsAuthSSPI.cpp:

>+    //  The caller must supply a service name to be used. (For why we now require
>+    //  a service name for NTLM, see bug 487872.)
>     NS_ENSURE_TRUE(serviceName && *serviceName, NS_ERROR_INVALID_ARG);

Nit: use only one space after //.

>+    SEC_WINNT_AUTH_IDENTITY_W *pai = nsnull;
...
>+        else {
>+            ai.Domain = nsnull;
>+            ai.DomainLength = 0;
>+        }

Nit: this file uses NULL instead of nsnull.  You may want to continue
that convention.

>+            ai.Domain = const_cast<PRUnichar*>(domain);
...
>+        ai.User = const_cast<PRUnichar*>(username);
...
>+        ai.Password = const_cast<PRUnichar*>(password);

Nit: consider using const_cast<unsigned short*> because unsigned short
is the type of ai.{Domain,User,Password}.  When I suggested that you
remove the (unsigned short*) C-style casts, I didn't realize you were
using them to cast away the const.

In extensions/auth/nsHttpNegotiateAuth.cpp:

>+        // strip off any padding (see bug 230351)
>+        while (challenge[len - 1] == '=')
>+            len--;

I had a typo in my previous comment about this.  What I meant was that
we may want to assert or check that len % 4 != 1 after stripping off
the trailing '=' characters.

You can also consider stripping off just the extraneous '=' characters
so that |len| becomes a multiple of 4.

In modules/libpref/src/init/all.js:

Now that I remember the user comments in Chromium issue 19 (http://crbug.com/19),
I believe that most users who use Firefox to do NTLM are already using sys-ntlm,
so we don't need to add this preference.  I don't want to jerk you around, so
you can continue the course of action and remove this preference after the new
code has been tested in the field for a while.

>+// native implementation provided by the os. This pref is primarily for diagnosing

Remove "primarily" to make it clear that's the only purpose of this preference.

>+// authentication can expose the user to reflection attack vulnerabilities. Do not
>+// change this unless you know what you're doing!

Please add something like "This pref should be removed six months after Firefox 3.6
is released."

If this pref is applicable to Windows only, please document that in the comment.

In netwerk/protocol/http/src/nsHttpNTLMAuth.cpp:

>+static const char kUseGeneric[]  = "network.auth.force-generic-ntlm";

I suggest renaming kUseGeneric, UseGenericNTLM, useGeneric to
kForceGeneric, ForceGenericNTLM, forceGeneric to match the new name of the
preference.

>+// Check to see if we should use default credentials for this host or proxy.
>+static PRBool
>+UseDefaultCredentials(nsIHttpChannel *channel, PRBool isProxyAuth)

Consider renaming this function CanUseDefaultCredentials.

>     PRBool val;
>     if (isProxyAuth) {
>         if (NS_FAILED(prefs->GetBoolPref(kAllowProxies, &val)))
>             val = PR_FALSE;
>-        LOG(("sys-ntlm allowed for proxy: %d\n", val));
>+        LOG(("Default credentials allowed for proxy: %d\n", val));
>         return val;
>     }

Nit: move the declaration of |val| to the inside of the if block
because |val| is only used in that scope.

>+        PRBool ssForHost = (uri && TestPref(uri, kTrustedURIs));

Please rename ssForHost to isTrustedHost.  ("ss" comes from "single
sign-on" in the old patch.)

>+    LOG(("Default credentials not allowed for host or proxy.\n"));
>     return PR_FALSE;

Remove these two lines because they are now unreachable (the
preceding "if" and "else" blocks both end with a return statement).
Does any compiler warn about this?

>+        // through UseGenericNTLM. (We use native impl. auth by default if the

Nit: don't abbreviate "impl."

>+            if (UseDefaultCredentials(channel, isProxyAuth) && !*continuationState) {
>+
>+                // Try logging in with the user's default credentials. If
>+                // successful, |identityInvalid| is false, which will trigger
>+                // a default credentials attempt once we return.
>+                module = do_CreateInstance(NS_AUTH_MODULE_CONTRACTID_PREFIX "sys-ntlm");
> #ifdef PR_LOGGING
>-            if (!module)
>-                LOG(("failed to load sys-ntlm module\n"));
>+                if (!module)
>+                    LOG(("Native sys-ntlm auth module not found.\n"));
> #endif
>+            }
>+#ifdef XP_WIN
>+            else {
>+                // Try to use native NTLM and prompt the user for their domain,
>+                // username, and password. (only supported by windows nsAuthSSPI module.)
>+                // Note, for servers that use LMv1 a weak hash of the user's password
>+                // will be sent. We rely on windows internal apis to decide whether
>+                // we should support this older, less secure version of the protocol.
>+                module = do_CreateInstance(NS_AUTH_MODULE_CONTRACTID_PREFIX "sys-ntlm");
>+                *identityInvalid = PR_TRUE;
>+#ifdef PR_LOGGING
>+                if (!module)
>+                    LOG(("Native sys-ntlm auth module not found.\n"));
>+#endif
>+            }
>+#endif // XP_WIN

This if-else statement is hard to understand because of the #ifdef XP_WIN
around the else block.  If you stare at the code, you'll see that the if
and else blocks differ in only the
                  *identityInvalid = PR_TRUE;
statement in the "else" block.  So it should be possible to simplify this
if-else statement.  This would require always trying to instantiate the
sys-ntlm module on non-Windows platforms, which I think is fine because
we will only try it once if the sys-ntlm module doesn't exist.

In security/manager/ssl/src/nsNTLMAuthModule.cpp:

> NS_IMETHODIMP
> nsNTLMAuthModule::Init(const char      *serviceName,
>                        PRUint32         serviceFlags,
>                        const PRUnichar *domain,
>                        const PRUnichar *username,
>                        const PRUnichar *password)
> {
>-  NS_ASSERTION(serviceName == nsnull, "unexpected service name");

Just curious: why did you remove this assertion?
Attachment #413374 - Flags: review+
A search across litmus showed no hits for "ntlm"!

I'd suggest we add a couple windows litmus tests:

1) Install IIS7 on Vista or Win7, it is shipped with the os by default.
http://learn.iis.net/page.aspx/28/install-iis-7-on-windows-vista-and-windows-7/

2) add a localhost web server

3) configure Authentication for the web server:
- disable anonymous
- enable Windows Authentication
- under Windows Authentication - providers, remove everything and add NTLM

4) open firefox and browse to http://localhost/

- you should get prompted for username & password. enter your desktop login credentials. 

results: you should browse to the default IIS home page.

5) under Windows Authentication - providers, add Negotiate, and give it priority over NTLM. repeat step 4

results: you should browse to the default IIS home page.

6) open about:config, search for 'ntlm', and add:

network.automatic-ntlm-auth.trusted-uris = 'http://127.0.0.1'

7) open firefox and browse to http://127.0.0.1/

results: you should go directly to the default home page without any prompting.

(we should also add some tests for linux/osx as well, although that requires setting up samba, so not sure if that's possible.)
Flags: in-litmus?
Make sure to restart the browser every time before loading http://localhost
(In reply to comment #39)
> (From update of attachment 413374 [details] [diff] [review])
> In extensions/auth/nsAuthSSPI.cpp:
> 
> >+    //  The caller must supply a service name to be used. (For why we now require
> >+    //  a service name for NTLM, see bug 487872.)
> >     NS_ENSURE_TRUE(serviceName && *serviceName, NS_ERROR_INVALID_ARG);
> 
> Nit: use only one space after //.

fixed.

> >+    SEC_WINNT_AUTH_IDENTITY_W *pai = nsnull;
> ...
> >+        else {
> >+            ai.Domain = nsnull;
> >+            ai.DomainLength = 0;
> >+        }
> 
> Nit: this file uses NULL instead of nsnull.  You may want to continue
> that convention.

updated.

> >+            ai.Domain = const_cast<PRUnichar*>(domain);
> ...
> >+        ai.User = const_cast<PRUnichar*>(username);
> ...
> >+        ai.Password = const_cast<PRUnichar*>(password);
> 
> Nit: consider using const_cast<unsigned short*> because unsigned short
> is the type of ai.{Domain,User,Password}.  When I suggested that you
> remove the (unsigned short*) C-style casts, I didn't realize you were
> using them to cast away the const.

updated.

> 
> In extensions/auth/nsHttpNegotiateAuth.cpp:
> 
> >+        // strip off any padding (see bug 230351)
> >+        while (challenge[len - 1] == '=')
> >+            len--;
> 
> I had a typo in my previous comment about this.  What I meant was that
> we may want to assert or check that len % 4 != 1 after stripping off
> the trailing '=' characters.
> 
> You can also consider stripping off just the extraneous '=' characters
> so that |len| becomes a multiple of 4.

I'll update this in the followup set of patches for the problem in bug 230351.

> In modules/libpref/src/init/all.js:
> 
> Now that I remember the user comments in Chromium issue 19
> (http://crbug.com/19),
> I believe that most users who use Firefox to do NTLM are already using
> sys-ntlm,
> so we don't need to add this preference.  I don't want to jerk you around, so
> you can continue the course of action and remove this preference after the new
> code has been tested in the field for a while.
> 
> >+// native implementation provided by the os. This pref is primarily for diagnosing
> 
> Remove "primarily" to make it clear that's the only purpose of this preference.
> 
> >+// authentication can expose the user to reflection attack vulnerabilities. Do not
> >+// change this unless you know what you're doing!
> 
> Please add something like "This pref should be removed six months after Firefox
> 3.6
> is released."

updated.

> If this pref is applicable to Windows only, please document that in the
> comment.

It's not, samba support for linux/osx applies here as well.

> In netwerk/protocol/http/src/nsHttpNTLMAuth.cpp:
> 
> >+static const char kUseGeneric[]  = "network.auth.force-generic-ntlm";
> 
> I suggest renaming kUseGeneric, UseGenericNTLM, useGeneric to
> kForceGeneric, ForceGenericNTLM, forceGeneric to match the new name of the
> preference.
> >+// Check to see if we should use default credentials for this host or proxy.
> >+static PRBool
> >+UseDefaultCredentials(nsIHttpChannel *channel, PRBool isProxyAuth)
> 
> Consider renaming this function CanUseDefaultCredentials.

updated.

> >     PRBool val;
> >     if (isProxyAuth) {
> >         if (NS_FAILED(prefs->GetBoolPref(kAllowProxies, &val)))
> >             val = PR_FALSE;
> >-        LOG(("sys-ntlm allowed for proxy: %d\n", val));
> >+        LOG(("Default credentials allowed for proxy: %d\n", val));
> >         return val;
> >     }
> 
> Nit: move the declaration of |val| to the inside of the if block
> because |val| is only used in that scope.

updated.

> 
> >+        PRBool ssForHost = (uri && TestPref(uri, kTrustedURIs));
> 
> Please rename ssForHost to isTrustedHost.  ("ss" comes from "single
> sign-on" in the old patch.)

updated.

> >+    LOG(("Default credentials not allowed for host or proxy.\n"));
> >     return PR_FALSE;
> 
> Remove these two lines because they are now unreachable (the
> preceding "if" and "else" blocks both end with a return statement).
> Does any compiler warn about this?

updated.

> >+        // through UseGenericNTLM. (We use native impl. auth by default if the
> 
> Nit: don't abbreviate "impl."

updated.

> >+            if (UseDefaultCredentials(channel, isProxyAuth) && !*continuationState) {
> >+
> >+                // Try logging in with the user's default credentials. If
> >+                // successful, |identityInvalid| is false, which will trigger
> >+                // a default credentials attempt once we return.
> >+                module = do_CreateInstance(NS_AUTH_MODULE_CONTRACTID_PREFIX "sys-ntlm");
> > #ifdef PR_LOGGING
> >-            if (!module)
> >-                LOG(("failed to load sys-ntlm module\n"));
> >+                if (!module)
> >+                    LOG(("Native sys-ntlm auth module not found.\n"));
> > #endif
> >+            }
> >+#ifdef XP_WIN
> >+            else {
> >+                // Try to use native NTLM and prompt the user for their domain,
> >+                // username, and password. (only supported by windows nsAuthSSPI module.)
> >+                // Note, for servers that use LMv1 a weak hash of the user's password
> >+                // will be sent. We rely on windows internal apis to decide whether
> >+                // we should support this older, less secure version of the protocol.
> >+                module = do_CreateInstance(NS_AUTH_MODULE_CONTRACTID_PREFIX "sys-ntlm");
> >+                *identityInvalid = PR_TRUE;
> >+#ifdef PR_LOGGING
> >+                if (!module)
> >+                    LOG(("Native sys-ntlm auth module not found.\n"));
> >+#endif
> >+            }
> >+#endif // XP_WIN
> 
> This if-else statement is hard to understand because of the #ifdef XP_WIN
> around the else block.  If you stare at the code, you'll see that the if
> and else blocks differ in only the
>                   *identityInvalid = PR_TRUE;
> statement in the "else" block.  So it should be possible to simplify this
> if-else statement.  This would require always trying to instantiate the
> sys-ntlm module on non-Windows platforms, which I think is fine because
> we will only try it once if the sys-ntlm module doesn't exist.

I've moved the logging down so it isn't copied, but the rest I left alone. This is a tricky part of the code, i'd rather not mess with it at this point. 

> 
> In security/manager/ssl/src/nsNTLMAuthModule.cpp:
> 
> > NS_IMETHODIMP
> > nsNTLMAuthModule::Init(const char      *serviceName,
> >                        PRUint32         serviceFlags,
> >                        const PRUnichar *domain,
> >                        const PRUnichar *username,
> >                        const PRUnichar *password)
> > {
> >-  NS_ASSERTION(serviceName == nsnull, "unexpected service name");
> 
> Just curious: why did you remove this assertion?

The patch in bug 487872 sets the service name triggering this in the generic module.
Depends on: 529906
Blocks: 529907
Blocks: 529909
Attached patch pushedSplinter Review
http://hg.mozilla.org/mozilla-central/rev/275225278550
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 531425, 533529
Depends on: 542318
Looks like this cause regression bug 542318.  I asked the reporter in a private email to use an adjusted build (see bug 542318 comment 12) and switch network.auth.force-generic-ntlm to true and the problem disappeared.

I also told him to check with official FF 3.6 and the option switch, what result we get.  I'm waiting for a response atm.
Blocks: 592692
Group: core-security
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.