721369 - Dialog "Add Security Exception" does not show when autoconfiguring TB10

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Martin Schuster]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0 Iceweasel/8.0
Build ID: 20111130203257

Steps to reproduce:

Started TB10b4 on RHEL5 for the first time (i.e. no ~/.thunderbird), while having a autoconfig-file in [tb install dir]/isp/[my domain].xml

(this file points to an IMAP server that is using SSL, but the certificate is not trusted because the CA certificate has not (yet) been added to the list of trustworthy certs)

Entered my name, email-address, password (all correctly)

Pressed [Continue], configuration was taken from the aforementioned XML-file

Pressed [Create Account] (tried multiple times)


Actual results:

Error message "Configuration could not be verified -- is the username or password wrong?" appeared, and the text "Username or password invalid" next to the password-field.


Expected results:

As in TB3, the "Add Security Exception" should have appeared, and after confirming that I /do/ trust the server, I should have been able to continue creating my account.

Comment 1

[Martin Schuster]

If I press [Cancel], and then use Edit -> Account Settings... -> Add Mail Account..., and try again (i.e. enter name, address, password, press [Continue], [Create Account]), the dialog /does/ pop up.

Pressing [Confirm Security Exception] and [Create Account] than results in a working account.

If, otoh, I press [Cancel], exit TB, and start it again, it behaves as in the original report (i.e. not possible to create the account)

Comment 2

[Martin Schuster]

Errors/Warnings in the error console:

WRN: Error in parsing value for 'clip'. Declaration dropped.
  resource://qre-resources/ua.css  Line: 273
ERR: Gloda.myContact is null
  resource:///components/glautocomp.js  Line: 286
WRN: Expected color but found 'dialog'. Error in parsing value for 'background-color'. Declaration dropped.
  chrome://messenger/skin/accountCreation.css  Line: 213

but those are already there when I start entering the data, so maybe not related to the problem at hand.

Comment 3

[David :Bienvenu]

Those errors/warnings aren't relevant.

Does the same thing happen when you don't have a local autoconfig xml file, but rather use the ispdb, or probing, or manually tweaking the settings? The PSM code for cert exceptions has been changed, but I thought that change only landed for TB 11. I'm confirming, though I haven't reproduced this bug, because it's potentially serious.

Comment 4

[Ludovic Hirlimann [:Usul]]

