487872 - (CVE-2009-3983) Investigate NTLM reflection vulnerability

Status: RESOLVED FIXED

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

Description

[Brandon Sterne (:bsterne)]

Peter Allor from IBM X-Force reported that Mozilla's NTLM implementation is vulnerable to a type of "reflection attack" by which Firefox can be used to forward NTLM credentials to an arbitrary site.  I will attach the complete advisory shortly, but here are the first two paragraphs:

-----
Microsoft's MS08-068 patch addresses a SMB/NTLM vulnerability which allows an attacker to use a victim's NTLM credential in order to gain access to the victim's shared resources. This attack is often called a 'reflection attack'. The fix successfully protects a host against active exploitation by using tools such as smbrelay included in Metasploit. Similarly, part of this month's MS08-076 patch addresses instances in which a Windows Media Component fails to initialize NTLM security and thus opens an affected host to a potential reflection attack. However neither of these patches, as pointed out by HD Moore, address an attack that involves multiple targets since credential tokens used in all existing NTLM authentication protocols (i.e. LM, NTLM, and NTLMv2) can be forwarded to any host. In other words, the true vulnerability lies in the NTLM authentication's design.

This protocol flaw can be particularly troublesome since there are many applications that support NTLM.  In addition, we discovered that popular applications such as Mozilla Firefox and Opera are vulnerable to the reflection attack which involves two parties. Therefore, it is worth taking some time to discuss the internals of NTLM authentication in detail in order to understand the full extent of the vulnerability. For the rest of this article, we refer to two types of reflection attacks as two-party reflection attack and multi-host reflection attack. In a two-party reflection attack, attacker uses a stolen credential against the same victim, where as in a multi-host reflection attack, the credential is forwarded to a host other than the original victim.
-----

Comment 1

[Brandon Sterne (:bsterne)]

Attachment: NTLM relection advisory

Comment 2

[Daniel Veditz [:dveditz]]

Anyone know our NTLM code or how NTLM is supposed to work?

CC'ing Darin who may remember, and may have a similar issue in Chrome

Comment 3

[Darin Fisher]

I wrote the NTLM code for Moz.  CC'ing WTC since he is also familiar with this code.

Comment 4

[Darin Fisher]

The doc appears to recommend using InitializeSecurityContext from Secur32.dll to protect against this problem, but that is obviously not an option on Mac and Linux.  What we need to do then is understand how Microsoft's implementation protects against the two-party reflection attack and implement the same procedures.

The doc states that Chrome is not vulnerable, and that is true of Chrome 1 since it just uses WinHTTP.dll for networking, but in Chrome 2, we have a similar-to-moz custom implementation of NTLM for our network stack, which is likely also vulnerable.

Comment 5

[Wan-Teh Chang]

I'll be happy to investigate this NTLM reflection vulnerability.

Since the NTLM relection advisory that Brandon attached assumes
the reader knows what a reflection attack is, I looked up its
definition on the Web:
http://capec.mitre.org/data/definitions/90.html
http://en.wikipedia.org/wiki/Reflection_attack

Both web pages define a reflection attack with the same five
steps.  I believe a browser's role in the attack is in step 3,
and the attacker also needs to trick a browser user on the
target server to visit a site controlled by the attacker.

The NTLM relection advisory mentions using an active challenge
table as a local reflection attack protection.  It seems that
this table needs to be computer-wide, rather than local to a
browser.  If this is true, then this protection requires a
system service/daemon to maintain the active challenge table.
This will be problematic for our cross-platform NTLM auth
implementation.

Comment 6

[Daniel Veditz [:dveditz]]

Taking a stab at "sg:high" based on the MS08-068 rating of "Important"

Here's the related MS advisory for convenience:
http://www.microsoft.com/technet/security/bulletin/Ms08-068.mspx

And the H.D.Moore article linked from the .doc attachment in this bug
http://blog.metasploit.com/2008/11/ms08-067-metasploit-and-smb-relay.html

Comment 7

[Takehiro Takahashi]

Hi, 

I'm Takehiro from IBM X-Force, and I'm the original author of the paper Peter Allor
had sent to Brandon.

As Wan-Teh points out, Mozilla itself won't be able to provide a sufficient coverage
for the local NTLM reflection attack on non-windows platforms, 
and there must be a system-wide NTLM challenge table which would work with
applications such as Samba or Apache.

At this point, I do recommend implementing the NTLM authentication module 
using InitializeSecurityContext API call for the Windows platform. 
However, there is really no quick and easy way to address the same issue for other platforms.
Perhaps, you could create a hook which could be used for easy challenge-passing
when someone decides to start an open-source active NTLM-challenge repository project in the future.

