Closed Bug 225809 Opened 21 years ago Closed 21 years ago

Don't fall back to insecure authentication if "use secure authentication" is checked

Categories

(MailNews Core :: Networking: IMAP, defect)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: lorenzo, Assigned: lorenzo)

References

Details

Attachments

(1 file, 3 obsolete files)

patch addressing mscott's comments
3.74 KB, patch
lorenzo
: review+
lorenzo
: superreview+
asa
: approval1.6b+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031115 Firebird/0.7+ ((01) - Mike Tigas (Nova); MNG,DOMi,Venkman; G6;)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031115 Firebird/0.7+ ((01) - Mike Tigas (Nova); MNG,DOMi,Venkman; G6;)

If "use secure authentication" is turned on in the account settings, mozilla
tries to login using CRAM-MD5, and if it fails it falls back to an insecure
login, sending the plaintext password.

This can happen, for example, if mozilla exceeds the maximum number of
connections permitted my the server, if it tries to connect when the mailbox is
locked or the server is having problems, or if the user mistypes the password.
In all these cases retrying with plaintext authentication will not help and
using it defeats the purpose of secure authentication, which is to avoid sending
the password over the wire.

Since we have UI to turn secure authentication on and off, and secure auth is
off ny default, I propose that the secure authentication checkbox change meaning
from "try to use secure auth and fall back to plain text if it fails" to "use
secure auth only". The UI could be changed to read "Require secure
authentication" or "Use secure authentication only".

I can do this if people agree that it's a good idea.

Reproducible: Always

Steps to Reproduce:
1. Turn on secure authentication for a mailnews account
2. Mistype one character in the password

Actual Results:  
Mozilla retries using plaintext authentication, which of course fails. An
attacker armed with a sniffer can probably figure out your password by removing
your typo.

Expected Results:  
Mozilla should not retry. You have told it to use secure auth only, and that is
what it should use.
Assigning to myself.
mscott, if I write the code, will you review this?
Status: NEW → ASSIGNED
Lorenzo, are you talking about IMAP or POP3 or both?
So far I've only looked at the code for IMAP, since that's what I use, and it
looks like a simple fix could be trivial. But perhaps we should change the
behaviour for POP as well (especially if we change the UI), and I haven't looked
at that code yet.
Er, POP3 code already only tries secure mechanisms if "use secure
authentication" is on and vice versa. See the if on
http://lxr.mozilla.org/seamonkey/source/mailnews/local/src/nsPop3Protocol.cpp#1446.
Sorry, I was just reading the code now. I was looking at POP3_AUTH_FALLBACK and
thought that the code would fall back to plaintext auth in case CRAM-MD5 failed.
So you're sure that POP doesn't fall back to plaintext auth in case secure login
fails?

If that is so, I guess that means the UI is already wrong for POP3, and that
IMAP needs to be fixed so it doesn't fall back either. :-)
No, it really shouldn't fall back to plaintext if secure is on. At least that's
what I coded and what my tests showed.

Why is the UI wrong? "Use secure authentication" uses secure authentication. Ok,
adding an only would be possible. But users can interpret this as Mozilla uses
secure and unsecure if unchecked - so that only unsecure is toggled.
Really taking.
Assignee: sspitzer → lorenzo
Status: ASSIGNED → NEW
Component: Security: General → Networking: IMAP
Attached patch patch v0 (obsolete) — — Splinter Review 
Patch that disables fallback to insecure auth for IMAP.

If "use secure authentication" is checked and the server does not support
CRAM-MD5, an alert appears, the user is not prompted for a password, and the
login does not proceed.

Note that if the server claims to support CRAM-MD5, we will try to use it
regardless of the "use secure authentication" setting. This is different from
what the POP code does, but it required the least changes, and I think it's
also a better choice, since we already do fallback to insecure login.

bienvenu, can you look at the code and see if you like it?
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — — Splinter Review 
This patch is better: we don't fall back to insecure login if the user mistypes
the password. Also, it tries CRAM-MD5 first if the server supports it, even if
"use secure authentication" is off. If it fails, it falls back to plaintext
auth.
Attachment #135671 - Attachment is obsolete: true
So, to summarize:

