No authentication with DIGEST-MD5

RESOLVED WONTFIX

Status

enhancement
RESOLVED WONTFIX
13 years ago
3 months ago

People

(Reporter: jwa, Assigned: ch.ey)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchlove])

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

13 years ago
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

13 years ago
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

Updated

13 years ago
Component: General → Networking: IMAP
Product: Thunderbird → Core
Version: unspecified → Trunk
Assignee

Comment 2

13 years ago
taking
Assignee: bienvenu → ch.ey
Assignee

Comment 3

13 years ago
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

13 years ago
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

13 years ago
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

13 years ago
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?
Assignee

Comment 7

13 years ago
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.
Assignee

Comment 8

13 years ago
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

13 years ago
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.
Assignee

Comment 10

13 years ago
> 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

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

Comment 12

13 years ago
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

13 years ago
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.
Assignee

Comment 14

13 years ago
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.
Assignee

Updated

13 years ago
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
Assignee

Comment 16

13 years ago
(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.
Assignee

Comment 18

13 years ago
Posted patch proposed patch v3 (obsolete) — Splinter Review
Same es previous patch but with changes according to comment 15.
Attachment #241935 - Attachment is obsolete: true
Assignee

Comment 19

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

Trying to get review.
Attachment #246550 - Flags: review?(darin.moz)

Comment 20

13 years ago
Hey Darin, are you going to be able to review this, or is there anyone else we can request review from?
Assignee

Updated

13 years ago
Attachment #246550 - Flags: review?(darin.moz)
Assignee

Comment 21

13 years ago
Posted 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)

Comment 22

13 years ago
I'll try to review this shortly.
QA Contact: general → grylchan
Assignee

Comment 23

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

Removing Review since patch is obsolete.
Attachment #250952 - Flags: review?(darin.moz)
Assignee

Comment 24

12 years ago
Updated patch for compatibility with current repository. Next try to get review.
Attachment #250952 - Attachment is obsolete: true
Attachment #266636 - Flags: review?(darin.moz)
Assignee

Updated

12 years ago
Attachment #266636 - Flags: review?(darin.moz) → review?(cbiesinger)

Comment 25

12 years ago
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

Comment 27

10 years ago
Wayne, we probably should ping someone else for review.

Comment 28

10 years ago
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]

Comment 31

8 years ago
DIGEST-MD5 now become obsoleted by rfc5802 - Salted Challenge Response Authentication Mechanism (SCRAM) be reasons mention in rfc6331

Comment 32

8 years ago
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.

Updated

8 years ago
Attachment #266636 - Flags: feedback?(jduell.mcbugs)
Attachment #266636 - Flags: feedback?(simon)

Comment 33

7 years ago
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

7 years ago
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.

Comment 36

5 years ago
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
Last Resolved: 3 months ago
Resolution: --- → WONTFIX

Comment 38

3 months ago

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.

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