Cheers,
-tak

Comment 8

[Samuel Sidler (old account; do not CC)]

Wan-Teh: Are you still investigating this?

Comment 9

[Takehiro Takahashi]

FYI, we'll be publishing my paper as a blog post in next few weeks.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Dan, not to mince rating words, but do we really think that this is sg:high based on the social hack that Wan-Teh believes will be required in comment 5?

Tak - that's ... unfortunate, as you'll essentially be publishing an exploit which affects all major browsers, won't you?

Comment 11

[Wan-Teh Chang]

Takehiro: Thanks for the heads-up on your plan to publish your
paper.  Please give us some more time to investigate this.  I'd
also appreciate your advice on the following issues.

1. Since we don't have a practical solution for non-Windows
platforms, should we recommend that users not use browsers
on computers running servers that do NTLM authentication
to avoid this attack?  Any other precautions the user can
take?

2. For Windows, Firefox calls InitializeSecurityContext only when
it is fine to do automatic logon with NTLM.  This decision is
controlled by two Firefox preferences:
  network.automatic-ntlm-auth.allow-proxies
  network.automatic-ntlm-auth.trusted-uris

To implement your suggested fix of always calling InitializeSecurityContext,
we need to find out a way for InitializeSecurityContext to use
caller-supplied username and password.  Do you know how to do
that?  I guess that we can pass the username and password in
a SEC_WINNT_AUTH_IDENTITY structure to the AcquireCredentialsHandle
function:
http://msdn.microsoft.com/en-us/library/aa380131(VS.85).aspx
http://msdn.microsoft.com/en-us/library/aa374715(VS.85).aspx

You can look at our InitializeSecurityContext code in
http://mxr.mozilla.org/firefox/source/extensions/auth/nsAuthSSPI.h
http://mxr.mozilla.org/firefox/source/extensions/auth/nsAuthSSPI.cpp

Comment 12

[Samuel Sidler (old account; do not CC)]

Jason: Wan-Teh tells me his comment 11 is accurate (i.e., no more "I guess"), but he doesn't have time to work on this bug. Can you work on implementing that?

Comment 13

[Takehiro Takahashi]

Mike,
The reason we wanted to publish our paper soon was two fold:
1) we've finished implementing our coverage for the multi-party reflection attack, and our customers are protected as of Aug 11th, 2009.
2) the bug is at least 4months old, and there had been little progress, and some vendors have just neglected the issue completely.


Wan-Teh,
Certainly, and I'm sorry for the slow reply. There has been several security issues that required our urgent attention until today.

1.
The best users can do is to not send his/her credential to a site that cannot be trusted. The reflection attack is based on password prompt from unknown sites, so no cached credential will be sent automatically. This is also true for multi-party reflection attack which no browser can provide support for - the NTLM protocol is just flawed.


2.
Based on Samuel's comment above, I supposed you've already sorted your question out.
But the following link should still help the developers that are in charge of making the change.

http://msdn.microsoft.com/en-us/library/aa918215.aspx



Finally, I just like to make it clear that, at this point,
we don't plan on making information available to public 
until Mozilla (and Chromium) has fixed the problem.


Cheers,
-takehiro

Comment 14

[Takehiro Takahashi]

Posting this comment on behalf of Peter Allor

==========================================================================
Please inform us as to what timeline you are now on.

We have another vendor who will release later this month with no details.

Regards,
peter

==========================================================================

Comment 15

[Jim Mathies [:jimm]]

