423758 - Firefox can't authenticate to IIS when minimum NTLM level set to NTLMv2

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Christopher Nebergall]

If NTLM Compatiblity Level is set to level 5 on the box hosting IIS 6.0, and windows integrated authentication is enabled in IIS, the user can't get past the NTLM prompt unless they click cancel on the NTLM prompt and wait for the basic prompt.  Otherwise they keep getting additional NTLM prompts which all fail. 

NTLM Compatibility Levels
http://msdn2.microsoft.com/en-us/library/cc207934.aspx
http://davenport.sourceforge.net/ntlm.html#ntlmVersion2

5:DCs refuse LM and NTLM responses, and accept only NTLM v2. Clients use NTLM v2 authentication and use NTLM v2 session security if the server supports it. DCs refuse NTLM and LM authentication, and accept only NTLM v2 authentication.

I don't know the max level of NTLM for firefox 2.0 so I'm unsure if it is a bug in the NTLM implementation or not.

I'm working with a company that this breaks their access to all of their IIS boxes from firefox users. Since SSL is in place I'm ok with with an option in about:config in firefox to disable NTLM support all together and have it automatically fail down to basic auth.

The automatic send NTLM features in firefox for windows are not enabled, so I'm only talking about firefox's NTLM implementation, not the windows integrated version.

Comment 1

[Jim Mathies [:jimm]]

This should be addressed in Fx 3.6.

Comment 2

[Christopher Nebergall]

This appears to have been fixed for Windows Firefox, but for Linux builds.  Modern released linux firefox builds still has the same issue.   It was marked as depending on bug which is a security bug I don't have access to 520607.  I'm attempting to set the bug "RESOLVED: INCOMPLETE" because I don't have rights to re-open the bug.

Comment 3

[Christopher Nebergall]

Sorry typo

This appears to have been fixed for Windows Firefox, but NOT for Linux builds.

Comment 4

[Christopher Nebergall]

This is still broken on non-windows builds. Marking re-opened if someone wants me to open a new bug instead of reopening this one let me know.

Comment 5

[Harry]

I have a lot of Mac users in my organization who prefer Firefox over Safari; however, NTLM V1 protocol has been disabled on our domain controllers, so it is no longer possible for Mac users to login to Sharepoint (and some other Intranet sites) using Firefox. They can still use Safari to get in, but they find it annoying to switch back and forth between the two browsers. When will the Mac version of Firefox work with the latest NTLM protocol?

Comment 6

[Tony George]

I also have a lot of Mac users in my organization who prefer Firefox over Safari. At the University I work for we have just disabled the NTLM V1 protocol. My Mac users can nolonger access our SharePoint environment with Firefox. They can access just fine with Safari or with the PC version of Firefox. When will the Mac version of Firefox be fixed?

Comment 7

[Jason Duell]

If I read bug 520607 comment 12 correctly, the issue is that we rely on a windows system library for more recent NTLM versions, and we didn't have that available on non-windows platforms.  jmathies: do you know if something has changed that would let us use newer NTLM on OSX, or is Safari using something internal that we can't access?

Comment 8

[Joe Gasper]

I would be happy with just a way to tell Firefox to use offer basic auth to specific URIs.  A setting like network.auth.force-basic; type string. Many EDUs are moving forward with InCommon Silver certification that will require their MS Active Directory environments to allow only NTLMv2.

Comment 9

[Martin B. Smith]

+1 for allowing users to disable Firefox's NTLM support; as Joe said, this breaks for places requiring NTLMv2 only, making Firefox unusable on a site that accepts basic auth or NTLMv2 only. As folks focus on better security in Active Directory, this will show up as a problem more and more for Linux Firefox users.

Comment 10

[Nick Lowe]

For info: We have just implemented the same change to our Windows domain due to the recent attention surrounding the inherent cryptographic weakness of NTLMv1. We now find ourselves, sadly, having to advise Firefox users on platforms other than Windows that they have to, for the moment, switch Web browsers due to the issue in the browser that prevents NTLMv2 from working.

Comment 11

[Nick Lowe]

Any update on this? Please could this get some attention? It is causing significant pain in our environment.

There is an open source implementation of NTLMv2 at the link below. Is there any chance this, or a suitable alternative, could be integrated in Firefox?

http://david.woodhou.se/ntlm_auth_v2.c

It is the impact of this for Mac OS X users that is the main pain for us not the few Linux users we have who tend to be more tech savvy.

Thanks,

Nick

Comment 12

[gianfrus]

The few Linux users could "tend to be more tech savvy" but on company sites that have switched to NTLMv2 **only** authentication (very sad choice, because the benefits from SSO nowadays are, in fact, very little taking into account the security and capability of applications on storing passwords) is a real pain. And make it difficult to spread Linux in environments that would derive much benefit from it.

Please, take greater account of this bug (as, now, NTLMv2 implementation on Linux are present and well functioning).

Thanks,

Gianluca

Comment 13

[Andrew Bartlett]

Just to let folks know that I'm working on this, and expect to have some patches for this available soon.

Comment 14

[Marcus Moeller]

Great, because some of our systems are going to support NTLMv2 only, soon.

Comment 15

[Andrew Bartlett]

Attachment: patch to remove LM auth code (NTLM is always available)

This (and the real patch to follow, actually doing NTLMv2) is work funded by Mozilla that I've been doing to resolve this issue. 

The patches certainly still need to be further cleaned up, but if you pass in an UPPER case username, they work.

Comment 16

[Andrew Bartlett]

Attachment: 0002-initial-NTLMv2-work.patch

This patch implements NTLMv2 when the username is supplied in UPPER case.  

No validation of the server target info is done (Samba doesn't either...), and the LMv2 part isn't tested yet. 

Tested against Samba's git master 'ntlm_auth' and apache with the mod_auth_ntlm_winbind module.

Comment 17

[Magnus Melin [:mkmelin]]

Don't forget setting the review flag for your patches ([e-mail] could be a good choice)
https://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree#Getting_the_patch_reviewed

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 8451438 [details] [diff] [review]
patch to remove LM auth code (NTLM is always available)

I'll take a look!  Adding f? on me (feel free to it with your WIP patches yourself).

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8451438 [details] [diff] [review]
patch to remove LM auth code (NTLM is always available)

Review of attachment 8451438 [details] [diff] [review]:
-----------------------------------------------------------------

Few nits:

- please create you patches with 8 line context
- avoid trailing white spaces at the end of your added/modified lines

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ -1365,5 @@
>                                          getter_Copies(result));
>    }
>  
> -  bool sendLM = Preferences::GetBool("network.ntlm.send-lm-response",
> -                                     SEND_LM_DEFAULT);

OK, let's remove this preference also from /modules/libpref/src/init/all.js

note that by default this false, so this patch doesn't have a practical meaning.  it this a necessary prerequisite?  or just a cleanup of code we will never use again?  (both are good arguments for this patch).

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8451439 [details] [diff] [review]
0002-initial-NTLMv2-work.patch

Review of attachment 8451439 [details] [diff] [review]:
-----------------------------------------------------------------

This is only a preliminary feedback, I'll walk though the algorithm itself later, feel free to update the patch at any time.

According uppercase for unicode strings, I'll try to ask around some folks and point them here.

::: security/manager/ssl/src/nsNTLMAuthModule.cpp
@@ +446,2 @@
>    uint32_t    targetLen;     // target length in bytes
> +  const uint8_t *targetInfo;        // target AV pairs

what's AV?  please try not to use abbrevs or explain them

@@ +503,4 @@
>    // read challenge
>    memcpy(msg->challenge, cursor, sizeof(msg->challenge));
>    cursor += sizeof(msg->challenge);
> +  

nit: try to avoid white space only changes.

