Closed Bug 335924 Opened 19 years ago Closed 6 years ago

No authentication with DIGEST-MD5

Categories

(MailNews Core :: Networking: IMAP, enhancement)

Product:
MailNews Core
Component:
Networking: IMAP
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: jwa, Assigned: ch.ey)

Details

(Whiteboard: [patchlove])

Attachments

(1 file, 4 obsolete files)

proposed patch v5
73.10 KB, patch
Details | Diff | Splinter Review
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
right, we don't support digest-md5 - changing to enhancement.
Assignee: mscott → bienvenu
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Component: General → Networking: IMAP
Product: Thunderbird → Core
Version: unspecified → Trunk
taking
Assignee: bienvenu → ch.ey
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.
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.
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.
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?
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.
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?
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.
> 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.
Darin's cc'ed on the bug so I think he'd see these comments...
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.
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.
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.
Attachment #240023 - Attachment is obsolete: true
+#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
(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"?
another name I'd say, since it doesn't implement nsIAuthModule.
Attached patch proposed patch v3 (obsolete) — — Splinter Review
Same es previous patch but with changes according to comment 15.
Attachment #241935 - Attachment is obsolete: true
Comment on attachment 246550 [details] [diff] [review] proposed patch v3 Trying to get review.
Attachment #246550 - Flags: review?(darin.moz)
Hey Darin, are you going to be able to review this, or is there anyone else we can request review from?
Attachment #246550 - Flags: review?(darin.moz)
Attached patch proposed patch v4 (obsolete) — — Splinter Review
Some servers can't cope with space after the commas in digest-response. I've removed them in this patch.
Attachment #246550 - Attachment is obsolete: true
Attachment #250952 - Flags: review?(darin.moz)
I'll try to review this shortly.
QA Contact: general → grylchan
Comment on attachment 250952 [details] [diff] [review] proposed patch v4 Removing Review since patch is obsolete.
Attachment #250952 - Flags: review?(darin.moz)
Updated patch for compatibility with current repository. Next try to get review.
Attachment #250952 - Attachment is obsolete: true
Attachment #266636 - Flags: review?(darin.moz)
Attachment #266636 - Flags: review?(darin.moz) → review?(cbiesinger)
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.
Product: Core → MailNews Core
cbiesinger, ping re: review ... unless Christian redirects
Status: NEW → ASSIGNED
QA Contact: grylchan → networking.imap
Wayne, we probably should ping someone else for review.
I think MoCo has a new person working on Netwerking code...maybe someone on the cc list knows who that is.
That'd be jduell.
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).
Attachment #266636 - Flags: review?(cbiesinger)
Attachment #266636 - Flags: feedback?(simon)
Attachment #266636 - Flags: feedback?(jduell.mcbugs)
Whiteboard: [patchlove]
DIGEST-MD5 now become obsoleted by rfc5802 - Salted Challenge Response Authentication Mechanism (SCRAM) be reasons mention in rfc6331
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.
Attachment #266636 - Flags: feedback?(jduell.mcbugs)
Attachment #266636 - Flags: feedback?(simon)
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
Should we rename this bug or open a new one?
(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.
Due to http://tools.ietf.org/html/rfc6331, I suggest making this WONTFIX.

From https://tools.ietf.org/html/rfc2831 besides functional advantages, I see only one security advantage mentioned:
"compared to CRAM-MD5, DIGEST-MD5 prevents chosen plaintext attacks".

From RFC 6331:
"The MD5 hash is sufficiently weak to make a brute force attack on DIGEST-MD5 easy with common hardware".

I think the conclusion is, DIGEST-MD5 doesn't achieve what it intended to achieve.

I think using a secure SSL/TLS connection between client and server should be preferred.

I connected to a few IMAP servers, and none had DIGEST-MD5 as its only authentication mechanism. I think that nowadays most servers should offer another mechanism in combination with a secure connection.

I agree with wontfix and not spend time on implementing an md5-based mechanism in 2019.
Please add new comments if you can provide strong arguments why TB really should support it.

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX

As a co-author of SASL DIGEST-MD5 (RFC 2831), I concur with both closing this as WONTFIX and RFC 6331 which moved that mechanism to IETF HISTORIC state. I also removed DIGEST-MD5 support from our server implementation some time ago.

I believe SASL SCRAM-SHA-256-PLUS and SCRAM-SHA-256 are mechanisms that could improve over passwords-over-TLS in the future, but at this point I'd say the industry consensus is that the risks of passwords-over-TLS do not yet justify the cost of investment in something better like SCRAM-SHA-256-PLUS (RFC 7677, RFC 5802/5803). I do hope that will change.

Yes, it is important to have all SCRAM-SHA-XXX-(PLUS) support!
SHA-1, SHA-2 family and SHA-3 family too.
Any news on it?

There are 2 tickets for the real ending for CRAM-MD5 and DIGEST-MD5:

Like Chris Newman (co-author of RFC 2831: SASL DIGEST-MD5) said, there is SCRAM since several years ago.

It is already done for XMPP:

SCRAM-SHA-1-PLUS and SCRAM-SHA-256-PLUS are missing because https://bugzilla.mozilla.org/show_bug.cgi?id=563276
People can look?

Tickets:

RFCs:

IANA:

Cyrus SASL supports:

Dovecot SASL supports:

GNU SASL supports:

CRAM-MD5 to Historic:

RFC6331: Moving DIGEST-MD5 to Historic

More informations:

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

Attachment

General

Creator:
Created:
Updated:
Size: