Closed Bug 496234 Opened 11 years ago Closed 5 years ago

NTLM authentication fails on passwords > 28 characters

Categories

(Core :: Networking, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: neinbrucke, Assigned: keeler)

References

Details

Attachments

(3 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10 (.NET CLR 3.5.30729)

Authentication against a webserver that requests NTLM authentication fails when a password longer then 28 characters has been set.

Reproducible: Always

Steps to Reproduce:
1.Set a (domain) Windows password of < 28 characters
2.Try to logon to a webserver using NTLM authentication using this password. (this should work)
3.Logout
4.Set a (domain) Windows password of > 28 characters
5.Try to logon to the webserver again.
Actual Results:  
This last step fails. User account doesn't get locked out on this one, so credentials are probably not even sent to the server.

Expected Results:  
Successful logon was expected.

This limit of 28 characters might be related to the NTLM/MD4 implementation. For NTLM a Unicode password is run through the MD4 algorithm. This means that with 28 characters, 56 input bytes go through MD4_update. Longer inputs require multiple rounds of MD4.
OS: Windows Vista → All
Component: Security → Networking
Product: Firefox → Core
QA Contact: firefox → networking
Version: unspecified → 1.9.0 Branch
I've faced the same issue in Firefox/Iceweasel 3.0.9-1 from current Debian GNU/Linux Testing with all updates.
I've run into this myself on Ubuntu 11.04 Firefox 7.0.1
Had a password over 28 characters long, and the site wouldn't take it.  Changed my domain password to something under that, and now it works great.  
The site runs on Windows server IIS so I'm guessing this is NTLM auth.  On internal pages that didn't directly use the IIS auth, my long password worked fine.
What's weird is that Firefox on windows 7 does not have this problem and will work fine with a long password.
Also the long passwords work on Chromium under Ubuntu.
I can reproduce this with
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:23.0) Gecko/20100101 Firefox/23.0
when authenticating to IIS6 and IIS7 servers.

I cannot reproduce with
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
when authenticating to the same servers. This leads me to believe it's platform-specific.

On Firefox on Mac I have no problem with passwords shorter than or equal to 27 characters. Passwords longer than or equal to 28 characters fail.
The initial report seems quite likely correct.  The MD4 code here differs to that in Samba, and while some is just implementation style, I agree that > 56 is a special case in MD4 and the code paths diverge in this area in particular.  

The first step is to add the RFC1320 test vectors to the build, then to either fix or replace the MD4 code if needed.
This patch follows Samba's example in using the RFC1320 test values in a test rig and shows that the MD4 routine is likely faulty.
I can also confirm I have reproduced this with mod_ntlm_winbind and Samba's ntlm_auth.  Using the --password argument to ntlm_auth makes it trivial to set a 28 and 27 char password alternately, and to then copy them into the password buffer.

Can someone please mark this confirmed?
Depends on: 1176125
Assignee: nobody → abartlet
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8622860 [details] [diff] [review]
0001-Bug-496234-Show-that-the-MD4-routines-in-Mozilla-are.patch

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

The test shows the issue up nicely, but I can't figure out how to get it included in the main make test.

Additionally, I would like some confirmation that the #include of the C file is an acceptable approach here.  I know some consider it distasteful, but it seems better than creating a library for this test.
Attachment #8622860 - Flags: feedback?(dkeeler)
Version: 1.9.0 Branch → Trunk
Attachment #8622860 - Flags: feedback?(honzab.moz)
Hi Andrew, I am happy for the md4.c and md4.h code I wrote for Samba to be re-licensed as needed for Mozilla. I think a dual-license as MPL and GPLv3+ makes most sense, but if another license is needed then please ask.
Cheers, Tridge
Comment on attachment 8622860 [details] [diff] [review]
0001-Bug-496234-Show-that-the-MD4-routines-in-Mozilla-are.patch

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

If this actually runs and doesn't break our test runs then I'm ok.  What results does it give you, Andrew?
Attachment #8622860 - Flags: feedback?(honzab.moz) → feedback+
Comment on attachment 8622860 [details] [diff] [review]
0001-Bug-496234-Show-that-the-MD4-routines-in-Mozilla-are.patch

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

Thanks for preparing this. When I run it, it fails on longer inputs, indicating our md4 implementation may be incorrect. I'll investigate further, but I might not have time this week.
Attachment #8622860 - Flags: feedback?(dkeeler) → feedback+
Correct, that's the point.  So the plan is to use the permission from Tridge above to bring in md4.c and md4.h from Samba under the MPL.

