231303 - Unable to use use IMAP on in.virgilio.it because of broken CRAM-MD5 on server

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Christian Eyrich]

This is a regression to bug 225809.
In the patch to this, the former behaviour to only use secure authentication
mechanisms (like CRAM-MD5) was changed.

The CRAM-MD5 support of Virgilio (big italian provider) is broken and fallback
is impossible for us because the server disconnects after failure.
So I vote for only using sec_auth when specified by the user as it is in POP for
the same reason (not Virgilio, but the problem is quite widespread).

Comment 1

[David :Bienvenu]

Christian, are you saying we'll try cram-md5 even if "use secure auth" is turned
off for the imap server? Are you saying that we should not try cram-md5 if use
secure auth is not checked?

Comment 2

[Christian Eyrich]

Yes, that is what I'm saying.

In nsImapProtocol.cpp version 1.530
  if (m_useSecAuth && flag & kHasCRAMCapability)
has been replace by
  if (flag & kHasCRAMCapability)

That's the problem.

Comment 3

[David :Bienvenu]

OK, I'll back out that part of the patch. I thought I'd attached a patch to this
bug, but I don't see it...

Comment 4

[David :Bienvenu]

Attachment: proposed fix

Comment 5

[Lorenzo Colitti]

*** Bug 227560 has been marked as a duplicate of this bug. ***

Comment 6

[Lorenzo Colitti]

Attachment: Alternative patch

Another possible patch. With this one, all the decisions are made in in
TryToLogon() and AuthLogin() just does what it's told.

Comment 7

[David :Bienvenu]

With your patch, I think we won't try auth plain or auth login either. That may
be desirable, but I'd like to try it my way first. If it turns out that using
Auth plain or auth login also has problems, we can go with your patch.

Comment 8

[(not reading, please use [e-mail] instead)]

fix checked in.

Comment 9

[Christian Eyrich]

David, it looks it's the other way round.
With your/my patch PLAIN and LOGIN will not be tried if !useSecAuth and the
server offers CRAM-MD5. That's because AuthLogin() is called with
kHasCRAMCapability, but this if fails and so the InsecureLogin() on the bottom
will be used.

Comment 10

[Lorenzo Colitti]

I looked at the code again and it seems that my patch does actually fall back to
auth plain and auth login. This is what I have in my tree:

        // Use CRAM only if secure auth is enabled. This is for servers that
        // say they support CRAM but are so badly broken that trying it causes
        // all subsequent login attempts to fail (bug 231303, bug 227560)
        if (m_useSecAuth && GetServerStateParser().GetCapabilityFlag() &
kHasCRAMCapability)
        {
          AuthLogin (userName, password, kHasCRAMCapability);
          logonTries++;
        }
        else 
        if (GetServerStateParser().GetCapabilityFlag() & kHasAuthPlainCapability)

I'm not sure, but I think the other patch actually falls back to insecure login
without trying plain and/or auth login. If this is the case, then I think my
patch is more appropriate.

Reopening the bug so that folks here can consider the issue.

Comment 11

[David :Bienvenu]

Lorenzo, the server in question drops the connection on an attempted cram-md5
logon, so the fallback code is not used. That's why we have to allow the
disabling of cram-md5

Comment 12

[David :Bienvenu]

Christian, I think I'll use your patch - you're right about the fall-back
behaviour...

Comment 13

[Lorenzo Colitti]

David: my patch does allow the disabling of CRAM! It just does so in a different
way.

The problem I was referring to is if m_useSecAuth is false and the server says
it supports CRAM. In this case, the patch which was checked in will not attempt
AUTH PLAIN or AUTH LOGIN and will immediately fall back to InsecureLogin(). This
is what happens:

TryToLogon() does:

7052: if (GetServerStateParser().GetCapabilityFlag() & kHasCRAMCapability)
7053: {
7054:   AuthLogin (userName, password, kHasCRAMCapability);

This is true, since the server says it supports CRAM, so AuthLogin is called.
AuthLogin() does:

4742: if (m_useSecAuth && flag & kHasCRAMCapability)

This is false, since m_useSecAuth is false. So we skip to:

4783:   } // if CRAM response was received
4784:   else 
4785:   if (flag & kHasAuthPlainCapability)

*This is also false*, since flag == kHasCRAMCapability! So we skip to:

4818:   } // if auth plain capability
4819: 
4820:   else if (flag & kHasAuthLoginCapability)

False again. So we skip to:

4857:   if (!m_useSecAuth)
4858:     InsecureLogin(userName, password);

So PLAIN and LOGIN are not tried. I believe this is what Christian was referring
to in comment #9.

If this is what we want, then sorry for making such a fuss! But it seemed to me
that in comment #7 you wanted to try PLAN and LOGIN even if m_useSecAuth is off.
I think my patch should handle that case properly, while also disabling CRAM if
m_useSecAuth == false.

Not reopening. :)
Someone else please reopen if they agree with me though...

Comment 14

[David :Bienvenu]

Lorenzo, right, that's why I said I was going to take Christian's patch, which I
believe does what I want, and what you describe.

I don't think your patch allowed the user to disable even attempting cram-md5 if
the server said it supported it. It would try cram-md5 even if m_useSecAuth was
false, and that's the behaviour we need to remove, because of those poorly
behaved servers that drop the connection when cram-md5 is tried.

Comment 15

[Christian Eyrich]

Er, David, what patch do you refer to as Lorenzo's patch and what as mine?
I think Lorenzo refers to the "Alternative patch" attached here, not the
original from the prior bug.

"Mine" is actually the one that's not checked in. That is because I just looked
at the old patch to re-establish the old state. But now I think moving the check
for m_useSecAuth should better be moved to TryToLogon().

I hope we don't talk at cross-purposes.

Comment 16

[David :Bienvenu]

I thought Lorenzo was referring to his initial patch. I know your patch is not
checked in. I'm going to check it in soon, tonight, I hope.

Comment 17

[Lorenzo Colitti]

Comment on attachment 139337 [details] [diff] [review]
Alternative patch

Oh, sorry. I thought attachment 139316 [details] [diff] [review] (the first patch in this bug, which
disables PLAIN and LOGIN) was already checked in (as per Seth's comment #8).

Just clarify things: I know my original code from bug 225809 did not allow
disabling CRAM. But I think attachment 139337 [details] [diff] [review] ("Alternative patch") fixes both
issues. Are we in agreement here? I'm confused. :)

Requesting reviews just to make sure.

Comment 18

[Christian Eyrich]

Ok, file revision 1.537 now shows the Lorenzo's "Alternative patch" (attachment
139337 [details] [diff] [review]) beeing checked in and attachment 139316 [details] [diff] [review] is out.

So everything looks fine now. The last bit of confusion might has been caused by
a sentence from my comment #15
> "Mine" is actually the one that's not checked in.
where the "not" is wrong. My only excuse is the late hour I wrote that comment.

Comment 19

[Jo Hermans]

*** Bug 231596 has been marked as a duplicate of this bug. ***

Comment 20

[David :Bienvenu]

fixed on m4 branch.

Comment 21

[David :Bienvenu]

Comment on attachment 139337 [details] [diff] [review]
Alternative patch

clearing request