Last Comment Bug 335924 - No authentication with DIGEST-MD5
: No authentication with DIGEST-MD5
Status: ASSIGNED
[patchlove]
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: All All
: -- enhancement with 7 votes (vote)
: ---
Assigned To: Christian Eyrich
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-29 10:28 PDT by Joern W. Andersen
Modified: 2014-08-26 15:24 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
implementation of Digest-MD5 for SASL (89.50 KB, patch)
2006-09-25 09:59 PDT, Christian Eyrich
no flags Details | Diff | Splinter Review
implementation using a component through an interface (68.98 KB, patch)
2006-10-11 06:12 PDT, Christian Eyrich
no flags Details | Diff | Splinter Review
proposed patch v3 (70.57 KB, patch)
2006-11-25 10:24 PST, Christian Eyrich
no flags Details | Diff | Splinter Review
proposed patch v4 (70.56 KB, patch)
2007-01-09 05:13 PST, Christian Eyrich
no flags Details | Diff | Splinter Review
proposed patch v5 (73.10 KB, patch)
2007-05-30 11:14 PDT, Christian Eyrich
no flags Details | Diff | Splinter Review

Description Joern W. Andersen 2006-04-29 10:28:32 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2

Trying to connect to a mailbox on a Cyrus server, accepting DIGEST-MD5 only, is
rejected with message:
"You cannot log in to mail.servername.tld bacuse you have enabled secure authentication and this server does not support it"
Yes it does. I can login with other tools (i.e. imtest) with DIGEST-MD5, and
the mailserver accepts it. As soon as I additionally enable CRAM-MD5 on the
server, Firefox can login via CRAM. Firefox is brand new, vers. 1.5.0.2.
Same problem occurs when i use the mail client from Mozilla 1.7.13.
Regards, Joe.-

Reproducible: Always

Steps to Reproduce:
1. Login to a amilbox on a server, accepting DIGEST_MD5 only
2.
3.

Actual Results:  
As described

Expected Results:  
Accepting the login via DIGEST-MD5
Comment 1 David :Bienvenu 2006-04-29 11:16:32 PDT
right, we don't support digest-md5 - changing to enhancement.
Comment 2 Christian Eyrich 2006-09-25 09:57:41 PDT
taking
Comment 3 Christian Eyrich 2006-09-25 09:59:39 PDT
Created attachment 240023 [details] [diff] [review]
implementation of Digest-MD5 for SASL

The intention of this patch is to introduce Digest authentication mechanism for SASL while preventing doubling code. Implementation of Digest authentication for HTTP according to RFC 2617 currently lives in nsHttpDigestAuth. I wanted to reuse most of the code there and extend it with what's new and unique to Digest for SASL as described in RFC 2831.

Therefore I moved the main part to a class on its own (nsDigestMd5Auth) and rewrote nsHttpDigestAuth to only contain HTTP specific stuff and then call the general GenerateCredentials().
Analogue to this, the new nsMsgProtocol::GenerateDigestCredentials() prepares SASL specific stuff and then calls the general GenerateCredentials() too.

Flaws of the implementation in the current patch:
1. The "general" Digest code is contained twice: once in nsHttpDigestMd5Auth and once in nsDigestMd5Auth. Apart from the class names, it's the very same code. The reason for that is, that I don't know where I should place the one implementation in order to make it generally available (i.e. to nsHttpDigestAuth as well as to nsMsgProtocol).
I already tried adding "msgbaseutil" to "Requires" in Makefile.in of the nkhttp_s library and creating a nsDigestMd5Auth object in nsHttpDigestAuth. While that compiled, it crashed at runtime.

2. mResponseAuth is saved in nsMsgProtocol while it would be righter in nsDigestMd5Auth. But then that object must be kept alive between step1 and step2 of the authentication which is what I don't know how to do.

3. If response-auth from the server is wrong "*" is issued (authentication cancelled), but simply restarted after our normal fallback, using the next weaker mechanism.
Instead the login should be cancelled completely and the user should get alerted since wrong resp-auth indicates forgery.


So while Digest authentication is working with my patch for Http as well as SMTP, POP3 and IMAP, its implementation is suboptimal. I need help at least for fixing 1, while it would be nice for 2 and 3 too.
Comment 4 David :Bienvenu 2006-09-25 10:02:12 PDT
Thx very much for taking this on, Christian! I'm cc'ing Darin who's the nsIAuthModule expert, and I think also the implementor of http digest md5.
Comment 5 timeless 2006-09-25 10:29:29 PDT
you can't possibly have http depend on mailnews, we rarely build mailnews.

what you'll have to do is make an xpcom component to express digest. it sounds like you already made an interface, that's basically giving the object a contract in the http module file, and then do_CreateInstance(CONTRACT) from the mailnews and http consumers.
Comment 6 David :Bienvenu 2006-09-25 10:33:45 PDT
yes, as timeless says, http can't depend on mailnews, but mailnews can depend on http, via xpcom. Should/Can DIGEST-MD5 hide behind an nsIAuthModule implementation?
Comment 7 Christian Eyrich 2006-09-25 10:55:22 PDT
Thanks for your feedback, I just put nsDigestMd5Auth in Mailnews since I was able to use it there. I'm fine with moving it somewhere else - either in http or a real commons place. Just don't have any experience with creating components.