@@ +530,5 @@
> +      msg->targetInfoLen = targetInfoLen;
> +      msg->targetInfo = ((const uint8_t *) inBuf) + offset;
> +    }
> +    else
> +    {

nit: (despite I don't like it) we prefer 

} else { 

(on a single line)

@@ +559,4 @@
>      return rv;
>  
>    bool unicode = (msg.flags & NTLM_NegotiateUnicode);
> +  bool ntlmv2 = (sNTLMv1Enabled == false);

sNTLMv1Enabled is no longer in the code

@@ +686,5 @@
> +		     (uint8_t *)userUpperPtr, userUpperLen,
> +		     (uint8_t *)domainUpperPtr, domainUpperLen,
> +		     NULL, 0,
> +		     ntlmv2Hash, NTLMv2_HASH_LEN);
> +    if (rv != NS_OK) {

if (NS_FAILED(rv))

@@ -710,5 @@
>  
> -  if (!sNTLMv1Enabled) {
> -    // Unconditionally disallow usage of the generic module.
> -    return NS_ERROR_NOT_AVAILABLE;
> -  }

I think this has already change.  It's good to update your mozilla-central reporsitory every few days (maybe in your case only once a week).

@@ +1045,5 @@
> +  if (!ctxt)
> +  {
> +    NS_ERROR("no context");
> +    goto done;
> +  }

nit: preferred style is:

if () {
}

@@ +1074,5 @@
> +			    const uint8_t *input2, uint32_t input2Len, 
> +			    const uint8_t *input3, uint32_t input3Len, 
> +			    uint8_t *result, uint32_t resultLen)
> +{
> +  CK_MECHANISM_TYPE cipherMech = CKM_MD5_HMAC;

pass directly?

Comment 21

[Andrew Bartlett]

(In reply to Honza Bambas (:mayhemer) from comment #19)

> @@ -1365,5 @@
> >                                          getter_Copies(result));
> >    }
> >  
> > -  bool sendLM = Preferences::GetBool("network.ntlm.send-lm-response",
> > -                                     SEND_LM_DEFAULT);
> 
> OK, let's remove this preference also from /modules/libpref/src/init/all.js

Sure.
 
> note that by default this false, so this patch doesn't have a practical
> meaning.  it this a necessary prerequisite?  or just a cleanup of code we
> will never use again?  (both are good arguments for this patch).

Just cleanup of code that should not ever be needed, and to avoid perverse preference combinations of enabling LM and NTLMv2 at the same time, for example. 

Andrew Bartlett

Comment 22

[Honza Bambas (:mayhemer)]

I've made following testing to confirm the patch works:

* I'm locally running a domain "test.local" on IIS 7.5 (win7), HTTPS access setup too
* It's setup to accept "back connections": HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Lsa\MSV1_0\BackConnectionHostNames = "test.local"
* By default accept-NTLMv2-only settings are engaged: HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Lsa\LMCompatibilityLevel = 5
* there is a /ntlm2/ page setup for authentication only with Windows Authentication, provider NTLM, no Extended Protection enforcement, accessible only to a testing user

W/ your patch, network.auth.force-generic-ntlm = true, otherwise default, going to http://test.local/ntlm2/ - using manually upper-cased username, I can connect!

W/O your patch, otherwise same, no luck.

Note that changing the provider in IIS to only Negotiate, I'm not even getting auth prompt in Firefox.  That is however a different problem (not in scope of this bug).

Comment 23

[Honza Bambas (:mayhemer)]

To let you know, we currently disable NTLMv1 when connecting a proxy or an end server we are not connecting via SSL (https).  We only enable it when connecting an end server through SSL.  There are two preferences, see the first two prefs referenced here:

http://www.janbambas.cz/ntlm-v1-and-firefox/

When for a scenario ntlmv1 is enabled, what do we send to the server?  Does the server expresses in some flag it talks ntlmv2?  In that case I would send out ntlmv2 response only, regardless of preferences.  ntlmv1 should only be a fallback (when available according target we connect to and the preferences.)

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 8451439 [details] [diff] [review]
0002-initial-NTLMv2-work.patch

So far f+, mainly based on positive testing.

Comment 25

[Andrew Bartlett]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> Comment on attachment 8451439 [details] [diff] [review]
> 0002-initial-NTLMv2-work.patch
> 
> Review of attachment 8451439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is only a preliminary feedback, I'll walk though the algorithm itself
> later, feel free to update the patch at any time.

Thanks,

> According uppercase for unicode strings, I'll try to ask around some folks
> and point them here.
> 
> ::: security/manager/ssl/src/nsNTLMAuthModule.cpp
> @@ +446,2 @@
> >    uint32_t    targetLen;     // target length in bytes
> > +  const uint8_t *targetInfo;        // target AV pairs
> 
> what's AV?  please try not to use abbrevs or explain them
> 
> @@ +503,4 @@
> >    // read challenge
> >    memcpy(msg->challenge, cursor, sizeof(msg->challenge));
> >    cursor += sizeof(msg->challenge);
> > +  
> 
> nit: try to avoid white space only changes.

What is the preferred way to improve code clarity?  Must I do it in a distinct patch?

> @@ +530,5 @@
> > +      msg->targetInfoLen = targetInfoLen;
> > +      msg->targetInfo = ((const uint8_t *) inBuf) + offset;
> > +    }
> > +    else
> > +    {
> 
> nit: (despite I don't like it) we prefer 
> 
> } else { 
> 
> (on a single line)
> 
> @@ +559,4 @@
> >      return rv;
> >  
> >    bool unicode = (msg.flags & NTLM_NegotiateUnicode);
> > +  bool ntlmv2 = (sNTLMv1Enabled == false);
> 
> sNTLMv1Enabled is no longer in the code
> 
> @@ +686,5 @@
> > +		     (uint8_t *)userUpperPtr, userUpperLen,
> > +		     (uint8_t *)domainUpperPtr, domainUpperLen,
> > +		     NULL, 0,
> > +		     ntlmv2Hash, NTLMv2_HASH_LEN);
> > +    if (rv != NS_OK) {
> 
> if (NS_FAILED(rv))
> 
> @@ -710,5 @@
> >  
> > -  if (!sNTLMv1Enabled) {
> > -    // Unconditionally disallow usage of the generic module.
> > -    return NS_ERROR_NOT_AVAILABLE;
> > -  }
> 
> I think this has already change.  It's good to update your mozilla-central
> reporsitory every few days (maybe in your case only once a week).
> 
> @@ +1045,5 @@
> > +  if (!ctxt)
> > +  {
> > +    NS_ERROR("no context");
> > +    goto done;
> > +  }
> 
> nit: preferred style is:
> 
> if () {
> }
> 
> @@ +1074,5 @@
> > +			    const uint8_t *input2, uint32_t input2Len, 
> > +			    const uint8_t *input3, uint32_t input3Len, 
> > +			    uint8_t *result, uint32_t resultLen)
> > +{
> > +  CK_MECHANISM_TYPE cipherMech = CKM_MD5_HMAC;
> 
> pass directly?

This is used in 3 following calls, and and this matches the des code above.

I'll post the updated patch shortly.

Andrew Bartlett

Comment 26

[Andrew Bartlett]

(In reply to Honza Bambas (:mayhemer) from comment #23)
> To let you know, we currently disable NTLMv1 when connecting a proxy or an
> end server we are not connecting via SSL (https).  We only enable it when
> connecting an end server through SSL.  There are two preferences, see the
> first two prefs referenced here:

My patches (to be posted shortly) will first revert the situation back to this module being available by default, for all connection types, and then add NTLMv2 (on by default).  Servers never indicate that they support NTLMv2, there is no negotiation here.  All modern and not-so-modern servers support NTLMv2. 

Andrew Bartlett

Comment 27

[Nick Lowe]

Once NTLMv2 support is available, NTLMv1 support should be disabled by default for both HTTP and HTTPS.

Arguably NTLMv1 should, at that point, be removed completely from the browser.

Comment 28

[Andrew Bartlett]

Attachment: mozilla-ntlmv2.patch

Attached is my patch following the feedback so far.  Hopefully I've addressed everything except the need for a UTF16 string upper case routine.

Comment 29

[Honza Bambas (:mayhemer)]

(In reply to Andrew Bartlett from comment #25)
> > nit: try to avoid white space only changes.
> 
> What is the preferred way to improve code clarity?  Must I do it in a
> distinct patch?

If it's for code clarity, please leave it in this patch!  That's good and I appreciate it :)  Sorry, missed that the first time (7 lines context!)

> > > +  CK_MECHANISM_TYPE cipherMech = CKM_MD5_HMAC;
> > 
> > pass directly?
> 
> This is used in 3 following calls, and and this matches the des code above.

Missed that, OK.

Comment 30

[Honza Bambas (:mayhemer)]

(In reply to Nick Lowe from comment #27)
> Once NTLMv2 support is available, NTLMv1 support should be disabled by
> default for both HTTP and HTTPS.
> 
> Arguably NTLMv1 should, at that point, be removed completely from the
> browser.

I wouldn't rush that much.  I'd rather for some time leave it off by default and adjustable via a pref.  This is the problem with NTLM - we know we screw up something only up in the release channel.  Any changes then have to wait at least a whole release (cannot be quick fixed on the release channel) or even longer.  So, I'd rather be careful and check first how many people may live w/o NTLMv1.  If it's low+they fix their servers/proxies or even zero, then we can remove the code completely.  Note that we were still keeping ssl 2.0 etc a relatively long time around until it has been decided to remove it completely.

Comment 31

[Honza Bambas (:mayhemer)]

Andrew, please update you mozilla-central repository and then merge your patch(es) to that latest source version.  Then resubmit.  Thanks.

Comment 32

[Honza Bambas (:mayhemer)]

Andrew, this is what you need:
http://mxr.mozilla.org/mozilla-central/source/intl/unicharutil/util/nsUnicharUtils.h#19

Comment 33

[Honza Bambas (:mayhemer)]

Comment on attachment 8454237 [details] [diff] [review]
mozilla-ntlmv2.patch

Andrew, this patch contains some same files twice or ever 3 times.  How did you generate the patch?  It's pretty hard and confusing to take a look at this.

Comment 34

[Andrew Bartlett]

It is a patch series of the 4 independent patches involved, two reverting the recent preference changes, and one removing LM and one implementing NTLMv2.  

I take it you would prefer these as 4 separate attachments?

I used:
git format-patch -U8 -4 --stdout > mozilla-ntlmv2.patch

Thanks,

Comment 35

[Andrew Bartlett]

Attachment: 0001-Revert-Bug-1030426-network.negotiate-auth.allow-inse.patch

Comment 36

[Andrew Bartlett]

Attachment: 0002-Revert-Bug-1023748-Allow-NTLMv1-over-SSL-TLS-by-defa.patch

Comment 37

[Andrew Bartlett]

Attachment: 0003-Remove-LM-code-from-internal-NTLM-handler.patch

Comment 38

[Andrew Bartlett]

Attachment: 0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Attached as distinct patches are the 4 patches in my patch series.  They revert the recent preferences changes, remove LM support and add NTLMv2 support with a new preference network.negotiate-auth.force-less-secure-ntlm-v1 to allow NTLMv1 if required.

This is now tested with a lowercase (ascii only at this point, but the code is meant to do unicode) username.  Understanding of memory allocation patterns/requirements around this would be very helpful. 

This should resolve this for now, but the next step would be to support TLS channel bindings.  I also need guidance on how to add an automated test for this, given it requires an external apache process to really test. 

Andrew Bartlett

Comment 39

[Honza Bambas (:mayhemer)]

(In reply to Andrew Bartlett from comment #34)
> It is a patch series of the 4 independent patches involved, two reverting
> the recent preference changes, and one removing LM and one implementing
> NTLMv2.  
> 
> I take it you would prefer these as 4 separate attachments?
> 
> I used:
> git format-patch -U8 -4 --stdout > mozilla-ntlmv2.patch
> 
> Thanks,

For bugzilla (and splinter) as well as for the final commit it's better to have separate patches (if you already have it so in your patch queue and it makes sense to split - what here does), best also marked as "part 1 of N", so others know easily in what order the patches are about to be applied and are all present in bugzilla.  

Thanks.

Comment 40

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #39)
> best also marked as "part 1 of N",

The current naming of the patches is OK :)  I'll look at them hopefully today.  Thanks!

Comment 41

[Honza Bambas (:mayhemer)]

Andrew, I think I follow what you wanted to submit.  If you want a complete patch (I'm not against, it's usually simpler to review and this is not that large) you have to submit a 'folded' single patch.

If I want to keep the local queue then (using hg) I run the following command to produce a folded patch:

$ hg diff -r first-patch-parent-rev:last-patch-rev > my-complete.patch

Please, add 8 line context and resubmit.  It's very hard to review with only 3 lines of context.

Thanks.

Comment 42

[Andrew Bartlett]

Attachment: 0001-Revert-Bug-1030426-network.negotiate-auth.allow-inse.patch

Comment 43

[Andrew Bartlett]

Attachment: 0002-Revert-Bug-1023748-Allow-NTLMv1-over-SSL-TLS-by-defa.patch

Comment 44

[Andrew Bartlett]

Attachment: 0003-Remove-LM-code-from-internal-NTLM-handler.patch

Comment 45

[Andrew Bartlett]

Attachment: 0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Comment 46

[Andrew Bartlett]

Sorry for the lack of context previously, this series should have the required context.

I think they belong as distinct patches, because two are reverts and the LM removal is a logically distinct change, but naturally I'll defer to Mozilla's style and preference.

(Samba has taken the 'small tiny one-step-at-a-time' git/Linux kernel style, so that is what I'm used to).

Comment 47

[Honza Bambas (:mayhemer)]

(In reply to Andrew Bartlett from comment #46)
> Sorry for the lack of context previously, this series should have the
> required context.
> 
> I think they belong as distinct patches, because two are reverts and the LM
> removal is a logically distinct change, but naturally I'll defer to
> Mozilla's style and preference.
> 
> (Samba has taken the 'small tiny one-step-at-a-time' git/Linux kernel style,
> so that is what I'm used to).

Thanks!  It makes review/feedback much simpler.  There is not a specific mozilla style, it's up to how patch author and reviewer agree on.  I personally prefer both - partial patches and also the whole 'folded' one.  But here split patches are sufficient.

Going to take a look yet today.

Comment 48

[Honza Bambas (:mayhemer)]

Comment on attachment 8456737 [details] [diff] [review]
0003-Remove-LM-code-from-internal-NTLM-handler.patch

Review of attachment 8456737 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNTLMAuthModule.cpp
@@ +93,5 @@
>  #define NTLM_TYPE2_HEADER_LEN 32
>  #define NTLM_TYPE3_HEADER_LEN 64
>  
> +/**
> + * We don't actually send a LM response, but we still have to send something in this spot 

avoid trailing whitespaces in lines you add (modify)

Comment 49

[Honza Bambas (:mayhemer)]

the large patch review in progress, will finish tomorrow (Thursday)

Comment 50

[Honza Bambas (:mayhemer)]

Comment on attachment 8456739 [details] [diff] [review]
0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Review of attachment 8456739 [details] [diff] [review]:
-----------------------------------------------------------------

Retested, works!

::: modules/libpref/src/init/all.js
@@ +1419,5 @@
>  //   [scheme "://"] [host [":" port]]
>  // For example, "foo.com" would match "http://www.foo.com/bar", etc.
>  
> +// Force less-secure NTLMv1 when needed (NTLMv2 is the default).
> +pref("network.negotiate-auth.force-less-secure-ntlm-v1", false);

since I don't like the "negotiate-auth" prefix part, please rename this pref to "network.auth.force-generic-ntlm-v1" or, if you prefer (I don't care) "network.auth.force-generic-less-secure-ntlm-v1" or alike.

::: security/manager/ssl/src/nsNTLMAuthModule.cpp
@@ +41,5 @@
> +static nsresult md5sum(const uint8_t *input, uint32_t inputLen, uint8_t *result);
> +static nsresult hmac_md5sum(const uint8_t *key, uint32_t keyLen, 
> +			    const uint8_t *input, uint32_t inputLen, 
> +			    const uint8_t *input2, uint32_t input2Len, 
> +			    const uint8_t *input3, uint32_t input3Len, 

again, please avoid trailing spaces.  there are more in the file, there are visible as red/pink bars in splinter (the 'review' link by the attachment)

I can also provide a simple script to run the patch through.

@@ +447,3 @@
>    uint32_t    targetLen;     // target length in bytes
> +  const uint8_t *targetInfo;        // target Attribute-Value pairs (DNS domain, et al)
> +  uint32_t    targetInfoLen;     // target AV pairs length in bytes

please arrange the comments

@@ +510,5 @@
>    LogBuf("flags", (const uint8_t *) &msg->flags, 4);
>    LogFlags(msg->flags);
>    LogBuf("challenge", msg->challenge, sizeof(msg->challenge));
>  
> +  if (inLen < NTLM_TYPE2_HEADER_LEN + 16) {

what is the 16?  should also be #defined

@@ +526,5 @@
> +    // ... read offset from inBuf.
> +    uint32_t offset = ReadUint32(cursor);
> +    // Check the offset / length combo is in range of the input buffer, including
> +    // integer overflow checking.
> +    if (MOZ_LIKELY(offset < offset + targetInfoLen && offset + targetInfoLen <= inLen)) {

we have CheckedInt class for this:

http://mxr.mozilla.org/mozilla-central/source/mfbt/CheckedInt.h#499

it has all += etc operators overloaded and there is isValid() method telling you the overflow status.

The usage should be:


CheckedInt<uint32_t> targetInfoEnd = offset;
targetInfoEnd += targetInfoLen;

if (!targetInfoEnd.isValid() || targetInfoEnd.value() > inLen) {
  ...error...
  return ...;
}

msg->target...
...

@@ +557,5 @@
>    if (NS_FAILED(rv))
>      return rv;
>  
>    bool unicode = (msg.flags & NTLM_NegotiateUnicode);
> +  bool ntlmv2 = (sNTLMv1Forced == false);

I don't know NTLM to that detail, but I would expect something more like:

sNTLMv1Forced == false:

send NTLMv2 response only


sNTLMv1Forced == true:

send NTLMv1 response
send NTLMv2 response


But from how I read the code it would be

sNTLMv1Forced == true:

send NTLMv1 response only

Do we really want this?

@@ +656,5 @@
> +    nsAutoString ucsDomainUpperBuf, ucsUserUpperBuf;
> +    const void *domainUpperPtr, *userUpperPtr;
> +    uint32_t domainUpperLen, userUpperLen;
> +    uint8_t client_random[8];
> +    time_t unix_time;

Please define local vars close to first use.

@@ +657,5 @@
> +    const void *domainUpperPtr, *userUpperPtr;
> +    uint32_t domainUpperLen, userUpperLen;
> +    uint8_t client_random[8];
> +    time_t unix_time;
> +    uint64_t nt_time = time(&unix_time);

needs #include <time.h> for me.

@@ +661,5 @@
> +    uint64_t nt_time = time(&unix_time);
> +    nt_time += 11644473600LL;
> +    nt_time *= 1000*1000*10;
> +
> +    PK11_GenerateRandom(client_random, 8);

the 8 should be #defined

@@ +664,5 @@
> +
> +    PK11_GenerateRandom(client_random, 8);
> +
> +    ucsUserUpperBuf = username;
> +    ToUpperCase(ucsUserUpperBuf);

Please use the ToUpperCase(const nsAString& aSource, nsAString& aDest); version of the function so that it's clear you created a copy of the string.  The reason is you on big-endian platforms manipulate the buffer directly.

@@ +710,5 @@
> +    PK11_GenerateRandom(&ntlmv2_blob1[16], 8);
> +    
> +    if (msg.targetInfoLen == 0) {
> +      NS_ERROR("failed to get NTLMv2 target info, can not do NTLMv2");
> +      return NS_ERROR_UNEXPECTED;

can this check be made sooner?  seems like you are not touching msg.targetInfoLen in this block prior this line at all.

@@ +724,5 @@
> +      return rv;
> +    }
> +
> +    ntlmRespLen = NTLMv2_RESP_LEN + NTLMv2_BLOB1_LEN + msg.targetInfoLen;
> +    if (ntlmRespLen < NTLMv2_RESP_LEN || ntlmRespLen < NTLMv2_BLOB1_LEN || ntlmRespLen < msg.targetInfoLen) {

CheckedInt is your friend here again.

@@ +764,5 @@
> +      || *outLen < hostLen
> +      || *outLen < domainLen
> +      || *outLen < userLen
> +      || *outLen < LM_RESP_LEN
> +      || *outLen < ntlmRespLen) {

CheckedInt

@@ +794,4 @@
>    offset += LM_RESP_LEN;
> +  cursor = WriteSecBuf(cursor, ntlmRespLen, offset);
> +  if (ntlmv2) {
> +    if (ntlmRespLen - NTLMv2_RESP_LEN - NTLMv2_BLOB1_LEN != msg.targetInfoLen) {

if ((ntlmRespLen - NTLMv2_RESP_LEN - NTLMv2_BLOB1_LEN) != msg.targetInfoLen) {

@@ +1039,3 @@
>  {
> +  uint32_t resultLen = 16;
> +  nsresult ret = NS_ERROR_UNEXPECTED;

nsresult rv (this is the norm).

@@ +1042,3 @@
>    PK11Context *ctxt = PK11_CreateDigestContext(SEC_OID_MD5);
> +  if (!ctxt)
> +  {

if () {
}

if () {
}

@@ +1042,5 @@
>    PK11Context *ctxt = PK11_CreateDigestContext(SEC_OID_MD5);
> +  if (!ctxt)
> +  {
> +    NS_ERROR("no context");
> +    goto done;

return rv;

I think PK11_DestroyContext crashes when context is null

@@ +1054,5 @@
> +  {
> +    NS_ERROR("failed to do digest");
> +    goto done;
> +  }
> +  PK11_DigestFinal(ctxt, result, &resultLen, resultLen);

returns SECStatus as well, check the result.

@@ +1072,5 @@
> +			    const uint8_t *input2, uint32_t input2Len, 
> +			    const uint8_t *input3, uint32_t input3Len, 
> +			    uint8_t *result, uint32_t resultLen)
> +{
> +  CK_MECHANISM_TYPE cipherMech = CKM_MD5_HMAC;

really, rather pass directly to all function calls, this evokes sense like it should change (since it's not const).

@@ +1077,5 @@
> +  SECItem keyItem, *param = nullptr;
> +  PK11SymKey *symkey = nullptr;
> +  PK11SlotInfo *slot = nullptr;
> +  PK11Context *ctxt = nullptr;
> +  nsresult ret = NS_ERROR_UNEXPECTED;

again, except nsresult rv (which is used all around the block) please define other local vars close to the first use

@@ +1080,5 @@
> +  PK11Context *ctxt = nullptr;
> +  nsresult ret = NS_ERROR_UNEXPECTED;
> +  slot = PK11_GetBestSlot(cipherMech, nullptr);
> +  if (!slot)
> +  {

if () {
}

@@ +1125,1 @@
>      {

maybe join to a single && condition?

@@ +1147,3 @@
>    }
> +
> +  PK11_DigestFinal(ctxt, result, &resultLen, resultLen);

check result

Comment 51

[Nick Lowe]

NTLMv1 fallback support should not be enabled by default.

See the Microsoft's documentation on the LmCompatibilityLevel registry setting for their control over this in Windows:

http://technet.microsoft.com/en-us/library/cc960646.aspx

Windows Vista and later ships with this value set to 3 by default.
There, “Clients use only NTLMv2 authentication".

Firefox, as the client, should therefore have no cause to be able to use NTLMv1 in its default configuration as this has shipped in Windows since 2006.

As I stated before, arguably, such support should be removed completely as it serves no legitimate purpose.

Any change that allows NTLMv1 to be used in the default configuration should be rejected.

Comment 52

[Andrew Bartlett]

(In reply to Nick Lowe from comment #51)
> NTLMv1 fallback support should not be enabled by default.

It isn't.

Andrew Bartlett

Comment 53

[Andrew Bartlett]

Attachment: 0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

This updated patch hopefully addresses the whitespace (with git rebase --whitespace=fix), and everything except the integer overflow stuff.

Thank you for your patience and review of this work.  

I'll try to understand the overflow stuff soon and re-submit.

Comment 54

[Andrew Bartlett]

Attachment: 0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

This fixed version uses the alternate ToUpperCase invocation as requested.  Integer overflow work is still pending.

Comment 55

[Andrew Bartlett]

Attachment: 0001-Use-mozilla-CheckedInt-in-NTLMv2-handler.patch

Comment 56

[Andrew Bartlett]

Attached is the extra patch to use CheckedInt.  

Now I need some advice on how to do automated testing of this.

Comment 57

[Honza Bambas (:mayhemer)]

Comment on attachment 8459922 [details] [diff] [review]
0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Review of attachment 8459922 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  Looks great.  Sorry for the delay, I was off for a serious and sudden family business.

::: modules/libpref/src/init/all.js
@@ +1434,5 @@
>  //   [scheme "://"] [host [":" port]]
>  // For example, "foo.com" would match "http://www.foo.com/bar", etc.
>  
> +// Force less-secure NTLMv1 when needed (NTLMv2 is the default).
> +pref("network.ntlm.force-generic-ntlm-v1", false);

"network.auth..." please.

::: security/manager/ssl/src/nsNTLMAuthModule.cpp
@@ +706,5 @@
> +
> +    time_t unix_time;
> +    uint64_t nt_time = time(&unix_time);
> +    nt_time += 11644473600LL;
> +    nt_time *= 1000*1000*10;

spaces around asterisks

Comment 58

[Honza Bambas (:mayhemer)]

Comment on attachment 8465208 [details] [diff] [review]
0001-Use-mozilla-CheckedInt-in-NTLMv2-handler.patch

Review of attachment 8465208 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNTLMAuthModule.cpp
@@ +271,5 @@
>  
>  static void *
> +WriteSecBuf(void *buf,
> +	    mozilla::CheckedInt<uint16_t> checked_length,
> +	    mozilla::CheckedInt<uint32_t> checked_offset)

I'm not sure why you are passing (and by a copy) checkedint here.  I think you are OK with passing in uint16_t and use .value() for calls when needed.

@@ +580,5 @@
>    // pointers and lengths for the string buffers; encoding is unicode if
>    // the "negotiate unicode" flag was set in the Type-2 message.
>    const void *domainPtr, *userPtr, *hostPtr;
> +  uint32_t domainLen, userLen, hostLen;
> +  mozilla::CheckedInt<uint16_t> ntlmRespLen = NTLM_RESP_LEN;

why do you assign when you override the value later?

@@ +817,3 @@
>    cursor = WriteSecBuf(cursor, ntlmRespLen, offset);
>    if (ntlmv2) {
> +    if ((ntlmRespLen - NTLMv2_RESP_LEN - NTLMv2_BLOB1_LEN).value() != msg.targetInfoLen) {

please add a comment that ntlmRespLen is ensured to be >= NTLMv2_RESP_LEN + NTLMv2_BLOB1_LEN

Comment 59

[Andrew Bartlett]

Attachment: 0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Update NTLMv2 patch per comments.

Comment 60

[Andrew Bartlett]

Attachment: 0005-Use-mozilla-CheckedInt-in-NTLMv2-handler.patch

Updated CheckedInt additions per feedback

Comment 61

[Andrew Bartlett]

Attachment: 0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Sorry, forgot the formatting comment.  Fixed.

Comment 62

[Honza Bambas (:mayhemer)]

Andrew sorry, for late answer, but I'm lost among all the patches.  I think it's time to merge them to a single one, such that one should also be finally landed.

Can you please do it?  Thanks.

Comment 63

[Andrew Bartlett]

Thanks,  Yes, I realise the list is much more confusing that I intended it to be.  I'll squash it into a clearly ordered series without the CheckedInt changes as distinct patches.  We may wish to push LM removal to another bug, if that clarifies things further.

Comment 64

[Andrew Bartlett]

Comment on attachment 8465208 [details] [diff] [review]
0001-Use-mozilla-CheckedInt-in-NTLMv2-handler.patch

This obsolete patch was making the list of patches much less clear.

Comment 65

[Andrew Bartlett]

Comment on attachment 8483915 [details] [diff] [review]
0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

This is also obsolete.  The patches can now be read in numeric order.  I'll squash in the CheckedInt stuff tomorrow.

Comment 66

[Andrew Bartlett]

Attachment: 0001-Bug-423758-Revert-Bug-1030426-network.negotiate-auth.patch

Added correct Bug prefix to patch 1

Comment 67

[Andrew Bartlett]

Attachment: 0002-Bug-423758-Revert-Bug-1023748-Allow-NTLMv1-over-SSL-.patch

Comment 68

[Andrew Bartlett]

Attachment: 0003-Bug-423758-Remove-LM-code-from-internal-NTLM-handler.patch

Comment 69

[Andrew Bartlett]

Attachment: 0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Comment 71

[Honza Bambas (:mayhemer)]

Comment on attachment 8487512 [details] [diff] [review]
0001-Bug-423758-Revert-Bug-1030426-network.negotiate-auth.patch

Review of attachment 8487512 [details] [diff] [review]:
-----------------------------------------------------------------

Where did the 8 line context go?

Comment 72

[Honza Bambas (:mayhemer)]

Comment on attachment 8487515 [details] [diff] [review]
0002-Bug-423758-Revert-Bug-1023748-Allow-NTLMv1-over-SSL-.patch

Review of attachment 8487515 [details] [diff] [review]:
-----------------------------------------------------------------

As well here.

Comment 73

[Honza Bambas (:mayhemer)]

Comment on attachment 8487516 [details] [diff] [review]
0003-Bug-423758-Remove-LM-code-from-internal-NTLM-handler.patch

Review of attachment 8487516 [details] [diff] [review]:
-----------------------------------------------------------------

This is already f+'ed...  8 line context.

Comment 74

[Honza Bambas (:mayhemer)]

Attachment: (by Andrew Bartlett) 0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

- corrected context to 8 lines

Comment 75

[Honza Bambas (:mayhemer)]

Comment on attachment 8490055 [details] [diff] [review]
(by Andrew Bartlett) 0004-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Review of attachment 8490055 [details] [diff] [review]:
-----------------------------------------------------------------

Only about nits.  This is ready for review.  Next step is to finalize those few formatting details, merge all the patches to a single one (yes please!) and submit this time for a regular review from me _and_ from :keeler.

Thanks!

::: security/manager/ssl/src/nsNTLMAuthModule.cpp
@@ +271,5 @@
>  
>  static void *
> +WriteSecBuf(void *buf,
> +	    uint16_t length,
> +	    uint32_t offset)

this change is probably not needed.

@@ +744,1 @@
>    {

} else if () {

@@ +758,5 @@
>      NTLM_Hash(password, ntlmHash);
>      LM_Response(ntlmHash, sessionHash, ntlmResp);
>    }
>    else
>    {

maybe also fix this place:

} else {

@@ +781,5 @@
> +  }
> +  *outBuf = nsMemory::Alloc(totalLen.value());
> +  *outLen = totalLen.value();
> +  if (!*outBuf)
> +    return NS_ERROR_OUT_OF_MEMORY;

if () {
}

@@ +1089,5 @@
> +  if (PK11_DigestOp(ctxt, input, inputLen) != SECSuccess) {
> +    NS_ERROR("failed to do digest");
> +    goto done;
> +  }
> +  if (PK11_DigestFinal(ctxt, result, &resultLen, resultLen) != SECSuccess) {

blank line before this line

@@ +1106,5 @@
> +//-----------------------------------------------------------------------------
> +// MD5 support code
> +//
> +static nsresult hmac_md5sum(const uint8_t *key, uint32_t keyLen,
> +			    const uint8_t *input, uint32_t inputLen,

nit: maybe call this input1 and input1Len?

Comment 76

[Andrew Bartlett]

Attachment: 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Formatting fixes done and merged into a single patch

Comment 77

[Andrew Bartlett]

Attachment: 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

This patch includes the fixes by honza.

Comment 78

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Hi Andrew - I just wanted to let you know I am aware of this patch. I have started to review it, but I haven't been able to devote enough time to fully review it yet. I'm out of the office tomorrow, so it'll probably be sometime early next week by the time I finish.

Comment 79

[Honza Bambas (:mayhemer)]

Attachment: (By A. Bartlet) 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch (8 lines context)

Reuploaded with 8 line context....

Comment 80

[Honza Bambas (:mayhemer)]

Comment on attachment 8499726 [details] [diff] [review]
(By A. Bartlet) 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch (8 lines context)

Review of attachment 8499726 [details] [diff] [review]:
-----------------------------------------------------------------

David, please make sure there is no buffering boundary issue.  I don't see any.

r=honzab

Go go go!! :)

Comment 81

[Honza Bambas (:mayhemer)]

Comment on attachment 8497824 [details] [diff] [review]
0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Review of attachment 8497824 [details] [diff] [review]:
-----------------------------------------------------------------

r+'ed the 8 line ctx patch.

Comment 82

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8497824 [details] [diff] [review]
0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Review of attachment 8497824 [details] [diff] [review]:
-----------------------------------------------------------------

A couple of style nits:
- Use 'if (NS_FAILED(rv))' instead of 'if (rv != NS_OK)'
- use C++ style casts instead of C style casts
- use C++ style comments instead of C style comments
- use nullptr instead of NULL
- declare each variable on its own line, instead of all on one line
- use scoped types where appropriate (or use an encapsulated type - see comments)

The major issue is that the way this code reads data is unsafe in general. We really need to use something more like this: https://mxr.mozilla.org/mozilla-central/source/security/pkix/include/pkix/Input.h (it's basically a container that knows how long the buffer it points to is, can read data from it, but won't ever read beyond the end of it). Similarly, it would take a lot of effort to convince myself that the way data is written in this code is safe. We should have a similar Output class that knows how long its output buffer is, can write data to it, and won't ever write beyond the end of it. I realize I'm asking for some large changes, but we don't want this to be the next heartbleed-style bug.

::: security/manager/ssl/src/nsNTLMAuthModule.cpp
@@ +10,4 @@
>  #include "nsNativeCharsetUtils.h"
>  #include "prsystem.h"
>  #include "pk11pub.h"
> +#include "pk11priv.h"

nit: I believe this sorts before "pk11pub.h", but these are all out of order anyway, so maybe just sort this whole thing

@@ +16,5 @@
>  #include "mozilla/Telemetry.h"
>  #include "mozilla/Preferences.h"
> +#include "mozilla/Endian.h"
> +#include "nsUnicharUtils.h"
> +#include <time.h>

nit: non-mozilla includes go in a separate block before mozilla headers (see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices )

@@ +517,5 @@
>    LogFlags(msg->flags);
>    LogBuf("challenge", msg->challenge, sizeof(msg->challenge));
>  
> +  // Read (and skip) the reserved field
> +  ReadUint32(cursor);

Maybe I'm misunderstanding this, but I think we can read beyond the end of the buffer, here (in particular because even if targetEnd > inLen, this code still runs. In general, the way reads are done in this code is unsafe. We should really move to something more like this: https://mxr.mozilla.org/mozilla-central/source/security/pkix/include/pkix/Input.h )

@@ +532,5 @@
> +  // Check the offset / length combo is in range of the input buffer, including
> +  // integer overflow checking.
> +  if (MOZ_LIKELY(targetInfoEnd.isValid() && targetInfoEnd.value() <= inLen)) {
> +    msg->targetInfoLen = targetInfoLen;
> +    msg->targetInfo = ((const uint8_t *) inBuf) + offset;

nit: use C++ style casts: reinterpret_cast<const uint8_t *>(inBuf)

@@ +656,2 @@
>    //
> +  uint8_t lmResp[LM_RESP_LEN], ntlmResp[NTLM_RESP_LEN], ntlmv2Resp[NTLMv2_RESP_LEN], ntlmHash[NTLM_HASH_LEN];

nit: declare each variable on its own line

@@ +657,5 @@
> +  uint8_t lmResp[LM_RESP_LEN], ntlmResp[NTLM_RESP_LEN], ntlmv2Resp[NTLMv2_RESP_LEN], ntlmHash[NTLM_HASH_LEN];
> +  uint8_t ntlmv2_blob1[NTLMv2_BLOB1_LEN];
> +  if (ntlmv2) {
> +    // NTLMv2 mode, the default
> +    nsString userUpper, domainUpper;

nit: same here

@@ +663,5 @@
> +
> +    // temporary buffers for unicode strings
> +    nsAutoString ucsDomainUpperBuf, ucsUserUpperBuf;
> +    const void *domainUpperPtr, *userUpperPtr;
> +    uint32_t domainUpperLen, userUpperLen;

and here

@@ +674,5 @@
> +    ToUpperCase(username, ucsUserUpperBuf);
> +    userUpperPtr = ucsUserUpperBuf.get();
> +    userUpperLen = ucsUserUpperBuf.Length() * 2;
> +#ifdef IS_BIG_ENDIAN
> +    WriteUnicodeLE((void *) userUpperPtr, (const char16_t *) userUpperPtr,

nit: use C++ style casts

@@ +689,5 @@
> +    NTLM_Hash(password, ntlmHash);
> +
> +    rv = hmac_md5sum(ntlmHash, NTLM_HASH_LEN,
> +		     (uint8_t *)userUpperPtr, userUpperLen,
> +		     (uint8_t *)domainUpperPtr, domainUpperLen,

nit: casts

@@ +690,5 @@
> +
> +    rv = hmac_md5sum(ntlmHash, NTLM_HASH_LEN,
> +		     (uint8_t *)userUpperPtr, userUpperLen,
> +		     (uint8_t *)domainUpperPtr, domainUpperLen,
> +		     NULL, 0,

nit: nullptr, not NULL

@@ +692,5 @@
> +		     (uint8_t *)userUpperPtr, userUpperLen,
> +		     (uint8_t *)domainUpperPtr, domainUpperLen,
> +		     NULL, 0,
> +		     ntlmv2Hash, NTLMv2_HASH_LEN);
> +    if (rv != NS_OK) {

'if (NS_FAILED(rv))' here and elsewhere

@@ +697,5 @@
> +      return rv;
> +    }
> +
> +    uint8_t client_random[NTLM_CHAL_LEN];
> +    PK11_GenerateRandom(client_random, NTLM_CHAL_LEN);

Do we have any guarantees that NSS won't be shutting down while this is called? (see e.g. nsNSSShutDown.h). Maybe this should be delegated to some PSM interface that handles that part...

@@ +699,5 @@
> +
> +    uint8_t client_random[NTLM_CHAL_LEN];
> +    PK11_GenerateRandom(client_random, NTLM_CHAL_LEN);
> +
> +    /* Prepare the LMv2 response */

nit: use C++ style comments

@@ +703,5 @@
> +    /* Prepare the LMv2 response */
> +    rv = hmac_md5sum(ntlmv2Hash, NTLMv2_HASH_LEN,
> +		     msg.challenge, NTLM_CHAL_LEN,
> +		     client_random, NTLM_CHAL_LEN,
> +		     NULL, 0,

nit: nullptr, not NULL

@@ +713,5 @@
> +    memset(ntlmv2_blob1, 0, NTLMv2_BLOB1_LEN);
> +
> +    time_t unix_time;
> +    uint64_t nt_time = time(&unix_time);
> +    nt_time += 11644473600LL;

Is this value specified somewhere? We should document where it comes from.

@@ +771,5 @@
> +  totalLen += ntlmRespLen.value();
> +
> +  if (!totalLen.isValid()) {
> +     NS_ERROR("failed preparing to allocate NTLM response: integer overflow?!?");
> +     return NS_ERROR_UNEXPECTED;

NS_ERROR_FAILURE

@@ +1074,1 @@
>    PK11Context *ctxt = PK11_CreateDigestContext(SEC_OID_MD5);

Use a scoped type (see next comment), or use https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsICryptoHash.idl instead of directly calling NSS functions.

@@ +1111,5 @@
> +{
> +  SECItem keyItem, *param = nullptr;
> +  PK11SymKey *symkey = nullptr;
> +  PK11SlotInfo *slot = nullptr;
> +  PK11Context *ctxt = nullptr;

Use scoped types for these and avoid using goto: security/manager/ssl/src/ScopedNSSTypes.h (or, again, use nsICryptoHash).

Comment 83

[Andrew Bartlett]

Thanks for the feedback.  I've done the simple style changes, and I'll try and understand how to handle the larger suggestions over the next little while.

Comment 84

[Andrew Bartlett]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #82)

> @@ +517,5 @@
> >    LogFlags(msg->flags);
> >    LogBuf("challenge", msg->challenge, sizeof(msg->challenge));
> >  
> > +  // Read (and skip) the reserved field
> > +  ReadUint32(cursor);
> 
> Maybe I'm misunderstanding this, but I think we can read beyond the end of
> the buffer, here (in particular because even if targetEnd > inLen, this code
> still runs. In general, the way reads are done in this code is unsafe. We
> should really move to something more like this:
> https://mxr.mozilla.org/mozilla-central/source/security/pkix/include/pkix/
> Input.h )

I've just checked this over, and I'm confident that this is protected by this at the top of the function:

  if (inLen < NTLM_TYPE2_HEADER_LEN)
    return NS_ERROR_UNEXPECTED;

Comment 85

[Andrew Bartlett]

Attachment: 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

The attached patch hopefully addresses the major style issues.  I do apologise, I'm a C programmer at heart working on what was essentially C code and I'm still getting a good grip on the C++ side of things, 

The larger tasks requested are still pending, but any feedback on the fixes so far most welcome.

Thanks,

Comment 86

[Andrew Bartlett]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #82)
> Comment on attachment 8497824 [details] [diff] [review]
> 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch
> 
> Review of attachment 8497824 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +697,5 @@
> > +      return rv;
> > +    }
> > +
> > +    uint8_t client_random[NTLM_CHAL_LEN];
> > +    PK11_GenerateRandom(client_random, NTLM_CHAL_LEN);
> 
> Do we have any guarantees that NSS won't be shutting down while this is
> called? (see e.g. nsNSSShutDown.h). Maybe this should be delegated to some
> PSM interface that handles that part...

Can you please explain this part some more?  I'm not really up on the full detail of Mozilla internals, so I'll need a few more clues here. 
 

> @@ +1074,1 @@
> >    PK11Context *ctxt = PK11_CreateDigestContext(SEC_OID_MD5);
> 
> Use a scoped type (see next comment), or use
> https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/
> nsICryptoHash.idl instead of directly calling NSS functions.
> 
> @@ +1111,5 @@
> > +{
> > +  SECItem keyItem, *param = nullptr;
> > +  PK11SymKey *symkey = nullptr;
> > +  PK11SlotInfo *slot = nullptr;
> > +  PK11Context *ctxt = nullptr;
> 
> Use scoped types for these and avoid using goto:
> security/manager/ssl/src/ScopedNSSTypes.h (or, again, use nsICryptoHash).

I agree this new code, like the code before it is written much more in C than C++, but is the transformation a strictly required step to progress on this patch?  I've looked over the proposed interfaces, and I can't say they are much simpler for the multi-step cases required here.  (They may be a little simpler for the single-step case). 

Thanks,

Comment 87

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8503844 [details] [diff] [review]
0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Review of attachment 8503844 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good stylistically. Casts should probably be c++-style casts, not c-style.

::: security/manager/ssl/src/nsNTLMAuthModule.cpp
@@ +1081,2 @@
>    PK11Context *ctxt = PK11_CreateDigestContext(SEC_OID_MD5);
> +  if (!ctxt){

nit: space before '{'

@@ +1184,5 @@
> +  }
> +  rv = NS_OK;
> +
> +done:
> +  if (ctxt)

nit: braces around conditional bodies

Comment 88

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Andrew Bartlett from comment #86)
> (In reply to David Keeler (:keeler) [use needinfo?] from comment #82)
> > @@ +697,5 @@
> > > +      return rv;
> > > +    }
> > > +
> > > +    uint8_t client_random[NTLM_CHAL_LEN];
> > > +    PK11_GenerateRandom(client_random, NTLM_CHAL_LEN);
> > 
> > Do we have any guarantees that NSS won't be shutting down while this is
> > called? (see e.g. nsNSSShutDown.h). Maybe this should be delegated to some
> > PSM interface that handles that part...
> 
> Can you please explain this part some more?  I'm not really up on the full
> detail of Mozilla internals, so I'll need a few more clues here. 

NSS is the library that provides cryptographic implementations for Gecko to use. If it hasn't been initialized or it has already shut down, in general it isn't safe to use the functions it provides. Gecko code can prevent NSS from shutting down using what's outlined in nsNSSShutDown.h. If we have reason to believe that NSS can't shut down while this code is running, we can skip that. However, it would be easier to just use an implementation that already handles NSS shutdown - i.e. nsICryptoHash.

> > @@ +1074,1 @@
> > >    PK11Context *ctxt = PK11_CreateDigestContext(SEC_OID_MD5);
> > 
> > Use a scoped type (see next comment), or use
> > https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/
> > nsICryptoHash.idl instead of directly calling NSS functions.
> > 
> > @@ +1111,5 @@
> > > +{
> > > +  SECItem keyItem, *param = nullptr;
> > > +  PK11SymKey *symkey = nullptr;
> > > +  PK11SlotInfo *slot = nullptr;
> > > +  PK11Context *ctxt = nullptr;
> > 
> > Use scoped types for these and avoid using goto:
> > security/manager/ssl/src/ScopedNSSTypes.h (or, again, use nsICryptoHash).
> 
> I agree this new code, like the code before it is written much more in C
> than C++, but is the transformation a strictly required step to progress on
> this patch?  I've looked over the proposed interfaces, and I can't say they
> are much simpler for the multi-step cases required here.  (They may be a
> little simpler for the single-step case). 
> 
> Thanks,

It's much easier to convince yourself that an implementation is correct when using these scoped types. Also, see above re: nsICryptoHash.

Comment 89

[Andrew Bartlett]

Thanks for the feedback.  Can you give me an example of what the correct C++ style cast you are looking for here would be?  

I've also been trying to look over the Input routines you suggested, but I can't see how to read and compare strings or expected bytes such as the NTLMSSP signature, or how to extract out whole chunks of memory (compared with reading 1 or 2 bytes at a time).

Can you give me some other examples I can use as a reference?

Thanks,

Comment 90

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8503844 [details] [diff] [review]
0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Review of attachment 8503844 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNTLMAuthModule.cpp
@@ +533,5 @@
> +  // Check the offset / length combo is in range of the input buffer, including
> +  // integer overflow checking.
> +  if (MOZ_LIKELY(targetInfoEnd.isValid() && targetInfoEnd.value() <= inLen)) {
> +    msg->targetInfoLen = targetInfoLen;
> +    msg->targetInfo = ((const uint8_t *) inBuf) + offset;

This should probably be a 'reinterpret_cast<const uint8_t*>(inBuf)'

@@ +808,5 @@
> +     NS_ERROR("failed preparing to write NTLM response: integer overflow?!?");
> +     return NS_ERROR_UNEXPECTED;
> +  }
> +  cursor = WriteSecBuf(cursor, LM_RESP_LEN, offset.value());
> +  memcpy((uint8_t *) *outBuf + offset.value(), lmResp, LM_RESP_LEN);

This should probably be 'reinterpret_cast<uint8_t*>(*outBuf) + offset.value()' (or, since *outBuf gets used as a uint8_t* so much, maybe it should have its own alias)

Comment 91

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I pointed out some c++-style casts in comment 90.

Looks like we took out some of the Input functions I was thinking of. In any case, maybe it would be best to implement something specific to this use-case. I was thinking of something that would get used like this:

Input input(data, length);
// check that initialization succeeded
int32_t someValue;
rv = input.Read32(someValue);
// check for success, use someValue for something...
...
uint8_t bytes[] = { 0x0a, 0x0b, 0x0c, ... };
rv = input.ExpectBytes(bytes);
// check for success

Then, you can also do something like this:

Output output(totalLength);
// check that allocation succeeded
rv = output.Write32(someValue);
// check for success
rv = output.WriteBytes(data, len);
// check for success
...

*outBuf = output.getData();

Does this help?

Comment 92

[Honza Bambas (:mayhemer)]

Andrew, what is the status?  Can we expect next round of updates to the patch and walk towards landing this?  Do you need any help on our side?

Comment 93

[Andrew Bartlett]

G'Day Honza,

The status is that:
 - For the request to use Input/Output classes, I think the only realistic way forward is if the completed safe read/write classes are done first outside this effort, and then I can use them given suitable examples.  Otherwise, I would like to see this merged without them.
 - For the NSS shutdown risk, the GetNextToken call seems to already have protection.
 - For the request to use nsICryptoHash, I've got an attempt to use this for the md5sum() call, but it doesn't work.  For nsICryptoHMAC, needed for the hmac_md5sum() call, I would need an example of how to use nsICryptoHMAC in C, as it is not at all clear how to pass in a cleartext key to nsIKeyObject.

In terms of working patches not yet year, I only have a patch doing a few more C++ casts.

Thanks,

Comment 94

[Andrew Bartlett]

Attachment: 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Attached is an updated patch using the requested C++ classes for the MD5 and HMAC MD5 code.  It also covers the C++ casts as requested.

As mentioned above, the request for Input/Output isn't implemented, pending provision of such a facility by someone more qualified on the Mozilla codebase.

Thanks to Garming Sam for helping me out with understanding Mozilla's XPCOM. 

This passes simple tests, but current master in the git mirror isn't fully stable for me, even without this patch.

Comment 95

[Honza Bambas (:mayhemer)]

@David: I think it would be wiser to keep this bug and its patches just to provide an NTLMv2 in the same quality as we have NTLMv1 (the current code).  I was mostly OK with the patch before you started to review it, the patch was just adding the NTLMv2 logic bits.

If there is a concern that even the current unpatched code has, I would not bother in this bug.

We can have followups to fix anything more, beyond this task as making it more transparent and safe - I'm definitely not against!

What do you think?  This would otherwise take too long to deploy, it may be a trap ; what you suggest (a cleanup) can do an intern and I'd rather let Andrew work on something more matching his expertise area.


(In reply to Andrew Bartlett from comment #93)
> G'Day Honza,
> 
> The status is that:
>  - For the request to use Input/Output classes, I think the only realistic
> way forward is if the completed safe read/write classes are done first
> outside this effort, and then I can use them given suitable examples. 
> Otherwise, I would like to see this merged without them.

Sounds like David wants bug 966024 :)

>  - For the NSS shutdown risk, the GetNextToken call seems to already have
> protection.
>  - For the request to use nsICryptoHash, I've got an attempt to use this for
> the md5sum() call, but it doesn't work.  For nsICryptoHMAC, needed for the
> hmac_md5sum() call, I would need an example of how to use nsICryptoHMAC in
> C, as it is not at all clear how to pass in a cleartext key to nsIKeyObject.

Does the patch work w/o these changes?  If yes, I'd file followups.

> 
> In terms of working patches not yet year, I only have a patch doing a few
> more C++ casts.
> 
> Thanks,

Comment 96

[Andrew Bartlett]

(In reply to Honza Bambas (:mayhemer) from comment #95)
> @David: I think it would be wiser to keep this bug and its patches just to
> provide an NTLMv2 in the same quality as we have NTLMv1 (the current code). 
> I was mostly OK with the patch before you started to review it, the patch
> was just adding the NTLMv2 logic bits.
> 
> If there is a concern that even the current unpatched code has, I would not
> bother in this bug.
> 
> We can have followups to fix anything more, beyond this task as making it
> more transparent and safe - I'm definitely not against!
> 
> What do you think?  This would otherwise take too long to deploy, it may be
> a trap ; what you suggest (a cleanup) can do an intern and I'd rather let
> Andrew work on something more matching his expertise area.
> 
> 
> (In reply to Andrew Bartlett from comment #93)
> > G'Day Honza,
> > 
> > The status is that:
> >  - For the request to use Input/Output classes, I think the only realistic
> > way forward is if the completed safe read/write classes are done first
> > outside this effort, and then I can use them given suitable examples. 
> > Otherwise, I would like to see this merged without them.
> 
> Sounds like David wants bug 966024 :)
> 
> >  - For the NSS shutdown risk, the GetNextToken call seems to already have
> > protection.
> >  - For the request to use nsICryptoHash, I've got an attempt to use this for
> > the md5sum() call, but it doesn't work.  For nsICryptoHMAC, needed for the
> > hmac_md5sum() call, I would need an example of how to use nsICryptoHMAC in
> > C, as it is not at all clear how to pass in a cleartext key to nsIKeyObject.
> 
> Does the patch work w/o these changes?  If yes, I'd file followups.
> Thanks,

It was very unclear (no C++ examples for the nsICryptoHMAC), but we worked it out in the latest patch, so at least that part of the concerns are now addressed.

Comment 97

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Honza Bambas (:mayhemer) from comment #95)
> @David: I think it would be wiser to keep this bug and its patches just to
> provide an NTLMv2 in the same quality as we have NTLMv1 (the current code). 
> I was mostly OK with the patch before you started to review it, the patch
> was just adding the NTLMv2 logic bits.

Ok - we probably don't have to block this improvement on the other improvements I'd like to make to this code in general. I probably won't have time today, but I'll review this patch as soon as possible.

Comment 98

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8530721 [details] [diff] [review]
0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Review of attachment 8530721 [details] [diff] [review]:
-----------------------------------------------------------------

Please file two follow-up bugs:
* find a way to test our NTLM implementation
* replace the buffer reading/writing nsNTLMAuthModule does with safe, automatically bounds-checked reads/writes

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ -1172,5 @@
>                                          getter_Copies(result));
>    }
>  
> -  bool sendLM = Preferences::GetBool("network.ntlm.send-lm-response",
> -                                     SEND_LM_DEFAULT);

SEND_LM_DEFAULT is now unused in nsNSSComponent.cpp

::: security/manager/ssl/src/nsNTLMAuthModule.cpp
@@ +17,5 @@
>  #include "mozilla/Telemetry.h"
>  #include "mozilla/Preferences.h"
> +#include "mozilla/Endian.h"
> +#include "nsUnicharUtils.h"
> +#include "mozilla/CheckedInt.h"

Might as well sort this list alphabetically while you're adding new ones.

@@ +660,5 @@
> +    nsAutoCString ntlmHashStr;
> +    nsAutoCString ntlmv2HashStr;
> +    nsAutoCString lmv2ResponseStr;
> +    nsAutoCString ntlmv2ResponseStr;
> +    

nit: trailing whitespace

@@ +694,5 @@
> +    ntlmHashStr = nsAutoCString(reinterpret_cast<const char *>(ntlmHash), NTLM_HASH_LEN);
> +
> +    nsCOMPtr<nsIKeyObjectFactory> keyFactory =
> +        do_CreateInstance(NS_KEYMODULEOBJECTFACTORY_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

NS_ENSURE_SUCCESS hides a return, so we don't really want to use it anymore. Do this instead:

if (NS_FAILED(rv)) {
  return rv;
}

@@ +696,5 @@
> +    nsCOMPtr<nsIKeyObjectFactory> keyFactory =
> +        do_CreateInstance(NS_KEYMODULEOBJECTFACTORY_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<nsIKeyObject> ntlmKey = 

nit: trailing whitespace

@@ +700,5 @@
> +    nsCOMPtr<nsIKeyObject> ntlmKey = 
> +        do_CreateInstance(NS_KEYMODULEOBJECT_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    keyFactory->KeyFromString(nsIKeyObject::HMAC, ntlmHashStr, getter_AddRefs(ntlmKey));

Check the return value of this function.

@@ +714,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = hasher->Finish(false, ntlmv2HashStr);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    

nit: trailing whitespace (just delete this line)

@@ +718,5 @@
> +    
> +    uint8_t client_random[NTLM_CHAL_LEN];
> +    PK11_GenerateRandom(client_random, NTLM_CHAL_LEN);
> +
> +    nsCOMPtr<nsIKeyObject> ntlmv2Key = 

nit: trailing whitespace

@@ +723,5 @@
> +        do_CreateInstance(NS_KEYMODULEOBJECT_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // Prepare the LMv2 response
> +    keyFactory->KeyFromString(nsIKeyObject::HMAC, ntlmv2HashStr, getter_AddRefs(ntlmv2Key));

Check the return value of this function.

@@ +752,5 @@
> +    ntlmv2_blob1[0] = 1;
> +    ntlmv2_blob1[1] = 1;
> +    mozilla::LittleEndian::writeUint64(&ntlmv2_blob1[8], nt_time);
> +    PK11_GenerateRandom(&ntlmv2_blob1[16], NTLM_CHAL_LEN);
> +    

nit: trailing whitespace

@@ +859,5 @@
> +     return NS_ERROR_UNEXPECTED;
> +  }
> +  cursor = WriteSecBuf(cursor, ntlmRespLen.value(), offset.value());
> +  if (ntlmv2) {
> +    memcpy(reinterpret_cast<uint8_t*> (*outBuf) + offset.value(), ntlmv2Resp, NTLMv2_RESP_LEN);

This mixes two-space indents with four-space indents - we should be consistent with whatever is most common in this file.

Comment 99

[Andrew Bartlett]

Attachment: 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Updated patch correcting feedback on reviewed previous patch.

Comment 100

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I've had a long backlog of review requests. I should be able to address this tomorrow.

Comment 101

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8536277 [details] [diff] [review]
0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Review of attachment 8536277 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to use 8 lines of context (this patch appears to have 3).
r=me with nits addressed.

::: security/manager/ssl/src/nsNTLMAuthModule.cpp
@@ +494,3 @@
>    }
>    else
>    {

nit: if you feel like it, change this to:

'} else {'

(i.e. all on one line)

@@ +694,5 @@
> +    ntlmHashStr = nsAutoCString(reinterpret_cast<const char *>(ntlmHash), NTLM_HASH_LEN);
> +
> +    nsCOMPtr<nsIKeyObjectFactory> keyFactory =
> +        do_CreateInstance(NS_KEYMODULEOBJECTFACTORY_CONTRACTID, &rv);
> +    

nit: trailing whitespace

@@ +709,5 @@
> +    rv = keyFactory->KeyFromString(nsIKeyObject::HMAC, ntlmHashStr, getter_AddRefs(ntlmKey));
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> + 

nit: trailing whitespace

@@ +731,5 @@
> +    rv = hasher->Finish(false, ntlmv2HashStr);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +

nit: remove extra blank line

@@ +934,5 @@
>  
>    // 36 : user name sec buf
>    offset += domainLen;
> +  if (!offset.isValid()) {
> +     NS_ERROR("failed preparing to write NTLM response: integer overflow?!?");

nit: these lines should be indented 2 spaces, not 3

@@ +943,5 @@
>  
>    // 44 : workstation (host) name sec buf
>    offset += userLen;
> +  if (!offset.isValid()) {
> +     NS_ERROR("failed preparing to write NTLM response: integer overflow?!?");

same

Comment 102

[Garming Sam]

Attachment: 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Here's Andrew's patch with the last remaining nits fixed up and more lines of context.

Comment 103

[Andrew Bartlett]

Attachment: 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

And here is the patch after putting it via git-patch-to-hg-patch and adding r=keeler

Let us know if there are any issues.

Thanks,

Comment 104

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8ba1c9587ceb

Comment 105

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8538897 [details] [diff] [review]
0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

This is ready to go. I'll let the sheriffs check it in, though.

Comment 106

[Pavel Roskin]

Just tested the latest patch on top of mozilla-central on Ubuntu 14.10 i386. The build is working, no new warnings are introduced. NTLM authentication is working with my corporate server! Huge thanks to everybody who worked on it!

Comment 107

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fd0df8e208c

Comment 108

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4862425&repo=mozilla-inbound

Comment 109

[Garming Sam]

Attachment: 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

(In reply to Carsten Book [:Tomcat] from comment #108)
> sorry had to back this out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=4862425&repo=mozilla-
> inbound

Andrew's unfortunately on holiday for Christmas. However, I have managed to come up with a patch by copying most of the debug mozconfig and producing the same failures. 

The only difference I've made with this patch is some additional headers that were missing. 

I am rather curious as to why this didn't fail outright normally and why everything just worked.

Comment 110

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/0df4c88a32d2

Comment 111

[Carsten Book [:Tomcat]]

(In reply to Garming Sam from comment #109)
> Created attachment 8540420 [details] [diff] [review]
> 0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch
> 
> (In reply to Carsten Book [:Tomcat] from comment #108)
> > sorry had to back this out for bustage like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=4862425&repo=mozilla-
> > inbound
> 
> Andrew's unfortunately on holiday for Christmas. However, I have managed to
> come up with a patch by copying most of the debug mozconfig and producing
> the same failures. 
> 
> The only difference I've made with this patch is some additional headers
> that were missing. 
> 
> I am rather curious as to why this didn't fail outright normally and why
> everything just worked.

i guess i can not give any useful feedback for the patch itself here since i'm not a dev so i guess David or so can give a better feedback

Comment 112

[Pavel Roskin]

When I tried compiling Firefox using Ubuntu 14.10 package with the NTLMv2 patch, I also got errors from undefined symbols that were defined in some header files. When I compiled patched mozilla-cental, it worked. I guess it depends on the configuration. We should test all configurations.

Comment 113

[Andrew Bartlett]

Can someone please clarify what we need to do from here?

Thanks,

Comment 114

[Andrew Bartlett]

Comment on attachment 8540420 [details] [diff] [review]
0001-Bug-423758-Add-NTLMv2-to-internal-NTLM-handler.patch

Review of attachment 8540420 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for handling this Garming.  Looks good!

Comment 115

[Dana Keeler (she/her) (use needinfo) [:keeler]]

For good measure, I pushed this to try again:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b288c2baa04f
If/when those build successfully, you can add the 'checkin-needed' keyword to this bug to have a sheriff check the patch in.

Comment 116

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f09bfc814171

Comment 117

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f09bfc814171