If "use secure authentication" is off:
 - If server supports CRAM, moz tries CRAM and falls back to plaintext on failure
 - If server doesn't support CRAM, moz uses plaintext

If "use secure authentication" is on:
 - If server supports CRAM: moz tries CRAM only (no fallback)
 - If server doesn't support CRAM: moz warns the user and does not login
Comment on attachment 135676 [details] [diff] [review]
patch v1

bienvenu, can you look at this? It seems reasonable to me, and I haven't seen
any ill effects yet.
Attachment #135676 - Flags: review?(bienvenu)
Comment on attachment 135676 [details] [diff] [review]
patch v1

this looks  OK, thx
Attachment #135676 - Flags: review?(bienvenu) → review+
Comment on attachment 135676 [details] [diff] [review]
patch v1

Requesting sr.

mscott, if you like it, can you check it in? I don't have CVS access.
Attachment #135676 - Flags: superreview?(mscott)
This is the same as patch v1, except that the error message that appears if you
configure secure auth and then log in to a server that doesn't support it is
(hopefully) more user friendly.
If the server supports secure authentication but the login fails, wouldn't that
mean that the password was wrong (or, if there was some other problem such as
those described in the report, it presumably wouldn't work any better with a
plain-text password), so Mozilla shouldn't fall back to plain text?

Re comment #15, plaintext auth is needed because some servers say they support
secure auth but don't implement it properly. On these servers, secure auth fails
but plaintext auth using the same password succeeds.
Comment on attachment 135915 [details] [diff] [review]
same patch with revised error message

I'd suggest shortening the alert message to:

You cannot log in to %S because you have enabled secure authentication and this
server does not support it.\n\nTo log in, turn off secure authentication for
this account.

carrying forward the review flags.
Attachment #135915 - Flags: superreview+
Attachment #135915 - Flags: review+
Patch with revised error message as per mscott's comments.
Attachment #135676 - Attachment is obsolete: true
Attachment #135915 - Attachment is obsolete: true
Attachment #135676 - Flags: superreview?(mscott)
Comment on attachment 136279 [details] [diff] [review]
patch addressing mscott's comments

Carrying forward r= and sr=
Attachment #136279 - Flags: superreview+
Attachment #136279 - Flags: review+
Comment on attachment 136279 [details] [diff] [review]
patch addressing mscott's comments

Nominating for 1.6b. It's a very simple patch with no heavy lifting (just code
refactoring) so it should be low risk.

It also improves security, what more can you want? :-)
Attachment #136279 - Flags: approval1.6b?
Comment on attachment 136279 [details] [diff] [review]
patch addressing mscott's comments

a=asa (on behalf of drivers) for checkin to 1.6b.
Attachment #136279 - Flags: approval1.6b? → approval1.6b+
Look like this is ready to go. Can someone check it in?
checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Lorenzo, it looks like this patch is causing problems for users. 

See: 

http://forums.mozillazine.org/viewtopic.php?p=278225#278225

and
 
http://forums.mozillazine.org/viewtopic.php?p=278257#278257

seems like without some user intervention, some users are no longer able to
check mail.
mscott: it looks like the problem is due to users having checked "use secure
authentication" checkbox when their server does not support it. Before, they
were falling back to insecure auth, but now they can't. Unfortunately, this is
precisely the issue that this patch is designed to solve.

If I remember correctly, secure auth was never on by default so the user must
have turned it explicitly for this to happen. So we're talking about power users
here.

I see three possibilities (plus of course 4. back it out or 5. do nothing):

1. Document this in the release notes so the power users, who are the only ones
likely to have turned this on, are warned about the possible problem.
2. Provide an easy way to turn off secure auth from the dialog box that appears.
This would require more code, of course.
3. Deprecate the old pref and create a new one which defaults to false.

It would probably also help to change the label of the checkbox to "require
secure authentication" or "use secure authentication only" since that is what we
are really doing both for POP and for IMAP.

Personally I would prefer #1, but #2 might also make sense. I don't like #3 at
all. Do you think it would be enough to put it into the release notes?
Ok, we can try to focus on (1) in our release notes. 
*** Bug 263000 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: