Last Comment Bug 224653 - provide cross-platform NTLM auth implementation
: provide cross-platform NTLM auth implementation
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
-- enhancement with 2 votes (vote)
: mozilla1.6beta
Assigned To: Darin Fisher
: benc
: Patrick McManus [:mcmanus]
Depends on:
Blocks: 23679 152883 200436 222849
  Show dependency treegraph
Reported: 2003-11-03 19:04 PST by Darin Fisher
Modified: 2003-12-10 07:49 PST (History)
13 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1.0 patch (92.64 KB, patch)
2003-11-03 19:12 PST, Darin Fisher
no flags Details | Diff | Splinter Review
v1.1 patch - revised per comments from kai (87.52 KB, patch)
2003-11-10 15:39 PST, Darin Fisher
bryner: superreview+
Details | Diff | Splinter Review

Description User image Darin Fisher 2003-11-03 19:04:34 PST
this bug is about providing a cross-platform NTLM auth implementation that will
live in PSM (it leverages NSS) and can be used to provide NTLM auth for HTTP,

the implementation is based on

i filed this bug in an attempt to avoid the massive cc list of bug 23679... 
please do not mark this bug as a duplicate.  i'm hoping to get this in before
folks on that cc list get a chance to spam this bug.
Comment 1 User image Darin Fisher 2003-11-03 19:12:26 PST
Created attachment 134744 [details] [diff] [review]
v1.0 patch
Comment 2 User image Darin Fisher 2003-11-03 19:38:08 PST
to summarize:

  o  this patch moves the NTLM authentication behind a new interface, called
     nsIAuthModule.  that interface defines a simple contract for challenge/
     response based authenticator.  the NTLM authenticator is implemented in
     PSM so that it can leverage NSS crypto functions (DES_ECB and MD5).

  o  the NTLM hash requires the MD4 algorithm.  since NSS does not provide MD4,
     the patch includes a clean (from spec) MPL'd MD4 implementation.  the code
     for MD4 is pretty small and straightforward (if you have the spec in front
     of you!).

  o  this patch adds support for sending either: LMv1 + NTLMv1 _or_ the NTLM2 
     session response.  there is no support for LMv2 or NTLMv2 since those modes
     cannot be negotiated (they must be manually configured on the client and
     server).  i will implement those modes if we discover cases where those are 
     required.  in all cases, the NTLM2 session response, if negotiated 
     successfully, is preferred over even LMv2 or NTLMv2, so i feel that it is 
     most useful to have NTLM2 session response supported and less useful to 
     support LMv2 and/or NTLMv2.
  o  i have discarded the NTLM SSPI code.  i think we are better off using our
     own code for NTLM authentication.  the reason for this is simple.  on older
     clients, like Win9x, only the LMv1 response is generated by SSPI.  that is
     the least secure way of encrypting the user's password.  it is definitely a
     good idea to use our code on Win9x since it gives us the opportunity to
     authenticate the user w/ a NTLM2 session response.  also, SSPI has been a
     source of mysterious crashes on Win9x (see bug 222849 for example).  
     moreover, older versions of WinNT do not support the NTLM2 session 
     response, so we are better off on some versions of WinNT if we use our 
     code.  ah!  but, the tradeoff is that we may need to implement LMv2/NTLMv2
     if a site is configured to only allow those modes of authentication.

  o  i've added netwerk/test/ReadNTLM.cpp, which is a simple little program that
     decodes the NTLM messages.  this is useful for debugging a NTLM session,
     and for figuring out what IE and SSPI are doing ;-)

  o  i've decided to keep the base64 encoding/decoding in nsHttpNTLMAuth.cpp. 
     i was tempted to put it inside the NTLM auth module (since all of the 4
     protocols i previously mentioned need the NTLM messages to be base64 
     encoded).  but, i think the base64 encoding belongs closer to the protocol
     implementations since the encoding is a protocol dependent aspect.  plus,
     i can eliminate a buffer copy by keeping the base64 encoding in HTTP land.
     for the mail protocols, we can try to share the code for base64 encoding/

  o  i removed the remnants of the LanManager single singon code since we 
     weren't using it anyways.  (i.e., we always prompt the user at least once 
     for their username and password.)

lastly, i've tested this against 1) a Win2k server and 2) a Squid proxy
configured to use winbindd to authenticate with a NT domain server (Samba in my
testcase).  and, i've tested this code under Win2k and Linux.  there may be
issues on big-endian platforms (such as OSX).  i have not yet tested this code
on OSX.  that's next on my list ;-)
Comment 3 User image Kai Engert (:kaie) 2003-11-08 06:14:58 PST
Comment on attachment 134744 [details] [diff] [review]
v1.0 patch

r=kaie on the changes to existing files in security/manager/ssl

I'm giving a general moa=kaie on adding the new nsNTLMAuth* and md4* files to
security/manager/ssl, because the new service you are adding is self contained.

Please note that NTLM authentication will remain to be active in the session,
if the user activates FIPS mode. A restart is required to disable NTLM. You
should make sure this is compliant with FIPS requirements, or find a way to
disable NTLM at the same time FIPS gets activated.

Regarding the use of PK11_ functions, please make sure that you are cleaning up
NSS resources correctly, in order to not break the "switch profile at runtime"

For example, I do not see a call to PK11_FreeSlot, but I think you need to.
Comment 4 User image Darin Fisher 2003-11-10 15:15:15 PST
thanks for catching those mistakes kai!

  o  i'll add a PK11_IsFIPS() check in GetNextToken to have it fail if FIPS mode 
     has been enabled.

  o  and thanks for noticing the missing PK11_FreeSlot
Comment 5 User image Darin Fisher 2003-11-10 15:39:05 PST
Created attachment 135210 [details] [diff] [review]
v1.1 patch - revised per comments from kai
Comment 6 User image Brian Ryner (not reading) 2003-11-14 15:52:58 PST
Comment on attachment 135210 [details] [diff] [review]
v1.1 patch - revised per comments from kai

r/sr=bryner with the changes we discussed.
Comment 7 User image Kai Engert (:kaie) 2003-11-16 17:58:23 PST
Thanks for the new patch, Darin!
My r=kaie/moa=kaie still stands.
Comment 8 User image Darin Fisher 2003-11-17 18:21:22 PST
patch checked in for 1.6 beta :)
Comment 9 User image Aleksey Nogin 2003-11-17 21:10:17 PST
According to brad TBox, there are two "might be uised uninitialized" warnings in
the new code:

+ `PRUint32 inBufLen' might be used uninitialized in this function

+ `SECItem*param' might be used uninitialized in this function
Comment 10 User image Darin Fisher 2003-11-17 21:29:57 PST
aleksey: thanks for the notice!
Comment 11 User image Darin Fisher 2003-11-18 21:00:01 PST
i applied a fix for those warnings.  thanks again aleksey!
Comment 12 User image Manik Surtani 2003-12-08 03:19:50 PST
Hi, which CVS branch are these fixes in?  Would they have been built into the
latest nightlies?

Comment 13 User image Christian :Biesinger (don't email me, ping me on IRC) 2003-12-08 09:35:10 PST
Manik: only on HEAD. yes, nightlies contain it.
Comment 14 User image Simon Janes 2003-12-10 07:49:39 PST
I know this one is resolved, but is there a reason why this couldn't be extended
to NNTP authentication in addition to HTTP, SMTP, IMAP and POP3?

Note You need to log in before you can comment on or make changes to this bug.