202162 - Auth. mechanism fallback (for ever) isn't always good

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[Christian Eyrich]

Fix of bug 201724 added a fallback to the next weaker mechanism if one fails
(e.g CRAM-MD5 fails -> switching to PLAIN).

The problem now is, that e.g. CRAM-MD5 will be completely switched off for this
sending process. So if the user gives the wrong password, we switch to PLAIN,
its still wrong and we fail again, the user enters the correct password but we
send it PLAIN and don't even try with CRAM-MD5 again.

It get's even harder if CRAM-MD5 ist the only mechanism advertised by the
server. You now have only one try to insert the the right password since the
only mechanism on the list has been switched off.
So we either should only deactivate it if it's not the last/only mechanism in
the list. Or add a test if originally mechanisms where activated (check again a
flagbackup that has to be created) to the TestFlag ifs in ProcessAuth() (need to
set m_nextState to SMTP_AUTH_PROCESS_STATE instead of
SMTP_SEND_AUTH_LOGIN_USERNAME of course) and reenable the original mechanisms there.

In the later case we wouldn't need to do this in AuthLoginResponse() - case 5
right after ForgetPassword and deleting the Username - too.

BTW, I'm talking about the code in nsSmtpProtocol.cpp.

Comment 1

[David :Bienvenu]

I think we need to re-enable the cram-md5 auth when we re-prompt for the
password, which I think is the last of the choices you describe.

Comment 2

[Christian Eyrich]

We need to re-enable the CRAM-MD5, but only if it has been activated before in
SendEhloResponse(). So we need a backup of at least this flag to
SetFlag(SMTP_AUTH_CRAM_MD5_ENABLED) it.

But I'd prefer a backup of all the flags to reenable them at once by copying the
var holding the backuped state to the actual var. Although it would require two
new functions like

BackupFlags(){
m_backupFlags = m_flags;
}

and

RestoreFlags(){
m_flags = m_backupFlags;
}

and one new m_backupFlags or so.

At the latest when we introduce another mechanism like DIGEST-MD5 this will be
better and clear.

And we must ensure to clear pw and username, even it was CRAM-MD5 (or other
mechs to be added) that failed! But general spoken, only if there is no mech we
can/want fall back to.

Comment 3

[Christian Eyrich]

Attachment: suggestion

This patch passed my few tests I applied.
But nevertheless I only see it as a suggestion, especially the changes to the
nsSmtpProtocol class.

But it's to clearify what I think what is the right direction.

Comment 4

[David :Bienvenu]

thx, that patch looks pretty reasonable to me - I'll try to review it more
closely tonight.

Comment 5

[David :Bienvenu]

I tried this patch on a few test cases and it worked fine. Patch checked in,
thx, Christian.

Comment 6

[David :Bienvenu]

Comment on attachment 120710 [details] [diff] [review]
suggestion

r/sr=bienvenu

Comment 7

[Christian Eyrich]

Oh, wow, thanks David.

But, looking over the patch right now I recognized a bug:
void nsSmtpProtocol::RestoreFlags()
{
  m_flags |= m_backupFlags&SMTP_AUTH_CRAM_MD5_ENABLED;
}

restores only the CRAM_MD5-Flag!

It has to be
  m_flags |= m_backupFlags&SMTP_AUTH_ANY_ENABLED;
of course.

Actually we need the &SMTP_AUTH_ANY_ENABLED not at all because m_backupFlags
ever contains only the bits of SMTP_AUTH_ANY_ENABLED.

And on the other hand we can be sure m_flags has no bit of SMTP_AUTH_ANY_ENABLED
 set when RestoreFlags called. But maybe we should to be clean and do:
m_flags &= !SMTP_AUTH_ANY_ENABLED;
m_flags |= m_backupFlags&SMTP_AUTH_ANY_ENABLED;

In any case, sorry for the mistake, please fix it with any of the above
alternatives of your choice.

Comment 8

[David :Bienvenu]

yeah, I should have said something - when I looked at the code, I did expect to
see something more like this:

m_flags &= !SMTP_AUTH_ANY_ENABLED;
m_flags |= m_backupFlags&SMTP_AUTH_ANY_ENABLED;

But, i think the way to go is to change the var name to m_origAuthFlags, so it's
clear what we're storing there, and do the following

in backup
m_origAuthFlags = m_flags & SMTP_AUTH_ANY_ENABLED;

in restore
m_flags |= m_origAuthFlags;

Comment 9

[Christian Eyrich]

Ok, that's a way I can go too.
Do you do this fix, or shall I create a patch for formally reasons?

Comment 10

[David :Bienvenu]

I'll do it, and just attach a diff before I check in. Thx again for all the help.

Hey, do you want to add this support to POP3? The POP3 state machine is similar
to the SMTP state machine... I can point you at the bug if you're interested.

Comment 11

[Christian Eyrich]

Oh yes, I'll look at it and try. Just point me to the bug.

Comment 12

[David :Bienvenu]

darn, I can't find the bug, so I'm going to have to open a new one. bug 202442

Comment 13

[David :Bienvenu]

Attachment: clean up previous fix

I'll check this in.

Comment 14

[David :Bienvenu]

fix checked in.