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.
taking
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.
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...
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.
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.
+#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.
Created attachment 246550 [details] [diff] [review] proposed patch v3 Same es previous patch but with changes according to comment 15.
Comment on attachment 246550 [details] [diff] [review] proposed patch v3 Trying to get review.
Hey Darin, are you going to be able to review this, or is there anyone else we can request review from?
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.
I'll try to review this shortly.
Comment on attachment 250952 [details] [diff] [review] proposed patch v4 Removing Review since patch is obsolete.
Created attachment 266636 [details] [diff] [review] proposed patch v5 Updated patch for compatibility with current repository. Next try to get review.
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.
cbiesinger, ping re: review ... unless Christian redirects
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).
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.
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.
or