(In reply to comment #11)
> To implement your suggested fix of always calling InitializeSecurityContext,
> we need to find out a way for InitializeSecurityContext to use
> caller-supplied username and password.  Do you know how to do
> that?  I guess that we can pass the username and password in
> a SEC_WINNT_AUTH_IDENTITY structure to the AcquireCredentialsHandle
> function:
> http://msdn.microsoft.com/en-us/library/aa380131(VS.85).aspx
> http://msdn.microsoft.com/en-us/library/aa374715(VS.85).aspx
> 
> You can look at our InitializeSecurityContext code in
> http://mxr.mozilla.org/firefox/source/extensions/auth/nsAuthSSPI.h
> http://mxr.mozilla.org/firefox/source/extensions/auth/nsAuthSSPI.cpp

Is this fix confirmed as the appropriate approach? I've been looking over NTLM api's trying to familiarize myself with how it works and how we use it. According to the MS attachment here - 

"InitializeSecurityContext, implemented in secur32.dll, takes a server message as a user input (pInput), makes several Local Procedure Calls (LPC) to the Local Security Authority (LSA) to process authentication, and returns a client blob in pOutput. LSA, whose executable name is known as lsass.exe, sets up a public LPC port (\LsaAuthenticationPort) and takes the first DWORD of LPC message as an index for undocumented functions. LpcAcquireCreds, LpcGetUserName, LpcLogonSessionData, and LpcLookupAccountName are examples of these functions. The LSA is responsible for a variety of tasks such as retrieving/verifying users' credentials, keeping track of logon sessions, or digesting/generating authentication blobs. It also implements a local reflection attack protection by checking an incoming NTLM challenge against a table full of active challenges currently in use. If the LSA sees a duplicate challenge, it returns an error. Our research shows that this scheme was already in place as of Windows XP SP2, contrary to speculation that it was first introduced in the November SMB/NLTM patch.

The first call to InitializeSecurityContext produces a blob used in message 1 of Figure 1. The message merely declares the host/domain name of the client. The subsequent reply from the server has a blob which contains the server challenge, and is passed to the second call to InitializeSecurityContext."

To me it seemed as if it was this second call to InitializeSecurityContext that we were (also?) missing which sets up the reflection attack protection.

I have the time to work on a fix if Jason doesn't, however I'd need some help in clarifying exactly what needs to be implemented.

Comment 16

[Samuel Sidler (old account; do not CC)]

(In reply to comment #15)
> Is this fix confirmed as the appropriate approach?

Yes, per conversations with Wan-Teh.

> I have the time to work on a fix if Jason doesn't, however I'd need some help
> in clarifying exactly what needs to be implemented.

If you need any direction, Wan-Teh has offered to help guide, he just doesn't have time to code the full fix.

Comment 17

[Wan-Teh Chang]

Jim: glad to see your comment.  I was going to email Jason
today about this bug!

The MSDN URL that Takehiro provided in comment 13 is for the
Kerberos provider.  From there I found the URL for the NTLM
provider: http://msdn.microsoft.com/en-us/library/aa923611.aspx

So, the only thing you need to change is the 5th argument to
AcquireCredentialsHandle.  Right now Firefox passes NULL to
use the default cached credentials of the user:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/auth/nsAuthSSPI.cpp&rev=1.13&mark=271,275#271

because Firefox uses the "sys-ntlm" module only for single
sign-on (no username/password dialog) when CanUseSysNTLM returns
PR_TRUE.  The fix we need to implement is to also use the
"sys-ntlm" module, with user-supplied username/password passed
in a SEC_WINNT_AUTH_IDENTITY structure as the 5th argument to
AcquireCredentialsHandle, when CanUseSysNTLM returns PR_FALSE.

If you work in the Mountain View office, I'll be happy to
come over to work with you on this fix.

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #17)
> Jim: glad to see your comment.  I was going to email Jason
> today about this bug!
> 
> The MSDN URL that Takehiro provided in comment 13 is for the
> Kerberos provider.  From there I found the URL for the NTLM
> provider: http://msdn.microsoft.com/en-us/library/aa923611.aspx
> 
> So, the only thing you need to change is the 5th argument to
> AcquireCredentialsHandle.  Right now Firefox passes NULL to
> use the default cached credentials of the user:
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/auth/nsAuthSSPI.cpp&rev=1.13&mark=271,275#271
> 
> because Firefox uses the "sys-ntlm" module only for single
> sign-on (no username/password dialog) when CanUseSysNTLM returns
> PR_TRUE.  The fix we need to implement is to also use the
> "sys-ntlm" module, with user-supplied username/password passed
> in a SEC_WINNT_AUTH_IDENTITY structure as the 5th argument to
> AcquireCredentialsHandle, when CanUseSysNTLM returns PR_FALSE.
> 
> If you work in the Mountain View office, I'll be happy to
> come over to work with you on this fix.

I'm remote actually so will post here any questions once I start working on it, which should be later this afternoon/tomorrow morning'ish!

Comment 19

[Jim Mathies [:jimm]]

Sorry for the delay, got bogged down on something else. I have this and one other 1.9.2 blocker with some strings and this which will likely get blocking approval, so I will probably do the strings one first. Will post back when i start in on this, hopefully beginning of next week.

Comment 20

[Johnny Stenback (:jst)]

Jim, any updates here yet?

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #20)
> Jim, any updates here yet?

(In reply to comment #17)
> Jim: glad to see your comment.  I was going to email Jason
> today about this bug!
> 
> The MSDN URL that Takehiro provided in comment 13 is for the
> Kerberos provider.  From there I found the URL for the NTLM
> provider: http://msdn.microsoft.com/en-us/library/aa923611.aspx
> 
> So, the only thing you need to change is the 5th argument to
> AcquireCredentialsHandle.  Right now Firefox passes NULL to
> use the default cached credentials of the user:
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/auth/nsAuthSSPI.cpp&rev=1.13&mark=271,275#271
> 
> because Firefox uses the "sys-ntlm" module only for single
> sign-on (no username/password dialog) when CanUseSysNTLM returns
> PR_TRUE.  The fix we need to implement is to also use the
> "sys-ntlm" module, with user-supplied username/password passed
> in a SEC_WINNT_AUTH_IDENTITY structure as the 5th argument to
> AcquireCredentialsHandle, when CanUseSysNTLM returns PR_FALSE.
> 
> If you work in the Mountain View office, I'll be happy to
> come over to work with you on this fix.

I've been looking at this. I see where we need the additional information for the AcquireCredentialsHandle call, but am not entirely sure when we should be asking for that information. In nsHttpNTLMAuth::ChallengeReceived we make a decision on whether or not to use sys-ntlm. In GenerateCredentials we initialize the module and pass in the required user and pass. But I still am not sure where I should be asking for this information, and under what conditions. Both of these get called from the channel - 

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#3449

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#3086

AFAICT, CanUseSysNTLM restricts the use of this to certain domains. I guess we want to request this information in a more general case before calling Init on the module. Where exactly we would do that and under what conditions I'm still unsure of.

Comment 22

[Jim Mathies [:jimm]]

Does anyone have any ideas on the user credentials in this? I'm not seeing where we should be asking for them or how we would retrieve them.

Comment 23

[Wan-Teh Chang]

Jim, sorry about the late response.

Previously, I thought using Windows SSPI is the fix.  It turns
out that you also need to pass a non-NULL service principal
name as the third argument to InitializeSecurityContext.  As
a result of this new understanding, I suggest a different
strategy.

1. We first change nsAuthSSPI.cpp so that it passes a non-NULL
'sn' to sspi->InitializeSecurityContextW() for NTLM as well:

http://mxr.mozilla.org/mozilla-central/source/extensions/auth/nsAuthSSPI.cpp#341

This may be as simple as removing 'sn' and simply passing
wSN.get() to sspi->InitializeSecurityContextW().

This simple fix will protect the users who are using the
"sys-ntlm" module for their proxy servers (the default setting)
and the servers listed in network.automatic-ntlm-auth.trusted-uris.

I believe that most users of NTLM have configured their
network.automatic-ntlm-auth.trusted-uris list, judging from
the angry user comments in Chromium issue 19 (because
Chromium doesn't support that yet: http://crbug.com/19).

2. Figure out how to use the "sys-ntlm" module for all
servers, including those not listed in
network.automatic-ntlm-auth.trusted-uris.  This will cover
the extranet servers and the users who haven't configured
their network.automatic-ntlm-auth.trusted-uris list.

Comment 24

[Jim Mathies [:jimm]]

(In reply to comment #23)
> Jim, sorry about the late response.
> 
> Previously, I thought using Windows SSPI is the fix.  It turns
> out that you also need to pass a non-NULL service principal
> name as the third argument to InitializeSecurityContext.  As
> a result of this new understanding, I suggest a different
> strategy.
> 
> 1. We first change nsAuthSSPI.cpp so that it passes a non-NULL
> 'sn' to sspi->InitializeSecurityContextW() for NTLM as well:
> 
> http://mxr.mozilla.org/mozilla-central/source/extensions/auth/nsAuthSSPI.cpp#341
> 
> This may be as simple as removing 'sn' and simply passing
> wSN.get() to sspi->InitializeSecurityContextW().
> 
> This simple fix will protect the users who are using the
> "sys-ntlm" module for their proxy servers (the default setting)
> and the servers listed in network.automatic-ntlm-auth.trusted-uris.
> 
> I believe that most users of NTLM have configured their
> network.automatic-ntlm-auth.trusted-uris list, judging from
> the angry user comments in Chromium issue 19 (because
> Chromium doesn't support that yet: http://crbug.com/19).
> 
> 2. Figure out how to use the "sys-ntlm" module for all
> servers, including those not listed in
> network.automatic-ntlm-auth.trusted-uris.  This will cover
> the extranet servers and the users who haven't configured
> their network.automatic-ntlm-auth.trusted-uris list.

How critical is #2 to this fix? We can put together two patches and land the first separate if that mitigates a large portion of the risk. I'm a little concerned # will not make the new freeze date.

Comment 25

[Jim Mathies [:jimm]]

(In reply to comment #24)

> How critical is #2 to this fix? We can put together two patches and land the
> first separate if that mitigates a large portion of the risk. I'm a little
> concerned # will not make the new freeze date.

err - '#' -> '#2'

Comment 26

[Jim Mathies [:jimm]]

Digging into this, according to the ms word write up, we lack a valid SPN. 

According to MSDN -

service principal name 

(SPN) The name by which a client uniquely identifies an instance of a service. If you install multiple instances of a service on computers throughout a forest, each instance must have its own SPN. A given service instance can have multiple SPNs if there are multiple names that clients might use for authentication.


The docs on the initialize call also add -

"A pointer to a null-terminated string that indicates the service principal name (SPN) or the security context of the destination server."

My assumption here is that the security context of the destination is NTLM, which is what ms examples use. However, I'm wondering if an instance of a mozilla client running on the desktop would need something unique here to identify it and prevent the attack. Will dig some more...

Comment 27

[Jim Mathies [:jimm]]

After doing a bit of reading, here's a recap of the process:

1) Client requests an outbound security token for communication through AcquireCredentialsHandle, passing "NTLM" as the security package type.

2) To initiate the authentication, client retrieves a security token using InitializeSecurityContext which will be sent to the server.

(The pszTargetName parameter (which is optional) for InitializeSecurityContext should be specific to the server we are communicating with and possible the resource we are retrieving.)

3) Server receives the request, and initializes it's security credentials using AcquireCredentialsHandle with the "NTLM" package name.

4) Server requests a security context for the client using AcceptSecurityContext, into which it hands pointers to data received by the client. The result of this call dictates if authentication is complete or if additional challenges need to occur through the same process.

In step #2, we pass null into the nsAuthSSPI's Init call for service name -

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpNTLMAuth.cpp#305

We need that to be populated for the call to InitializeSecurityContext. Which comes back to what Wan-Teh said in his post:

> 1. We first change nsAuthSSPI.cpp so that it passes a non-NULL
> 'sn' to sspi->InitializeSecurityContextW() for NTLM as well:
> 
> This may be as simple as removing 'sn' and simply passing
> wSN.get() to sspi->InitializeSecurityContextW().

This won't work since wSN will be empty, however, if we could fill it, that would solve this part of the problem. What exactly we fill it with is still TBD, but I believe the target could simply be the domain of the server we are communicating with, retrieved from the channel.

If anyone spots any errors in this feel free to speak up. :)

Comment 28

[Jim Mathies [:jimm]]

Attachment: sn support for trusted hosts v.1

Comment 29

[Jim Mathies [:jimm]]

Comment on attachment 404305 [details] [diff] [review]
sn support for trusted hosts v.1

Actually that's not going to work quite yet as MakeSN can't handle these service name formats. Second rev coming up.

Comment 30

[Jim Mathies [:jimm]]

Attachment: sn support for trusted hosts v.2

Comment 31

[Wan-Teh Chang]

Comment on attachment 404316 [details] [diff] [review]
sn support for trusted hosts v.2

In extensions/auth/nsAuthSSPI.cpp:

> static nsresult
>-MakeSN(const char *principal, nsCString &result)
>+MakeSN(PRUint32 aPackageType, const char *principal, nsCString &result)
> {
>     nsresult rv;
> 
>+    if (aPackageType == PACKAGE_TYPE_NTLM) {
>+      // The principal is the service name, just log.
>+      LOG(("Using SPN of [%s]\n", principal));
>+      return NS_OK;
>+    }

Why can we return NS_OK without setting the output argument |result|?
Don't we need to copy |principal| to |result|?

Another idea is for NTLM to pass "HTTP@host" to the (original) MakeSN
function.  Takehiro told me that IE doesn't specify the port number
for both http and https, to my surprise.

The best solution seems to be for each authentication scheme to construct
the SN using their own rules and then pass the SN to Init(), so that Init()
doesn't need to construct the SN.

>+    // For NTLM, the service policy name can no longer be null. (Bug 487872)

Typo: policy => principal

>     if (mPackage != PACKAGE_TYPE_NTLM)
>       mServiceFlags = serviceFlags;

If mServiceFlags already has the value nsIAuthModule::REQ_DEFAULT here,
it's fine to just delete the if (mPackage != PACKAGE_TYPE_NTLM) test
because we pass serviceFlags=nsIAuthModule::REQ_DEFAULT to this function
for NTLM.  This will eliminate an unnecessary special case for NTLM.

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

>+          if (!isSecure)
>+            port = NS_HTTPS_DEFAULT_PORT;
>+          else
>+            port = NS_HTTP_DEFAULT_PORT;

The test should be if (isSecure), without the '!'.

>+        ToUpperCase(scheme);
>+        serviceName.Append(scheme);

Takehiro told me the <service type> in the SPN is "HTTP" for
both http and https URLs.  So you can get rid of |scheme| and
just say
          serviceName.AppendLiteral("HTTP/");

Comment 32

[Jim Mathies [:jimm]]

(In reply to comment #31)
> (From update of attachment 404316 [details] [diff] [review])
> In extensions/auth/nsAuthSSPI.cpp:
> 
> Why can we return NS_OK without setting the output argument |result|?
> Don't we need to copy |principal| to |result|?

Yes, I just wanted to get the logging call in the same place. This really isn't needed otherwise.

> 
> Another idea is for NTLM to pass "HTTP@host" to the (original) MakeSN
> function.  Takehiro told me that IE doesn't specify the port number
> for both http and https, to my surprise.
> 
> The best solution seems to be for each authentication scheme to construct
> the SN using their own rules and then pass the SN to Init(), so that Init()
> doesn't need to construct the SN.

Ok, will update.

> 
> >+    // For NTLM, the service policy name can no longer be null. (Bug 487872)
> 
> Typo: policy => principal
> 
> >     if (mPackage != PACKAGE_TYPE_NTLM)
> >       mServiceFlags = serviceFlags;
> 
> If mServiceFlags already has the value nsIAuthModule::REQ_DEFAULT here,
> it's fine to just delete the if (mPackage != PACKAGE_TYPE_NTLM) test
> because we pass serviceFlags=nsIAuthModule::REQ_DEFAULT to this function
> for NTLM.  This will eliminate an unnecessary special case for NTLM.

Will do.

> 
> In netwerk/protocol/http/src/nsHttpNTLMAuth.cpp:
> 
> >+          if (!isSecure)
> >+            port = NS_HTTPS_DEFAULT_PORT;
> >+          else
> >+            port = NS_HTTP_DEFAULT_PORT;
> 
> The test should be if (isSecure), without the '!'.
>
> >+        ToUpperCase(scheme);
> >+        serviceName.Append(scheme);
> 
> Takehiro told me the <service type> in the SPN is "HTTP" for
> both http and https URLs.  So you can get rid of |scheme| and
> just say
>           serviceName.AppendLiteral("HTTP/");

Will do!

Comment 33

[Jim Mathies [:jimm]]

Attachment: sn support for trusted hosts v.3

(In reply to comment #31)
> Another idea is for NTLM to pass "HTTP@host" to the (original) MakeSN
> function.  Takehiro told me that IE doesn't specify the port number
> for both http and https, to my surprise.
> 
> The best solution seems to be for each authentication scheme to construct
> the SN using their own rules and then pass the SN to Init(), so that Init()
> doesn't need to construct the SN.

I went with option one since I wanted to keep the changes minimal.

Comment 34

[Jim Mathies [:jimm]]

(In reply to comment #23)
> This simple fix will protect the users who are using the
> "sys-ntlm" module for their proxy servers (the default setting)
> and the servers listed in network.automatic-ntlm-auth.trusted-uris.
> 
> I believe that most users of NTLM have configured their
> network.automatic-ntlm-auth.trusted-uris list, judging from
> the angry user comments in Chromium issue 19 (because
> Chromium doesn't support that yet: http://crbug.com/19).
> 
> 2. Figure out how to use the "sys-ntlm" module for all
> servers, including those not listed in
> network.automatic-ntlm-auth.trusted-uris.  This will cover
> the extranet servers and the users who haven't configured
> their network.automatic-ntlm-auth.trusted-uris list.

Issue two above would involve updating/replacing the use of our ntlm auth module here - 

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNTLMAuthModule.cpp

with the functionality we have in nsAuthSSPI?

Comment 35

[Wan-Teh Chang]

Comment on attachment 404516 [details] [diff] [review]
sn support for trusted hosts v.3

r=wtc.  I verified that the nsAuthSSPI constructor initializes
mServiceFlags to REQ_DEFAULT, so the assignment
    mServiceFlags = serviceFlags;
is a safe no-op for NTLM.

Comment 36

[Wan-Teh Chang]

(In reply to comment #34)
To fix issue two, we should not use nsNTLMAuthModule.cpp (the "ntlm"
module) on Windows under any circumstances.  We should always use
nsAuthSSPI.cpp (the "sys-ntlm" module) on Windows, and we need to
figure out how to pass the user name and password to nsAuthSSPI.cpp
for untrusted URIs.

Comment 37

[Jim Mathies [:jimm]]

(In reply to comment #36)
> (In reply to comment #34)
> To fix issue two, we should not use nsNTLMAuthModule.cpp (the "ntlm"
> module) on Windows under any circumstances.  We should always use
> nsAuthSSPI.cpp (the "sys-ntlm" module) on Windows, and we need to
> figure out how to pass the user name and password to nsAuthSSPI.cpp
> for untrusted URIs.

Ok, cool that's what I was thinking was needed. I'm going to split that off into it's own independent sec bug.

Comment 38

[Daniel Veditz [:dveditz]]

bz: is this one you could super-review?

Comment 39

[Takehiro Takahashi]

Let me know if there's a test build I can run my PoC against.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Probably...

Jim, why is the patch using GetOriginalURI and not GetURI?  Specifically, what behavior do we want in these two cases:

1)  HTTP channel redirected via 3xx response to a different host.  For the
    post-redirect channel, which hostname do we want to be using?
2)  An extension-implemented protocol which opens up an HTTP channel for the
    actual data access.  Do we want to use the http:// URI or the original
    myprotocol: URI?

Comment 41

[Jim Mathies [:jimm]]

(In reply to comment #40)
> Probably...
> 
> Jim, why is the patch using GetOriginalURI and not GetURI?  Specifically, what
> behavior do we want in these two cases:
> 
> 1)  HTTP channel redirected via 3xx response to a different host.  For the
>     post-redirect channel, which hostname do we want to be using?
> 2)  An extension-implemented protocol which opens up an HTTP channel for the
>     actual data access.  Do we want to use the http:// URI or the original
>     myprotocol: URI?

According to msdn, it should identify host running the service we are requesting, and should be unique in cases where a single server is running multiple services. Sounds like (at least in the case of your first example) I want to use GetURI instead - I want the host name that uniquely identifies the proxy or server we are authenticating against.

Comment 42

[Jim Mathies [:jimm]]

(In reply to comment #39)
> Let me know if there's a test build I can run my PoC against.

I'll get some try builds going.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Yeah, you want GetURI, then: that identifies the server we're actually connecting to.  GetOriginalURI is the URI before any redirects or other monkey business.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 404516 [details] [diff] [review]
sn support for trusted hosts v.3

sr=bzbarsky if you use GetURI.

Comment 45

[Jim Mathies [:jimm]]

Attachment: sn support for trusted hosts v.4

try builds should be ready in a few hours.

Comment 46

[Jim Mathies [:jimm]]

Try builds: 
https://build.mozilla.org/tryserver-builds/[e-mail]-try-9e89c0d4e570/


This patch applies to hosts configured for single sign-on through network.automatic-ntlm-auth.trusted-uris. Bug 520607 will address the rest.

Comment 47

[Johnny Stenback (:jst)]

Can this land now, or do we need to land this together with some of the other bugs mentioned in recent comments?

Comment 48

[Jim Mathies [:jimm]]

(In reply to comment #47)
> Can this land now, or do we need to land this together with some of the other
> bugs mentioned in recent comments?

It can land. I was hoping Takehiro Takahashi would run his PoC against it to see if the issue was addressed.

Comment 49

[Takehiro Takahashi]

Will try my PoC this afternoon and let you guys know.

Cheers,
-takehiro

Comment 50

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/a6b2decfc62b

Comment 51

[Jim Mathies [:jimm]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c22a5d2ddefd

Comment 52

[Takehiro Takahashi]

I tested the following binary under XP SP2 (accessed my PoC in the intranet), and it appears to me that the vulnerability is still there.
https://build.mozilla.org/tryserver-builds/[e-mail]-try-9e89c0d4e570/try-9e89c0d4e570-win32.zip

Could it be that I had downloaded a wrong binary?
I don't see secur32.dll being loaded to make the necessary call. 

Cheers,

Comment 53

[Jim Mathies [:jimm]]

(In reply to comment #52)
> I tested the following binary under XP SP2 (accessed my PoC in the intranet),
> and it appears to me that the vulnerability is still there.
> https://build.mozilla.org/tryserver-builds/[e-mail]-try-9e89c0d4e570/try-9e89c0d4e570-win32.zip
> 
> Could it be that I had downloaded a wrong binary?
> I don't see secur32.dll being loaded to make the necessary call. 
> 
> Cheers,

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/

Latest nightly build should do as well. The site you test off of must be an approved host for sys-ntlm for the calls to kick in. I'm working on finishing up prompted auth in bug 520607 today.

Comment 54

[Wan-Teh Chang]

Takehiro: please add the server you're testing against to
the network.automatic-ntlm-auth.trusted-uris preference.

1. Type about:config in the location bar.

2. Click the "I'll be careful, I promise" button in the
warning page, if any.

3. In the "Filter" field, type "ntlm".

4. Add the server's name to network.automatic-ntlm-auth.trusted-uris.

Comment 55

[Takehiro Takahashi]

Jim, Wan-Teh,

That did the trick and I've confirmed that the vulnerability no longer exists in Firefox.
Now, what's the implication of network.automatic-ntlm-auth.trusted-uris?

Comment 56

[Jim Mathies [:jimm]]

(In reply to comment #55)
> Jim, Wan-Teh,
> 
> That did the trick and I've confirmed that the vulnerability no longer exists
> in Firefox.
> Now, what's the implication of network.automatic-ntlm-auth.trusted-uris?

Currently, hosts that are not configured fall back on our internal implementation. Changes in bug 520607 are going to disable that behavior. Users will be required to configure the trusted-uri pref, or optionally turn on prompted sys-ntlm auth.

Comment 57

[Jim Mathies [:jimm]]

(In reply to comment #56)
> (In reply to comment #55)
> > Jim, Wan-Teh,
> > 
> > That did the trick and I've confirmed that the vulnerability no longer exists
> > in Firefox.
> > Now, what's the implication of network.automatic-ntlm-auth.trusted-uris?
> 
> Currently, hosts that are not configured fall back on our internal
> implementation. Changes in bug 520607 are going to disable that behavior. Users
> will be required to configure the trusted-uri pref, or optionally turn on
> prompted sys-ntlm auth.

(on windows, other platforms will still use our internal impl.)

Comment 58

[Takehiro Takahashi]

Thanks.

I'll double check when the next version of Firefox is released, and then we'll move on to the bug disclosure process.

Comment 59

[Jim Mathies [:jimm]]

Attachment: 1.9.1 patch

Comment 60

[Daniel Veditz [:dveditz]]

Comment on attachment 411004 [details] [diff] [review]
1.9.1 patch

Approved for 1.9.1.6, a=dveditz for release-drivers

Does this patch apply to the 1.9.0 branch as well? I can't imagine that this code has changed.

Comment 61

[Jim Mathies [:jimm]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2cfd707c495a

Comment 62

[Jim Mathies [:jimm]]

Attachment: 1.9.0 patch

(In reply to comment #60)
> (From update of attachment 411004 [details] [diff] [review])
> Approved for 1.9.1.6, a=dveditz for release-drivers
> 
> Does this patch apply to the 1.9.0 branch as well? I can't imagine that this
> code has changed.

We did make some ansi-> unicode changes in our calls to the api. Doing a test build to make sure everything works right.

Comment 63

[Jim Mathies [:jimm]]

Comment on attachment 411447 [details] [diff] [review]
1.9.0 patch

.16 or .17, not sure? The patch compiled and tested fine.

Comment 64

[Daniel Veditz [:dveditz]]

On the non-current, stable branches you need approval for every patch, blocking or not. It is confusing to have different rules for different branches, but necessary given the different stages in their life-cycles. Each branch's rules will be at the top of its tinderbox page or linked to from there. But basically: trunk is open, current-branch is less open, and stable branches always require specific approvals.

Comment 65

[Daniel Veditz [:dveditz]]

Comment on attachment 411447 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.16, a=dveditz for release-drivers

Comment 66

[Jim Mathies [:jimm]]

(In reply to comment #64)
> On the non-current, stable branches you need approval for every patch, blocking
> or not. It is confusing to have different rules for different branches, but
> necessary given the different stages in their life-cycles. Each branch's rules
> will be at the top of its tinderbox page or linked to from there. But
> basically: trunk is open, current-branch is less open, and stable branches
> always require specific approvals.

noted!

Comment 67

[Daniel Veditz [:dveditz]]

Checking in netwerk/protocol/http/src/nsHttpNTLMAuth.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpNTLMAuth.cpp,v  <--  nsHttpNTLMAuth.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in extensions/auth/nsAuthSSPI.cpp;
/cvsroot/mozilla/extensions/auth/nsAuthSSPI.cpp,v  <--  nsAuthSSPI.cpp
new revision: 1.14; previous revision: 1.13
done

Comment 68

[Al Billings [:abillings - ex-MoCo]]

Can we get someone with NTLM set up to verify this is fixed with the 1.9.1 and/or 1.9.0 nightlies (ideally both)? I have no way to verify this bug since I don't have access to a network using this.

Comment 69

[Jim Mathies [:jimm]]

(In reply to comment #68)
> Can we get someone with NTLM set up to verify this is fixed with the 1.9.1
> and/or 1.9.0 nightlies (ideally both)? I have no way to verify this bug since I
> don't have access to a network using this.

If you have vista or win7, there are some simple litmus steps posted in bug 520607 to get ntlm working with IIS on the desktop.

Comment 70

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 version

1.9.0 + added #includes at nsHttpNTLMAuth.cpp

Comment 71

[Daniel Veditz [:dveditz]]

bug 535193 is most likely a regression from this fix.

Comment 72

[David :Bienvenu]

fixed for 2.0.24