335924 - No authentication with DIGEST-MD5

Status: ASSIGNED

Description

[Joern W. Andersen]

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]

right, we don't support digest-md5 - changing to enhancement.

Comment 2

[Christian Eyrich]

taking

Comment 3

[Christian Eyrich]

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]

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]

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]

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]

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]

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]

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]

> 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]

Darin's cc'ed on the bug so I think he'd see these comments...

Comment 12

[Christian Eyrich]

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]

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]

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)]

+#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]

(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)]

another name I'd say, since it doesn't implement nsIAuthModule.

Comment 18

[Christian Eyrich]

Created attachment 246550 [details] [diff] [review]
proposed patch v3

Same es previous patch but with changes according to comment 15.

Comment 19

[Christian Eyrich]

Comment on attachment 246550 [details] [diff] [review]
proposed patch v3

Trying to get review.

Comment 20

[David :Bienvenu]

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]

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]

I'll try to review this shortly.

Comment 23

[Christian Eyrich]

Comment on attachment 250952 [details] [diff] [review]
proposed patch v4

Removing Review since patch is obsolete.

Comment 24

[Christian Eyrich]

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]

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)]

cbiesinger, ping re: review ... unless Christian redirects

Comment 27

[Nikolay Shopik]

Wayne, we probably should ping someone else for review.

Comment 28

[David :Bienvenu]

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)]

That'd be jduell.

Comment 30

[Mark Banner (:standard8)]

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]

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)]

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]

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]

Should we rename this bug or open a new one?

Comment 35

[David Lechner (:dlech)]

(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]

Due to http://tools.ietf.org/html/rfc6331, I suggest making this WONTFIX.