Bug 487872 (CVE-2009-3983)

Investigate NTLM reflection vulnerability

RESOLVED FIXED in mozilla1.9.2

Status

()

Core
Networking: HTTP
P2
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: bsterne, Assigned: jimm)

Tracking

({fixed1.8.1.24, fixed1.9.0.16})

unspecified
mozilla1.9.2
x86
All
fixed1.8.1.24, fixed1.9.0.16
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
blocking1.9.0.16 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +

Firefox Tracking Flags

(status1.9.2 beta2-fixed, blocking1.9.1 .6+, status1.9.1 .6-fixed)

Details

(Whiteboard: [sg:high])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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.
-----
(Reporter)

Comment 1

8 years ago
Created attachment 372133 [details]
NTLM relection advisory
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

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

Comment 4

8 years ago
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

8 years ago
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.
Assignee: nobody → nobody
Component: Libraries → Networking: HTTP
Product: NSS → Core
QA Contact: libraries → networking.http
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
Flags: wanted1.9.1.x+
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.2?
Whiteboard: [sg:high]

Comment 7

8 years ago
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
Wan-Teh: Are you still investigating this?
status1.9.1: --- → wanted
Flags: wanted1.9.1.x+

Comment 9

8 years ago
FYI, we'll be publishing my paper as a blog post in next few weeks.
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

8 years ago
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
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?
Assignee: nobody → jduell.mcbugs

Comment 13

8 years ago
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

8 years ago
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

==========================================================================
(Assignee)

Comment 15

8 years ago
(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.
(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

8 years ago
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.
(Assignee)

Comment 18

8 years ago
(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!
(Assignee)

Updated

8 years ago
Assignee: jduell.mcbugs → jmathies
(Assignee)

Comment 19

8 years ago
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.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Jim, any updates here yet?
Target Milestone: --- → mozilla1.9.2
(Assignee)

Comment 21

8 years ago
(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.
(Assignee)

Comment 22

8 years ago
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

8 years ago
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.
(Assignee)

Comment 24

8 years ago
(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.
(Assignee)

Comment 25

8 years ago
(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'
(Assignee)

Comment 26

8 years ago
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...
(Assignee)

Comment 27

8 years ago
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. :)
(Assignee)

Comment 28

8 years ago
Created attachment 404305 [details] [diff] [review]
sn support for trusted hosts v.1
Attachment #404305 - Flags: review?(wtc)
(Assignee)

Comment 29

8 years ago
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.
Attachment #404305 - Flags: review?(wtc) → review-
(Assignee)

Comment 30

8 years ago
Created attachment 404316 [details] [diff] [review]
sn support for trusted hosts v.2
Attachment #404305 - Attachment is obsolete: true
Attachment #404316 - Flags: review?(wtc)

Comment 31

8 years ago
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/");
(Assignee)

Comment 32

8 years ago
(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!
(Assignee)

Comment 33

8 years ago
Created attachment 404516 [details] [diff] [review]
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.
Attachment #404316 - Attachment is obsolete: true
Attachment #404316 - Flags: review?(wtc)
(Assignee)

Updated

8 years ago
Attachment #404516 - Flags: review?(wtc)
(Assignee)

Comment 34

8 years ago
(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

8 years ago
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.
Attachment #404516 - Flags: superreview?(cbiesinger)
Attachment #404516 - Flags: review?(wtc)
Attachment #404516 - Flags: review+

Comment 36

8 years ago
(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.
(Assignee)

Comment 37

8 years ago
(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.
(Assignee)

Updated

8 years ago
Blocks: 520607
bz: is this one you could super-review?
Whiteboard: [sg:high] → [needs sr=cbiesinger][sg:high]

Comment 39

8 years ago
Let me know if there's a test build I can run my PoC against.
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?
(Assignee)

Comment 41

8 years ago
(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.
(Assignee)

Updated

8 years ago
Attachment #404516 - Flags: superreview?(cbiesinger) → superreview?(bzbarsky)
(Assignee)

Comment 42

8 years ago
(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.
(Assignee)

Updated

8 years ago
Whiteboard: [needs sr=cbiesinger][sg:high] → [sg:high]
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 on attachment 404516 [details] [diff] [review]
sn support for trusted hosts v.3

sr=bzbarsky if you use GetURI.
Attachment #404516 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 45

8 years ago
Created attachment 409094 [details] [diff] [review]
sn support for trusted hosts v.4

try builds should be ready in a few hours.
Attachment #404516 - Attachment is obsolete: true

Updated

8 years ago
Attachment #409094 - Flags: review+
(Assignee)

Comment 46

8 years ago
Try builds: 
https://build.mozilla.org/tryserver-builds/jmathies@mozilla.com-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.
Can this land now, or do we need to land this together with some of the other bugs mentioned in recent comments?
(Assignee)

Comment 48

8 years ago
(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

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

Cheers,
-takehiro
(Assignee)

Comment 50

8 years ago
http://hg.mozilla.org/mozilla-central/rev/a6b2decfc62b
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 51

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c22a5d2ddefd
status1.9.2: --- → final-fixed

Comment 52

8 years ago
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/jmathies@mozilla.com-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,
(Assignee)

Comment 53

8 years ago
(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/jmathies@mozilla.com-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

8 years ago
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

8 years ago
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?
(Assignee)

Comment 56

8 years ago
(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.
(Assignee)

Comment 57

8 years ago
(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

8 years ago
Thanks.

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

Comment 59

8 years ago
Created attachment 411004 [details] [diff] [review]
1.9.1 patch
Attachment #411004 - Flags: approval1.9.1.6?
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.
Attachment #411004 - Flags: approval1.9.1.6? → approval1.9.1.6+
blocking1.9.1: --- → ?
Flags: blocking1.9.0.16?
(Assignee)

Comment 61

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2cfd707c495a
(Assignee)

Comment 62

8 years ago
Created attachment 411447 [details] [diff] [review]
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.
(Assignee)

Updated

8 years ago
status1.9.1: wanted → .6-fixed
blocking1.9.1: ? → .6+
(Assignee)

Comment 63

8 years ago
Comment on attachment 411447 [details] [diff] [review]
1.9.0 patch

.16 or .17, not sure? The patch compiled and tested fine.
Attachment #411447 - Flags: approval1.9.0.16?
Flags: blocking1.9.0.16? → blocking1.9.0.16+
(Assignee)

Updated

8 years ago
Attachment #411447 - Flags: approval1.9.0.16?
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
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 on attachment 411447 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.16, a=dveditz for release-drivers
Attachment #411447 - Flags: approval1.9.0.16+
(Assignee)

Comment 66

8 years ago
(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!
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
Flags: blocking1.8.1.next?
Keywords: checkin-needed → fixed1.9.0.16
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.
(Assignee)

Comment 69

8 years ago
(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

8 years ago
Created attachment 415092 [details] [diff] [review]
1.8.0 version

1.9.0 + added #includes at nsHttpNTLMAuth.cpp
Alias: CVE-2009-3983
(Assignee)

Updated

8 years ago
Depends on: 535193
bug 535193 is most likely a regression from this fix.
Flags: blocking1.8.1.next? → blocking1.8.1.next+

Updated

8 years ago
Attachment #415092 - Flags: approval1.8.1.next+

Comment 72

8 years ago
fixed for 2.0.24
Keywords: fixed1.8.1.24
Group: core-security
You need to log in before you can comment on or make changes to this bug.