What I couldn't work out is how to make the test run in the testsuite, or perhaps I just don't know how to run the testsuite correctly.
The attached patch fixes the issue for me, by using Samba's code, kindly dual-licensed by Tridge.

Tridge,

Can you confirm this licence header is OK?

Thanks!
Attachment #8622860 - Attachment is obsolete: true
Attachment #8625630 - Flags: review?(dkeeler)
Attachment #8625630 - Flags: feedback?(andrew)
bug 496234 - use stdint types in md4 implementation

Also removes some trailing whitespace.
Attachment #8626263 - Flags: review?(honzab.moz)
bug 496234 - fix md4 implementation by appending the input length as a 64-bit number
Attachment #8626264 - Flags: review?(honzab.moz)
bug 496234 - add test vectors from RFC 1320 for md4 implementation
Attachment #8626265 - Flags: review?(honzab.moz)
Comment on attachment 8625630 [details] [diff] [review]
Replace MD4 code with code from Samba, add tests to show correctness

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

It turns out the error was fairly straight-forward: the RFC specifies that the length in bits be appended as a 64-bit number, which the original implementation was not doing. For long inputs, it meant that 4 bytes of uninitialized memory were being hashed, leading to incorrect hash values. I don't think we need to replace the implementation entirely at this point.
Attachment #8625630 - Flags: review?(dkeeler) → review-
(In reply to David Keeler [:keeler] (use needinfo?) from comment #17)
> Comment on attachment 8625630 [details] [diff] [review]
> Replace MD4 code with code from Samba, add tests to show correctness
> 
> Review of attachment 8625630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It turns out the error was fairly straight-forward: the RFC specifies that
> the length in bits be appended as a 64-bit number, which the original
> implementation was not doing. For long inputs, it meant that 4 bytes of
> uninitialized memory were being hashed, leading to incorrect hash values. I
> don't think we need to replace the implementation entirely at this point.

What would you like me to do from this point?  Should I just assign this back to you?

Thanks,
Sure - I guess it makes sense to assign the bug to me. I incorporated your test - if you're interested in taking a look, that would be great.
Assignee: abartlet → dkeeler
Attachment #8626263 - Flags: review?(honzab.moz) → review+
Comment on attachment 8626263 [details]
MozReview Request: bug 496234 - use stdint types in md4 implementation r=mayhemer

https://reviewboard.mozilla.org/r/12023/#review10665

Ship It!
Comment on attachment 8626264 [details]
MozReview Request: bug 496234 - fix md4 implementation by appending the input length as a 64-bit number

https://reviewboard.mozilla.org/r/12025/#review10667

::: security/manager/ssl/md4.c:130
(Diff revision 1)
> +  uint32_t inputLenBitsLow = (inputLenBits & 0xFFFFFFFF);

maybe explicitely cast to uint32_t?

and what about endianness?  MD4 wants it in little endian.
Attachment #8626264 - Flags: review?(honzab.moz)
Comment on attachment 8626265 [details]
MozReview Request: bug 496234 - add test vectors from RFC 1320 for md4 implementation

https://reviewboard.mozilla.org/r/12027/#review10681

::: security/manager/ssl/tests/compiled/TestMD4.cpp:22
(Diff revision 1)
> +      .data = "",

doesn't build for me in VC++ 2013 (v12)
Attachment #8626265 - Flags: review?(honzab.moz)
https://reviewboard.mozilla.org/r/12025/#review10667

> maybe explicitely cast to uint32_t?
> 
> and what about endianness?  MD4 wants it in little endian.

This is handles in the w2b() routines, and in the fact that we are first selecting the little end (the low bits), and then the big end (only really for the purpose of filling in the buffer with zeros, nobody will have a 2GB password).
https://reviewboard.mozilla.org/r/12025/#review10667

I don't know if mozilla normally worries about this, but I'm getting these warnings on my gcc version 4.9.2 (Debian 4.9.2-21)

1:01.58 In file included from /data/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/security/manager/ssl/Unified_c_security_manager_ssl0.c:2:0:
 1:01.58 /data/mozilla/mozilla-central/security/manager/ssl/md4.c: In function ‘md4sum’:
 1:01.59 Warning: -Wdeclaration-after-statement in /data/mozilla/mozilla-central/security/manager/ssl/md4.c: ISO C90 forbids mixed declarations and code
 1:01.59 /data/mozilla/mozilla-central/security/manager/ssl/md4.c:129:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 1:01.59    uint64_t inputLenBits = inputLen << 3;
 1:01.59    ^
 1:01.59 Warning: -Wdeclaration-after-statement in /data/mozilla/mozilla-central/security/manager/ssl/md4.c: ISO C90 forbids mixed declarations and code
 1:01.59 /data/mozilla/mozilla-central/security/manager/ssl/md4.c:132:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 1:01.59    uint32_t inputLenBitsHigh = ((inputLenBits >> 32) & 0xFFFFFFFF);
 1:01.59    ^
https://reviewboard.mozilla.org/r/12025/#review10667

In other news, I can confirm that a 30 char NTLM password works.
(In reply to Andrew Bartlett from comment #24)
> https://reviewboard.mozilla.org/r/12025/#review10667
> 
> I don't know if mozilla normally worries about this, but I'm getting these
> warnings on my gcc version 4.9.2 (Debian 4.9.2-21)
> 
> 1:01.58 In file included from
> /data/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/security/manager/
> ssl/Unified_c_security_manager_ssl0.c:2:0:
>  1:01.58 /data/mozilla/mozilla-central/security/manager/ssl/md4.c: In
> function ‘md4sum’:
>  1:01.59 Warning: -Wdeclaration-after-statement in
> /data/mozilla/mozilla-central/security/manager/ssl/md4.c: ISO C90 forbids
> mixed declarations and code
>  1:01.59 /data/mozilla/mozilla-central/security/manager/ssl/md4.c:129:3:
> warning: ISO C90 forbids mixed declarations and code
> [-Wdeclaration-after-statement]
>  1:01.59    uint64_t inputLenBits = inputLen << 3;
>  1:01.59    ^
>  1:01.59 Warning: -Wdeclaration-after-statement in
> /data/mozilla/mozilla-central/security/manager/ssl/md4.c: ISO C90 forbids
> mixed declarations and code
>  1:01.59 /data/mozilla/mozilla-central/security/manager/ssl/md4.c:132:3:
> warning: ISO C90 forbids mixed declarations and code
> [-Wdeclaration-after-statement]
>  1:01.59    uint32_t inputLenBitsHigh = ((inputLenBits >> 32) & 0xFFFFFFFF);
>  1:01.59    ^

These should be fixed.
Comment on attachment 8626263 [details]
MozReview Request: bug 496234 - use stdint types in md4 implementation r=mayhemer

bug 496234 - use stdint types in md4 implementation r=mayhemer

Also removes some trailing whitespace.
Attachment #8626263 - Attachment description: MozReview Request: bug 496234 - use stdint types in md4 implementation → MozReview Request: bug 496234 - use stdint types in md4 implementation r=mayhemer
Attachment #8626263 - Flags: review+ → review?(honzab.moz)
Comment on attachment 8626264 [details]
MozReview Request: bug 496234 - fix md4 implementation by appending the input length as a 64-bit number

bug 496234 - fix md4 implementation by appending the input length as a 64-bit number
Attachment #8626264 - Flags: review?(honzab.moz)
Comment on attachment 8626265 [details]
MozReview Request: bug 496234 - add test vectors from RFC 1320 for md4 implementation

bug 496234 - add test vectors from RFC 1320 for md4 implementation
Attachment #8626265 - Flags: review?(honzab.moz)
Comment on attachment 8626263 [details]
MozReview Request: bug 496234 - use stdint types in md4 implementation r=mayhemer

bug 496234 - use stdint types in md4 implementation r=mayhemer

Also removes some trailing whitespace.
Attachment #8626263 - Flags: review?(honzab.moz)
Thanks for the reviews. I believe I've addressed the issues noted. Here's a windows try run to be sure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbb8c6d98b54
Attachment #8625630 - Attachment is obsolete: true
Attachment #8625630 - Flags: feedback?(andrew)
Attachment #8626264 - Flags: review?(honzab.moz) → review+
Comment on attachment 8626264 [details]
MozReview Request: bug 496234 - fix md4 implementation by appending the input length as a 64-bit number

https://reviewboard.mozilla.org/r/12025/#review10869

Ship It!
Comment on attachment 8626265 [details]
MozReview Request: bug 496234 - add test vectors from RFC 1320 for md4 implementation

https://reviewboard.mozilla.org/r/12027/#review10871

Ship It!
Attachment #8626265 - Flags: review?(honzab.moz) → review+
You need to log in before you can comment on or make changes to this bug.