I'll try and shout for help if I don't get it.
Comment 8 Christian Eyrich 2006-09-25 14:45:34 PDT
So now I've got a problem accessing the Digest stuff through nsIAuthModule interface. I need to get the following in:
const PRUnichar *username,
const PRUnichar *password
PRBool aSASL,
PRBool aRequireExtraQuotes,
nsAFlatCString aMethod,
nsAFlatCString aUri,
PRUint32 aNc

But hhat Init method only has the parameters
const char *serviceName,
PRUint32 serviceFlags,
const PRUnichar *domain,
const PRUnichar *username,
const PRUnichar *password

Even if I abuse serviceName and domain, that's not enough. What's to do?
Comment 9 David :Bienvenu 2006-09-25 15:44:29 PDT
I'm not sure - I'm not even sure if nsIAuthModule is the way to go, and I'd like to hear from Darin about that before you spend too much time on a potentially wrong approach .

If nsIAuthModule is the way to go, you could perhaps do one or more of the following:

1. Really really abuse serviceName, and pass in all the extra args you need, in string form.

2. Encode the aSASL and aRequireExtraQuotes in the contract id used to create the right auth module - so there would be four different digest-md5 contract ids, 1-4, for the four possible values of aSasl and aRequireExtraQuotes. This seems like a real pain...

3. add an nsIDigestMD5 interface, and make the object that implements nsIAuthModule for digest also implement nsIDigestMD5, and the client code would have to QI to the nsIDigestMD5 interface to init the object correctly. The nsIDigestMD5 interface would just have a method or two to allow you to init it, and maybe store the extra data you talked about in your previous comments.

Of course, if you're going to need a new interface anyway, there might not be a big advantage to using nsIAuthModule...it just depends on how it looks in the client code.
Comment 10 Christian Eyrich 2006-09-26 11:09:58 PDT
> I'd like to hear from Darin about that before you spend too much time on a
> potentially wrong approach .

The approach of putting it into a component is surely the right, so I think I didn't waste my time. If I provide the said values via literals for testing, it already works through nsIAuthModule and using another interface isn't that much work.

Adding another or a completely own interface would seems the cleanest approach to me. But if I didn't err yesterday, Darin is excluded from getting bugmail for this bug.
Comment 11 David :Bienvenu 2006-09-26 11:19:32 PDT
Darin's cc'ed on the bug so I think he'd see these comments...
Comment 12 Christian Eyrich 2006-10-11 06:12:23 PDT
Created attachment 241935 [details] [diff] [review]
implementation using a component through an interface

In this new patch I introduced a new interface nsIAuthDigestModule. This is used to access the DigestAuth module from mailnews and http, so no doublicate code anymore. This works ok for me, but any thoughts are welcome.
Comment 13 Darin Fisher 2006-11-22 14:57:27 PST
My advice:  The approach taken by this patch seems fine to me.  If you wanted to make use of some of the nsIAuthModule API, then you could just have your interface extend the nsIAuthModule interface to provide a suitable initialization method.  However, if it feels like too much of an awkward fit, then just go with the custom interface as you have done here.
Comment 14 Christian Eyrich 2006-11-23 04:23:59 PST
Thanks for your evaluation. I tried extending nsIAuthModule but none of the existing methods were useful. If not violating conventions or messing up something, I'd like to use that second approach, yes.
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2006-11-24 18:28:17 PST
+#define NS_AUTH_MODULE_CONTRACTID_PREFIX \

that's already defined in nsIAuthModule, please don't redefine it here.

