10.09 KB, patch
|Details | Diff | Splinter Review|
9.61 KB, patch
|Details | Diff | Splinter Review|
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.
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)
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.
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.
(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 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 :)
(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.
(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.
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?
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...
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.
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.
this is working for me on the trunk - verifyLogon is causing the cert exception dialog to come up.
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.
Ludo, any chance you could try this?
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.
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.
Ah, sorry, seems it has indeed been taken permanently offline recently.
(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.
I'm confirming and this is how I tested. Created an entry in /etc/host (windows\system32\drivers\etc\host) : 18.104.22.168 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.
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.
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?
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?"
As said it took me a while of pressing the buttons to get the accoutn created. but I never saw any cert issues.
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.
> 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.
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...
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.
(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.)
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.
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.
Just spotted that we have a permanennt exception for mail.google.com, I wonder why.
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...
Yes, that whole function looks wrong.
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.
> but some of the suspect code is never hit because of "if (false && ...)" Yeah, that was me.
Created attachment 615429 [details] [diff] [review] Patch v1 - based on discussion with bienvenu - checked in.
Created attachment 615439 [details] [diff] [review] 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
> What's missing here is: testing it OK, this is scary. The patch is actually working. I tried with email@example.com , 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.
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.
> - 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 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.
> 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 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? firstname.lastname@example.org works, bienvenu's happy, so what more do we need? I repeated the test from comment 19 / 20 and it now works as expected.
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.
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?
> 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.
(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.
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.
(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.
what I see is, with my patch v2, when I search for email@example.com: 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 on attachment 615429 [details] [diff] [review] Patch v1 - based on discussion with bienvenu - checked in. I tried this yesterday, and it worked for me.
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 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 on attachment 615429 [details] [diff] [review] Patch v1 - based on discussion with bienvenu - checked in. bienvenu tested this for me
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?
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 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.
Checked into branches: http://hg.mozilla.org/releases/comm-aurora/rev/ec47a32e259b http://hg.mozilla.org/releases/comm-beta/rev/6e9b0c195e3e
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.
Landed on esr: http://hg.mozilla.org/releases/comm-esr10/rev/600d7637a548