(In reply to David :Bienvenu from comment #3)
> Those errors/warnings aren't relevant.
> 
> Does the same thing happen when you don't have a local autoconfig xml file,
> but rather use the ispdb, or probing, or manually tweaking the settings? The
> PSM code for cert exceptions has been changed, but I thought that change
> only landed for TB 11. I'm confirming, though I haven't reproduced this bug,
> because it's potentially serious.

any idea who from the psm team we should ping ? or is it in how we use psm that the problem is ?

Comment 5

[Martin Schuster]

I will give using probing and tweaking a try, but this might until Monday.

To reproduce the bug (even if your mailserver has a valid cert), I guess it would work to start TB, exit the wizard, go to the certlist and remove the one that has signed your mailserver-cert, exit TB, start it again, try to create an account using the wizard.

(I guess it's not called "wizard" anymore -- sorry :)

Comment 6

[David :Bienvenu]

(In reply to Ludovic Hirlimann [:Usul] from comment #4)
> (In reply to David :Bienvenu from comment #3)
> > Those errors/warnings aren't relevant.
> > 
> > Does the same thing happen when you don't have a local autoconfig xml file,
> > but rather use the ispdb, or probing, or manually tweaking the settings? The
> > PSM code for cert exceptions has been changed, but I thought that change
> > only landed for TB 11. I'm confirming, though I haven't reproduced this bug,
> > because it's potentially serious.
> 
> any idea who from the psm team we should ping ? or is it in how we use psm
> that the problem is ?

I haven't reproduced or diagnosed the problem, so I can't say - but bsmith is the person I would ping.

Comment 7

[Martin Schuster]

(In reply to David :Bienvenu from comment #3)
> [...]
> Does the same thing happen when you don't have a local autoconfig xml file,
> but rather use the ispdb, or probing, or manually tweaking the settings?
>
Tried without an XML-file; probing didn't give any result
("Thunderbird failed to find the settings for your email account"),
most likely because the server name is email.[sub].domain while
the email address is just user@domain.

Entering the correct server names (and setting SSL for IMAP) and pressing
[Re-test] resulted in "The following settings were found by probing the
server". Pressing [Create Account] then lead to the same problem as before,
i.e. the "Username or password invalid" message, and no dialog.

Again, doing this via Account Settings -> Add Mail Account...
/did/ lead to the dialog, and created the account successfully.

Comment 8

[David :Bienvenu]

When I do this with trunk builds, create account *does* cause the cert exception dialog to come up. But I did run into the issue that when the verifylogon fails because of the cert error, no dialog pops up, and we can't figure out the username correctly, and if the user then clicks create account, we create an account with the wrong username.  I think verifyLogon could and should bring up the cert exception dialog; what do you think, Ben?

Comment 9

[David :Bienvenu]

The alternative to making verifyLogon bring up the cert exception dialog is to have a patch where create account does some sort of logon verification...

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

As of Firefox 10 or so, PSM does not show any dialog boxes of its own. But, also, IIRC, Gecko hasn't showed the cert exception dialog box automatically since before Gecko 1.9.x. So, Thunderbird must have done something explicitly to cause the cert exception dialog box to come up--e.g. registering a nsIBadCertListener2 that triggers the exception dialog box like the "MiTM Me" addon for Firefox does. I found the code for the account creation case here:

http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/verifyConfig.js#339

I changed a lot of code in PSM regarding error handling logic in the Firefox 10 timeframe, so I wouldn't be surprised if I introduced some kind of bug regarding the calling of the bad cert listener.

Please provide a testcase (preferably an automated one, but an IMAP server with an untrusted certificate will also work), and I will figure out what the problem is.

Comment 11

[David :Bienvenu]

My mistake - verifyLogon does try to pass in a bad cert listener and put up the exception dialog, and that must not be working anymore. http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/verifyConfig.js#314

I'm not sure where it's failing - I'll have to add some tracing.

Comment 12

[David :Bienvenu]

this is working for me on the trunk - verifyLogon is causing the cert exception dialog to come up.

Comment 13

[David :Bienvenu]

sorry, verifyConfig, not verifyLogon, is bringing up the cert exception dialog. This is with probing; So this is all working as expected on the trunk. Unless someone can reproduce Martin's experiences with a trunk build, I don't think I can make progress on this.

Comment 14

[David :Bienvenu]

Ludo, any chance you could try this?

Comment 15

[Magnus Melin [:mkmelin]]

I'm not sure it's the same bug but i can't set up a mail account for imap.hut.fi  (certificate expired as server is being phased out, but old setups still working). Just try any @hut.fi address, it should use SSL/TLS.

Comment 16

[Ben Bucksch (:BenB)]

Magnus, imap.hut.fi port 993 and 143 doesn't react at all, I tried with openssl s_client and netcat. No connection refused, no response, nothing. So, not a bug.

Comment 17

[Magnus Melin [:mkmelin]]

Ah, sorry, seems it has indeed been taken permanently offline recently.

Comment 18

[Ludovic Hirlimann [:Usul]]

(In reply to David :Bienvenu from comment #14)
> Ludo, any chance you could try this?

Yeah once we get the momo server ups and I find the docs again, I will. I've pinged gozer on this last friday.

Comment 19

[Ludovic Hirlimann [:Usul]]

I'm confirming and this is how I tested.

Created an entry in /etc/host (windows\system32\drivers\etc\host) :
74.125.79.16   imap.toto.local


Setup a new account in daily to a gmail account
switched imap.googlemail.com to imap.toto.local created the account (had to press a bit more than once for the accont to be created).

I never saw a missmatch for the google cert.

Comment 20

[Ben Bucksch (:BenB)]

Confirmed comment 19. The account is created and used without ever showing a hint that the SSL certificate doesn't match the hostname, which means all SSL protections are gone.

Comment 21

[Ben Bucksch (:BenB)]

Also, the this means that the IMAP code that's run when checking mails (regular usage, not setup) doesn't notice anything about the SSL cert mismatch either, which is yet another code place. It also means that all SSL protections are void in Thunderbird. :-(

(In reply to Brian Smith (:bsmith) from comment #10)
> Please provide a testcase (preferably an automated one, but an IMAP server
> with an untrusted certificate will also work), and I will figure out what
> the problem is.

Testcase in comment 19.

Code for setup is:
http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/verifyConfig.js#292
http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/guessConfig.js#1090
http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/MyBadCertHandler.js

I notice that verifyConfig.js doesn't use MyBadCertHandler (I think it should) and also doesn't implement nsISSLErrorListener, only nsIBadCertListener2. I added a nsISSLErrorListener implementation, but no improvement.

> I changed a lot of code in PSM regarding error handling logic in the Firefox 10 timeframe,
> so I wouldn't be surprised if I introduced some kind of bug regarding the calling of the
> bad cert listener.

As I don't know what changed about the bad cert listener APIs, I don't really know where to look. Brian, could you take a look, please?

Comment 22

[Ben Bucksch (:BenB)]

I went to the PSM certification manager, saw that I had one for maninthemiddle.local:993 which I used in my testing for this bug, deleted all exceptions (including those with Server = *, which are blocklists), and I did get the "Add security exception" dialog.

I then created a new profile, tried the same again, and I am getting "Config could not be verified - password wrong?". On the console debug log, I see
mail.wizard     ERROR   cert error
mail.wizard     INFO    Finished verifyConfig resulted in 2153394164
mail.wizard     WARN    status error config_unverifiable
but I get no dialog.

This is confusing. Now I have 3 cases:
* In my old test profile, maninthemiddle.local -> imap.mail.yahoo.com succeeds without error, and somehow adds a cert exception. I am sure that I never confirmed that maninthemiddle.local:993 as exception, in fact I never saw a warning dialog for it. I have also never used that hostname before IIRC, so it can't be a remnant of an old test.
* Old test profile, after I deleted all exceptions: maninthemiddle.local triggers the standard PSM exception dialog (not ours in the mail wizard), 
* New profile: maninthemiddle.local fails without any dialog whatsoever, just "Config could not be verified - password wrong?"

Comment 23

[Ludovic Hirlimann [:Usul]]

As said it took me a while of pressing the buttons to get the accoutn created. but I never saw any cert issues.

Comment 24

[David :Bienvenu]

I'm not sure what's going on here - I definitely get the cert dialog with my setup (after removing the exception I'd previously added for the host). It's coming from verifyConfig (it's verifyLogon that could use a cert exception handler, I think). I wonder if the bug is dependent on what the cert error is. In my case, it's the mismatched host, because my ISP didn't spring for certs for each domain.

Comment 25

[Ben Bucksch (:BenB)]

> I definitely get the cert dialog

Which one? the square one from PSM (additional dialog) or the red one from the wizard?

I can't continue here without a hint from bsmith what in the PSM APIs changed.

Comment 26

[David :Bienvenu]

I get the additional dialog. I have not seen the wizard tell me the connection was bad; I don't remember if this error used to trigger that UI or not...

Comment 27

[Ben Bucksch (:BenB)]

Given that 2 people independently didn't see any SSL cert error and are thus vulnerable to MITM, I'd like to see this fixed before we ship, and adding sg:high.

Comment 28

[Ben Bucksch (:BenB)]

(worse, I got an explicit exception in my cert store, without having approved it, so the bug cannot be rectified by a later bugfix once we release like this.)

Comment 29

[David :Bienvenu]

My recollection is that the port probing code added a *temporary* cert exception, and warned the user. Perhaps that's what broke in the API, the temporariness of the cert exception.

Comment 30

[David :Bienvenu]

I tried my case (mismatched host name) on 3.1. The behavior is similar (the cert exception dialog comes up when trying to create the account), but creating the account actually works because we figure out the username correctly. That might be a separate regression, of course, but I'll dig into it a little.

Comment 31

[Ludovic Hirlimann [:Usul]]

Just spotted that we have a permanennt exception for mail.google.com, I wonder why.

Comment 32

[Dev Web.de Firefox Addon]

ludo: http://www.tomsguide.com/us/google-gmail-ssl-digital-certificate-authority,news-12343.html

Comment 33

[David :Bienvenu]

this seems obviously wrong - http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/guessConfig.js#1009

we want the override to be temporary, I would think. The whole method seems a bit suspect, judging from the comments...

Comment 34

[Ben Bucksch (:BenB)]

Yes, that whole function looks wrong.

Comment 35

[David :Bienvenu]

but some of the suspect code is never hit because of "if (false && ...)"

Ludo verified that his steps are broken in 3.1, and I suspect always. So I'm not convinced this is a recent regression.

Comment 36

[Ben Bucksch (:BenB)]

> but some of the suspect code is never hit because of "if (false && ...)"

Yeah, that was me.

Comment 37

[Ben Bucksch (:BenB)]

Attachment: Patch v1 - based on discussion with bienvenu - checked in.

Comment 38

[Ben Bucksch (:BenB)]

Attachment: Patch, v2 - remove cert override entirely, but still allow connection to proceed - backed out

This patch tries to:
- Allow connections with cert errors to proceed, without dialog.
- Not at a cert override to the cert store
So that would allow the probing to work even in face of cert errors, but not add any cert overrides at all.

What's missing here is:
- testing it
- reordering the guess results to prefer those results that have valid certs over those that have invalid certs

Comment 39

[Ben Bucksch (:BenB)]

> What's missing here is: testing it

OK, this is scary. The patch is actually working. I tried with [e-mail] , and it finds imap.davidbienvenu.org STARTTLS and smtp.davidbienvenu.org STARTTLS, and I'm not getting any popups duing the guessConfig. The standard PSM cert override dialog does come up when I click "Done" to verfiyConfig, and I can add a permanent exception.

This is pretty much exactly what we wanted.

> What's missing here is:
> - reordering the guess results to prefer those results that have valid certs over those that have invalid certs

but we can leave that part for later.

Comment 40

[David :Bienvenu]

OK, both these patches work equally well with my simple test cases - the one in https://bugzilla.mozilla.org/show_bug.cgi?id=721369#c19, and the one with my isp, which uses a wildcard cert for all the domains it hosts.

Be aware that the cert error in #c19's steps don't allow the user to override the cert error, which is fine with me.

Obviously, the second patch is more the direction we'd like to go, but it's a bit scarier.

Comment 41

[Ben Bucksch (:BenB)]

> - reordering the guess results to prefer those results that have valid certs over
> those that have invalid certs

Concretely: We find imap.isp.com with cert for mail.isp.com and mail.isp.com with cert for mail.isp.com. Given that the first is invalid and the last is valid, we should take the last.

Comment 42

[David :Bienvenu]

Comment on attachment 615439 [details] [diff] [review]
Patch, v2 - remove cert override entirely, but still allow connection to proceed - backed out

So this works for me. Ludo, it would be great if we could get some real world testing.

There's still the unrelated issue that user name probing is broken if there are cert exceptions. I'll try to figure that out too.

Comment 43

[Ben Bucksch (:BenB)]

> There's still the unrelated issue that user name probing is broken if there are cert exceptions.
> I'll try to figure that out too.

Please file a new bug for that. We might have changed that intentionally.

Comment 44

[Ben Bucksch (:BenB)]

Comment on attachment 615439 [details] [diff] [review]
Patch, v2 - remove cert override entirely, but still allow connection to proceed - backed out

Commited as http://hg.mozilla.org/comm-central/rev/c377ad53f15c

Leaving bug open for the mentioned reorder.

Ludo, and anybody else, please test this carefully with real-world test cases. If you find something wrong, please check that it's indeed a regression caused by this patch.


Per bienvenu, requesting approval for TB 12 beta.

[Approval Request Comment]
User impact if declined:
We do not warn about cert errors, which means the user is vulnerable to MITM attacks. We removed any and all SSL protections (!) with these silent, permanent overrides for all possible hostnames.

Testing completed (on c-c, etc.): 
Testing? What testing? [e-mail] works, bienvenu's happy, so what more do we need?
I repeated the test from comment 19 / 20 and it now works as expected.

Comment 45

[Ben Bucksch (:BenB)]

bsmith says that the fix can not work, because there is no such thing as a per-socket override anymore. Yet, it works for me, but my testing could be wrong.

Comment 46

[David :Bienvenu]

I think the problem with the patch we landed is that capability detection is broken if we don't do the temporary cert exception, because in the SSL case, we can't get the auth capabilities, and in the STARTTLS case, we can't get the capabilities after the STARTTLS connection happens, and some servers change the capabilities after STARTTLS succeeds. So, I suspect we need to go with the other patch. Ben, thoughts?

Comment 47

[Ben Bucksch (:BenB)]

> capability detection is broken

You think that based on the code and bsmith's comments, or based on your testing?
Because it works for me. I can't explain why.

Comment 48

[David :Bienvenu]

(In reply to Ben Bucksch (:BenB) from comment #47)
> > capability detection is broken
> 
> You think that based on the code and bsmith's comments, or based on your
> testing?
> Because it works for me. I can't explain why.

I bet if you were to do an imap protocol log, you wouldn't see us doing a CAPABILITY command on an SSL connection, or one after STARTTLS, in the cert error case. It probably works for you because PLAIN login is OK for most secure connections.

Comment 49

[Ben Bucksch (:BenB)]

The imap protocol log using NSPR only works in mailnews/imap/, right? guessConfig doesn't use that, it runs raw sockets and implements the tiny part of IMAP/POP3/SMTP itself. Thus, I don't think I'd see anything on the log in any case. I've been looking at the "wiredata" that you get when you set "mail.wizard" logging to "All", and it did contain the protocol chatter even for secure connections, as far as I could read the log based on the code.

Comment 50

[David :Bienvenu]

(In reply to Ben Bucksch (:BenB) from comment #49)
> The imap protocol log using NSPR only works in mailnews/imap/, right?
> guessConfig doesn't use that, it runs raw sockets and implements the tiny
> part of IMAP/POP3/SMTP itself. Thus, I don't think I'd see anything on the
> log in any case. I've been looking at the "wiredata" that you get when you
> set "mail.wizard" logging to "All", and it did contain the protocol chatter
> even for secure connections, as far as I could read the log based on the
> code.

Oh, you're right about the protocol logging. Ok, I can try to look at the mail.wizard.logging for the error case. But I suspect what Brian described might be the reason username guessing isn't working anymore.

Comment 51

[Ben Bucksch (:BenB)]

what I see is, with my patch v2, when I search for [e-mail]:
1) I find IMAP and SMTP servers with STARTTLS and Normal password. I see in the log that we actually do contact and talk with the server over STARTTLS. I see various "bad cert" warnings in the log, too.
2) when I remove STARTTLS from getIncomingTryOrder(), then I get only non-SSL connections as result. I still get "bad cert" warnings on the log
3) when I remove SSL (but leave STARTTLS) from getIncomingTryOrder(), then I get STARTTLS connections as result. I do *not* get "bad cert" warnings on the log

That means:
1) For normal SSL, the connection doesn't work at all (and no dialog) in case of cert errors.
2) For STARTTLS, we don't get any cert error, and the connection works, despite cert hostname mismatch.

This is pretty nasty, and we should investigate why the cert verifications fail for STARTTLS in guessConfig.js, which implements a minimal IMAP itself based on sockets. However, the checks do work in verifyConfig, which uses mailnews/imap/.

So: As long as we fix this bug, the above isn't a real problem, because verifyConfig will catch it at the right time. We just must make sure that guessConfig removes all cert overrides after it finishes, in all cases, so that verifyConfig does not find an override and shows the dialog.

So, I recomment to revert my patch v2 (at least for now) and test and apply patch v1.

Comment 52

[David :Bienvenu]

Comment on attachment 615429 [details] [diff] [review]
Patch v1 - based on discussion with bienvenu - checked in.

I tried this yesterday, and it worked for me.

Comment 53

[Ben Bucksch (:BenB)]

Backout of patch v2:
http://hg.mozilla.org/comm-central/rev/7e7a98d27279
http://hg.mozilla.org/comm-central/rev/0cf9322009b8

Commit of patch v1: (Based on bienvenu's tests)
http://hg.mozilla.org/comm-central/rev/69177e1f55f4

Comment 54

[David :Bienvenu]

Comment on attachment 615429 [details] [diff] [review]
Patch v1 - based on discussion with bienvenu - checked in.

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 55

[Ben Bucksch (:BenB)]

Comment on attachment 615429 [details] [diff] [review]
Patch v1 - based on discussion with bienvenu - checked in.

bienvenu tested this for me

Comment 56

[Andrew Sutherland [:asuth] (he/him)]

looking at guessConfig.js, I don't actually see anyplace where nsISSLSocketControl's StartTLS method actually gets called.  Nor elsewhere in accountcreation/.  Are you sure the code performs a TLS upgrade?

Comment 57

[Ben Bucksch (:BenB)]

I didn't write the code. But you're probably right, that this is simply missing.
I remember using that function when implementing XMPP.
We should open another bug for this.

Comment 58

[Mark Banner (:standard8)]

Comment on attachment 615429 [details] [diff] [review]
Patch v1 - based on discussion with bienvenu - checked in.

a=Standard8 for aurora/beta. This is a fix we need that has been verified.

Comment 59

[Mark Banner (:standard8)]

Checked into branches:

http://hg.mozilla.org/releases/comm-aurora/rev/ec47a32e259b
http://hg.mozilla.org/releases/comm-beta/rev/6e9b0c195e3e

Comment 60

[Mark Banner (:standard8)]

Comment on attachment 615429 [details] [diff] [review]
Patch v1 - based on discussion with bienvenu - checked in.

[Triage Comment]
Taking for ESR as well as this affects that branch also.

Comment 61

[Mark Banner (:standard8)]

Landed on esr:

http://hg.mozilla.org/releases/comm-esr10/rev/600d7637a548