also, it would be nice to limit the line length to 80 characters
Comment 16 Christian Eyrich 2006-11-25 03:28:55 PST
(In reply to comment #15)
> +#define NS_AUTH_MODULE_CONTRACTID_PREFIX \
> 
> that's already defined in nsIAuthModule, please don't redefine it here.

Ok, but what then? Only use another symbolic name or another value too? Or would it be ok to remove that define and just additionally include "nsIAuthModule.h"?
Comment 17 Christian :Biesinger (don't email me, ping me on IRC) 2006-11-25 06:07:18 PST
another name I'd say, since it doesn't implement nsIAuthModule.
Comment 18 Christian Eyrich 2006-11-25 10:24:16 PST
Created attachment 246550 [details] [diff] [review]
proposed patch v3

Same es previous patch but with changes according to comment 15.
Comment 19 Christian Eyrich 2006-12-04 05:40:06 PST
Comment on attachment 246550 [details] [diff] [review]
proposed patch v3

Trying to get review.
Comment 20 David :Bienvenu 2006-12-31 09:13:12 PST
Hey Darin, are you going to be able to review this, or is there anyone else we can request review from?
Comment 21 Christian Eyrich 2007-01-09 05:13:27 PST
Created attachment 250952 [details] [diff] [review]
proposed patch v4

Some servers can't cope with space after the commas in digest-response. I've removed them in this patch.
Comment 22 Darin Fisher 2007-01-10 14:30:55 PST
I'll try to review this shortly.
Comment 23 Christian Eyrich 2007-05-30 11:10:14 PDT
Comment on attachment 250952 [details] [diff] [review]
proposed patch v4

Removing Review since patch is obsolete.
Comment 24 Christian Eyrich 2007-05-30 11:14:32 PDT
Created attachment 266636 [details] [diff] [review]
proposed patch v5

Updated patch for compatibility with current repository. Next try to get review.
Comment 25 Simon Wilkinson 2007-10-13 12:50:08 PDT
It would be nice if all of the SASL mechanisms for mailnews could be implemented via nsIAuthModule. That would mean that they could be easily implemented with a single, reusable loop of code in each protocol, and that it would be very straightforward to add them to LDAP for address book usage, too.
Comment 26 Wayne Mery (:wsmwk, NI for questions) 2009-01-22 13:12:01 PST
cbiesinger, ping re: review ... unless Christian redirects
Comment 27 Nikolay Shopik 2009-08-15 12:54:58 PDT
Wayne, we probably should ping someone else for review.
Comment 28 David :Bienvenu 2009-08-15 13:06:44 PDT
I think MoCo has a new person working on Netwerking code...maybe someone on the cc list knows who that is.
Comment 29 Phil Ringnalda (:philor) 2009-08-15 17:59:25 PDT
That'd be jduell.
Comment 30 Mark Banner (:standard8) 2011-02-03 02:23:01 PST
Comment on attachment 266636 [details] [diff] [review]
proposed patch v5

cbiesinger isn't going to have time to review this, so cancelling the review there.

Jason, Simon, before we look at getting this patch updated, is it heading in a reasonable direction? or does it need restructuring in a better way (I'm looking to clarify on comment 25 here).
Comment 31 Nikolay Shopik 2012-01-12 06:19:54 PST
DIGEST-MD5 now become obsoleted by rfc5802 - Salted Challenge Response Authentication Mechanism (SCRAM) be reasons mention in rfc6331
Comment 32 Jason Duell [:jduell] (needinfo me) 2012-01-12 20:37:46 PST
Yes, and from wikipedia I see as of 2008 "US-CERT now says that MD5 "should be considered cryptographically broken and unsuitable for further use."

The auth code source code has changed a lot, so this patch doesn't apply at all any more.  IIRC the changes were mostly code movement (to accomodate sharing it with websockets), so it might be not too much work to unbitrot.

But I'd want to hear from someone with security chops that we actually really want this before moving forward.  I suspect this is WONTFIX, unless there's a sizable number of mail servers out there that are actually using it.
Comment 33 franz.wudy 2012-11-18 19:58:19 PST
Hello,
well it sure would be a nice thing to at least `encrypt` passwords in a secure and reliable manner.
If someone does not have the ability to TLS-encrypt the data stream per se, certain rules and regulations in certain countries (France, Russia, etc) might make this necessary, at least preventing someone else from listening into your transfers and stealing your passwords would be a nice thing.
And to be honest, as email traffic (content) is heavily monitored by the various institutions, I don't know if a transport encryption is a overally necessary thing in most cases as the damage has been done already.

But at least the passwords should be safe!

MD5 has been know to be insecure for quite a while now. Stuxnet & Co have shown what happens if you rely on MD5 in Microsoft's case, so something more sophisticated is needed.

SHA in is various configurations still seems to be a good choice for hashing algorithms.

DOVECOT imap servers as well as Cyrus and (i believe) UW's impad do support at least the "salted" SCRAM-SHA-1 scheme for quite a while now. SCRAM-SHA-1-PLUS is being worked on.

http://www.dovecot.org/list/dovecot/2011-September/061172.html
DOVECOT has been tested against GNU SASL.

As the various MD5 based schemes are highly insecure a SCRAM-SHA-1 impementation should not be an optional thing, yet it is necessary.

MD5-based schemes (CRAM, DIGEST, etc) should either not be allowed any more or at least the user should be warned that he is about to give up some of his most essential information.

So a common and safe denominator between servers and a client is SCRAM-SHA-1 these days and it should be implemented.

Thanks -
Franz
Comment 34 franz.wudy 2012-11-18 20:00:55 PST
Should we rename this bug or open a new one?
Comment 35 David Lechner (:dlech) 2012-11-19 13:59:58 PST
(In reply to franz.wudy from comment #34)
> Should we rename this bug or open a new one?

Open a new bug for suggesting using SCRAM-SHA-1.
Comment 36 Chris Newman 2014-08-26 15:24:08 PDT
Due to http://tools.ietf.org/html/rfc6331, I suggest making this WONTFIX.

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