739563 - No error message for SSL/TLS protocol errors and non-overridable cert errors

Status: NEW

(MailNews Core :: Networking, defect, P3)

Description

[Mark Banner (:standard8)]

+++ This bug was initially created as a clone of Bug #739558 +++

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1081.0 Safari/536.5 SUSE/19.0.1081.0

Per bug 739558, md5 signatures have now been disabled in core (bug 650355).

However, I spoke to cartman who filed the bug and he said he didn't get any prompt about an invalid certificate when security.enable_md5_signatures was set to false.

I think we should have some kind of prompt there.

Comment 1

[Kai Engert (:KaiE:)]

(In reply to Mark Banner (:standard8) from comment #0)
> 
> I think we should have some kind of prompt there.


We DID have prompts for that!

Say thanks to bsmith who removed that helpful prompt by default for any Mozilla application, as a simple solution for bug 682329...

Comment 2

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

Regarding the MD5 issue specifically: we need to build the telemetry and add better logging to help diagnose this problem, so that we don't waste time. I suspect, but haven't verified, that quite a few Thunderbird users are likely to be affected by the disabling of the MD5 algorithm in certificate signatures in Gecko. Note that Outlook and other programs that use the Windows APIs, and probably programs that are based on OpenSSL, will still trust the MD5-based certificates. AFAICT, only Gecko (Thunderbird, Firefox, and Google Chrome) have chosen to break compatibility here. Please let me know if you would like to discuss how to deal with this. We have more flexibility here--e.g. reverting the change in Gecko, or in Thunderbird, or putting the change off a little more--than there was with the BEAST TLS record splitting.

Regarding the general issue of the modal dialog box:

The modal dialog box is more harmful than helpful to Firefox. Also, the Necko team is planning to remove *all* prompts that occur as part of the internal operations of the Necko APIs, for various reasons.

We already have a bug open for creating a UI for handling SSL-related errors as a result of web page resource loads in Firefox. It might involve an infobar or something else. I am *sure* the UX team is not going to use a modal dialog box for that case.

The UI for Thunderbird will likely have to be different. I am willing to help someone on your team implement a good UI for this. Please let me know who to work with and what priority you think this bug should be given, relative to other things that you would like me to do to help the Thunderbird team.

Comment 3

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

(In reply to Brian Smith (:bsmith) from comment #2)
> The UI for Thunderbird will likely have to be different. I am willing to
> help someone on your team implement a good UI for this. Please let me know
> who to work with and what priority you think this bug should be given,
> relative to other things that you would like me to do to help the
> Thunderbird team.

By the way, IIRC, Thunderbird already has a UI for showing connection errors, right?. I suspect the problem is that the Gecko networking APIs are not returning a useful error code to the application (Thunderbird) for it to show a prompt. Please point out where in Thunderbird's code the UI for networking errors lives, and I will take a look at getting this hooked up to it.

Comment 4

[Ben Bucksch (:BenB)]

> so that we don't waste time.

The time wasted by users is 1000 times larger, so that's not an argument.

> I will take a look at getting this hooked up to it

Thanks. There's no central networking code in TB (apart from netwerk in Gecko), it's in each protocol's implementation.
http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Protocol.cpp
http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsSmtpProtocol.cpp

> The modal dialog box is more harmful than helpful to Firefox.

Even a suboptimal error UI is still better than silently breaking with no indication as to cause. If you're saying that this is not a common case, then the error dialog wouldn't come up often and doesn't pose a problem. If it's a common case, than the silent failure wastes a lot of time of users. Either way, in the concrete given situation, I am better off with a modal dialog (as bad as it may be) than with no error indication.

Comment 5

[Kai Engert (:KaiE:)]

Can you please test patch v4 in bug 682329?

Comment 6

[Kai Engert (:KaiE:)]

> Regarding the MD5 issue specifically: we need to build the telemetry and add
> better logging to help diagnose this problem, 

I disagree.

I'm not intestered in political arguments saying "these 5% of our users are irrelevant". 

I'm not interested in users turning away from Mozilla because it doesn't work and doesn't say why.

I'm interested in software that works, or tells our users why it choses not to work.


> quite a few Thunderbird users are likely
> to be affected by the disabling of the MD5 algorithm in certificate
> signatures in Gecko.


Sure. That's the whole idea of having introduced the pref to get it reenabled while those users evangelize their providers and go through the transition phase. Nevertheless it's the right thing to do. But our users must be made aware of the issue, not face software silence.


> Note that Outlook and other programs that use the
> Windows APIs, and probably programs that are based on OpenSSL, will still
> trust the MD5-based certificates.

This is why it's even more important to make our users aware of our reasonable decision to reject MD5 signatures, making sure that users can complain to their service providers, instead of going aware from Mozilla software, and switching to software that appears to work better.


> Please let me
> know if you would like to discuss how to deal with this.

I don't see a need for discussion, but feel free to start the discussion (off this bug).


> The modal dialog box is more harmful than helpful to Firefox.

I completely disagree with your too simple statement. The modal dialog is supposed to be shown (only) whenever software hasn't implemented proper error handling. It's a fallback to make everyone aware if such error handling is missing.

You are free to disable the modal dialog in all scenarios you are able to handle, but it's plain wrong to disable a catch all fallback error message, and thereby making our software appear broken in all scenarios that you've chosen not to handle explicitly, or are unable to handle explicitly.


> Also, the
> Necko team is planning to remove *all* prompts that occur as part of the
> internal operations of the Necko APIs, for various reasons.

I believe you misunderstood what "remove" is supposed to mean.

"Remove" is supposed to mean "replace with something better".


> We already have a bug open for creating a UI for handling SSL-related errors
> as a result of web page resource loads in Firefox. It might involve an
> infobar or something else. I am *sure* the UX team is not going to use a
> modal dialog box for that case.

Fine. But until you have that working, you must not remove the dialog.

And you absolutely must not simply remove the fallback dialog for all the scenarios where you cannot control that an infobar will be visible.

Really, the most you might do, is a Firefox specific change, that suppresses all dialogs, if you decide you want that behaviour in Firefox. And support for getting this disable should be implemented in the core.

And the code that actives the override must live OUTSIDE the shared core, or you're creating a major burden for applications that build on top of the core. You aren't supporsed to remove the error reporting abilities from the shared core.


> The UI for Thunderbird will likely have to be different. 

Irrelevant. Keep the default (that we already had, and that I want to re-add in bug 682329). and it will work as a fallback for any application.

Override that default in Firefox, if you believe that's reasonable, but keep it in the core platform.


> I am willing to
> help someone on your team implement a good UI for this.

You alone are unable to implement a good UI for each of the applications that make use of the Mozilla platform. Your time is better invested in a default fallback implementation, that application developers can override, if they need to.


> Please let me know
> who to work with and what priority you think this bug should be given,
> relative to other things that you would like me to do to help the
> Thunderbird team.

I ask that you don't resist getting the fallback re-added in bug 682329, using the patch that I wrote. That's all I'm asking for.

Comment 7

[Ben Bucksch (:BenB)]

Please remember that there are many more XULRunner applications apart from Firefox and Thunderbird. It must work there, too (without new code, because their maintainers are not aware of this), otherwise you introduce serious security holes in these applications.

Comment 8

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

I believe that I made an error when I did the SSL thread removal and/or XPCOM proxy removal: 

  if (!collected_errors)
  {
    // This will happen when CERT_*Verify* only returned error(s) that are
    // not on our whitelist of overridable certificate errors.
    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("[%p] !collected_errors: %d\n",
           fdForLogging, static_cast<int>(defaultErrorCodeToReport)));
    PR_SetError(defaultErrorCodeToReport, 0);
    return nsnull;
  }

I will post the fix in this bug and request approval for landing in mozilla-central. I believe this will re-introduce the bug that Thunderbird shows the cert override dialog box even for non-overridable cert errors. If so, I will file a new bug in the Thunderbird component for that and I will provide the fix for it, which should be simple.

Comment 9

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

Attachment: Example modification showing how to add default cert error behavior back to Thunderbird

I know dbienvenu asked in email for these errors to be made overridable. That is a simple change to make, code-wise, but there isn't agreement to do that in PSM yet. I will start a discussion with Kai and secteam about it.

If we don't agree to do that, then here is a patch for showing the default dialog box in Thunderbird again. But, note that the description of the existing bad cert listener says:

/**
 * This class implements nsIBadCertListener2.  Its job is to prevent "bad cert"
 * security dialogs from being shown to the user.  Currently it puts up the
 * cert override dialog, though we'd like to give the user more detailed
 * information in the future.
 */

So, the purpose of this class is precisely to prevent these types of dialog boxes from being shown. That makes me confused as to what you'd like to see.

Note that this patch requires some changes to PSM to work, because PSM has a bug where it does not set the SSLStatus of the socket before calling the SSL error listener. I will attach three patches to this bug, which resolve that problem; they are too complicated to take on mozilla-aurora but I think we could make a much simpler patch that also fixes that bug, if Thunderbird team decidse that they want the "Click OK to not continue" dialog box UX or a similar one that is based on nSISSLErrorListener.

Comment 10

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

Attachment: Ensure that the SSLStatus is set before we notify the SSL error listener, Part 1

Comment 11

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

Attachment: Ensure that the SSLStatus is set before we notify the SSL error listener, Part 2

Comment 12

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

Attachment: Ensure that the SSLStatus is set before we notify the SSL error listener, Part 3

These 3 patches to PSM are based on the patches in bug 703834 that landed today on mozilla-inbound. They are part of a series of prerequisite patches for bug 660749 and weren't written to directly address this issue, so they are bigger than they need to be. But, they are useful for testing the "Example modification showing how to add default cert error behavior back to Thunderbird" patch.

To test this stuff, I configured an IMAP account to Kai's test server, based on the instructions in https://bugzilla.mozilla.org/show_bug.cgi?id=590364#c7. Kai's server is not really an IMAP server, but it doesn't matter for the purposes of this bug.

If we do not make the MD5 blocking overridable, then it looks like Thunderbird would need additional changes besides the ones in my example patch. For example, you probably would want a different UX for LDAPS servers that have MD5-based certs, because you don't want a dialog box popping over and over when the user is typing in the To:/CC:/BCC: fields. But, you should be able to use the overall structure of the patch (implementing both nsIBadCertListener2 and nsISSLErrorListener2) to implement what you need.

Also, note that the nsISSLErrorListener2 notifySSLError function is called for any SSL protocol error, in addition to non-overridable cert errors. Unfortunately, PSM doesn't expose an API that allows applications to differentiate between overridable cert errors, non-overridable cert errors, and non-cert SSL errors. Perhaps, you would be better off modifying my example so that it only uses the certerror dialog box when we we actually have a sslStatus & cert, and uses a regular nsIPrompt.alert when we don't.

Comment 13

[Ben Bucksch (:BenB)]

> * Currently it puts up the
> * cert override dialog, though we'd like to give the user more detailed
> * information in the future.

> So, the purpose of this class is precisely to prevent these types of dialog boxes from being shown. > That makes me confused as to what you'd like to see.

To me, that code comment says that we want to give *more* information (and probably in a more user-friendly way). The bug here is that we show *no* information anymore.

Comment 14

[David :Bienvenu]

(In reply to Brian Smith (:bsmith) from comment #9)
>

> So, the purpose of this class is precisely to prevent these types of dialog
> boxes from being shown. That makes me confused as to what you'd like to see.
Iirc, the history of this comment is that a number of years ago, PSM stopped putting up the cert override dialog, but put up cert error dialogs that the user couldn't do anything with.  So we both want to suppress those errors, and put up the cert exception dialog, when possible.

For non-overridable errors (and by overridable, I really meant allowing cert exceptions), I'm OK with putting up the error. I think I'd prefer to allow md5 certs if possible, but I'd settle for putting up a useful error message, and if we get a lot of feedback about this, adding code to allow the md5 certs.

Comment 15

[David :Bienvenu]

Comment on attachment 619514 [details] [diff] [review]
Example modification showing how to add default cert error behavior back to Thunderbird

Thx for the example code. I'm giving this more of a feedback+. To pass review, I'd want to use lets instead of vars, use Cc., and address the XXX comment.

Comment 16

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

(In reply to Brian Smith (:bsmith) from comment #9)
> Note that this patch requires some changes to PSM to work, because PSM has a
> bug where it does not set the SSLStatus of the socket before calling the SSL
> error listener. I will attach three patches to this bug, which resolve that
> problem; they are too complicated to take on mozilla-aurora but I think we
> could make a much simpler patch that also fixes that bug, if Thunderbird
> team decidse that they want the "Click OK to not continue" dialog box UX or
> a similar one that is based on nSISSLErrorListener.

That is now PSM bug 754369.

Comment 17

[Mark Banner (:standard8)]

This has gone quiet, but is tracking for the next release - what is the way forward here?

Comment 18

[David :Bienvenu]

I think this is in Brian's court right now - this is blocked on bug 754369. I rather thought there were patches to fix that issue, but I don't see any in that bug.

Comment 19

[Ben Bucksch (:BenB)]

bsmith, can you give us info on how to proceed here? I got lost on comment 16 here vs. bug 754369 comment 2.

Comment 20

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

See bug 758314, which is almost ready to land. It makes the MD5 signature error overridable so that this issue doesn't apply. This issue will apply mainly to the case where the cert has been overridden. A patch for bug 754369 is required to fix this bug. In addition to the patch in bug 754369, a patch similar to the one I posted in this bug will be needed.

Comment 21

[Ben Bucksch (:BenB)]

> A patch for bug 754369 is required to fix this bug.

OK. Any progress there?

I just want to ensure that we nave have the situation that a connection fails silently without telling the user any hint or reason. And of course that we never allow an insecure connection unless we and the user really want to.

Comment 22

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

(In reply to Ben Bucksch (:BenB) from comment #21)
> OK. Any progress there?

Yes, I have some working patches for that bug but part needs to be redone to account for bug 729536. We are not very far away but I also don't have a solid ETA.

> I just want to ensure that we nave have the situation that a connection
> fails silently without telling the user any hint or reason. And of course
> that we never allow an insecure connection unless we and the user really
> want to.

IIRC, there are other networking errors that aren't reported to the user.

These errors actually get reported to the application (the caller of PR_Read/PR_Send, etc.) the same way any network error does. So, if these errors are totally unreported, that means the error handling code in the various protocol implementations (IMAP, etc.) is discarding the error without reporting it.

So, even with this bug fixed, AFAICT you will have other instances of basically the same problem, until you fix the underlying cause in nsIMAPProtocol and elsewhere.

Comment 23

[Ben Bucksch (:BenB)]

> Yes, I have some working patches for that bug but part needs to be redone to account for bug 729536.
> We are not very far away but I also don't have a solid ETA.

Great, thanks!

> IIRC, there are other networking errors that aren't reported to the user.
> These errors actually get reported to the application (the caller of PR_Read/PR_Send, etc.)

OK, that's not good. If you have concrete examples, maybe even with reproduction, can you file a bug or name them?
Thanks for the hint!

Comment 24

[David :Bienvenu]

(In reply to Brian Smith (:bsmith) from comment #22)
> These errors actually get reported to the application (the caller of
> PR_Read/PR_Send, etc.) the same way any network error does. So, if these
> errors are totally unreported, that means the error handling code in the
> various protocol implementations (IMAP, etc.) is discarding the error
> without reporting it.
Can you cite PR_Read errors that you think should be reported to the user? nsImapProtocol::CreateNewLineFromSocket reports the various connection errors and a few others like timeouts and resets.

The cert errors tend to happen on connection/start tls, do they not? And the bad cert listener should get notified, shouldn't it?

Comment 25

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

(In reply to David :Bienvenu from comment #24)
> Can you cite PR_Read errors that you think should be reported to the user?
> nsImapProtocol::CreateNewLineFromSocket reports the various connection
> errors and a few others like timeouts and resets.

SEC_ERROR_REVOKED_CERTIFICATE (a PRErrorCode), for example. It looks like in that code the error is represented as an nsresult, not a PRErrorCode. We have helper macros/functions and nsINSSErrorsService to convert back and forth between PRErrorCode and nsresult, but it looks like we don't export all the necessary API (e.g. mapping nsresult to PRErrorCode). 

> The cert errors tend to happen on connection/start tls, do they not?

Yes. These errors will be returned from whatever PR_Recv/PR_Send/etc. just like any other networking errors are, as the SSL handshake is done transparently as part of PR_Recv/PR_Send/etc.

> And the bad cert listener should get notified, shouldn't it?

The bad cert listener only gets notified for overridable cert errors. The SSL error listener gets notified about non-overridable cert errors and other SSL errors.

I prefer not to use nsIBadCertHandler2 or nsISSLErrorListener at all; instead I would instead centralize all my network error handling in CreateNewLineFromSocket and similar places, and use nsINSSErrorsService.getErrorClass to determine whether to show a bad cert UI or a "regular" networking error UI. However, implementing nsIBadCertHandler2 and/or nsISSLErrorListener *is* also supposed to work, and does work except that you sometimes cannot access the sslStatus in your nsISSLErrorListener, which is what bug 754369.

Comment 26

[Mark Banner (:standard8)]

(In reply to Brian Smith (:bsmith) from comment #20)
> See bug 758314, which is almost ready to land. It makes the MD5 signature
> error overridable so that this issue doesn't apply. This issue will apply
> mainly to the case where the cert has been overridden. A patch for bug
> 754369 is required to fix this bug. In addition to the patch in bug 754369,
> a patch similar to the one I posted in this bug will be needed.

Ok, I'm really confused. You say that the patch for bug 754369 is required to fix this, but in that bug you say that a patch for this bug will fix the issue?

What do we really need here? We should really be getting this fixed asap.

Comment 27

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

(In reply to Mark Banner (:standard8) from comment #26)
> Ok, I'm really confused. You say that the patch for bug 754369 is required
> to fix this, but in that bug you say that a patch for this bug will fix the
> issue?
> 
> What do we really need here? We should really be getting this fixed asap.

First, MD5 errors have been made overridable in bug 758314, so no more action is required as far as MD5-based certificates is concerned.

To report other errors that are *non*-overridable (e.g. revoked), a patch for bug 754369 is needed and then after that we will have to have a patch for this bug; similar to the "Example modification showing how to add default cert error behavior back to Thunderbird" patch attached to this bug. (That patch won't work until bug 758314 is fixed.)

Comment 28

[Kai Engert (:KaiE:)]

This should be a blocker for Thunderbird 17.

Not having error reporting for lots of scenarios is a serious usability regression, we must have a fix for the ESR branch.

Comment 29

[Kai Engert (:KaiE:)]

I didn't mean to clear the thunderbird15+ flag

Comment 30

[Kai Engert (:KaiE:)]

I propose to fix this bug by creating a fork of the security glue code in mozilla/security/manager, see bug 783548 and bug 783549.

Once we have those bugs fixed, we could land patch v8 from bug 682329 in the comm-central fork.

Comment 31

[Kai Engert (:KaiE:)]

Attachment: prompt approach, patch v9

I ported v8 from 682329 to mozilla 17, this patch assumes you have forked PSM to comm-central/psm and have patches from both bug 783548 and bug 783549 applied. Then you can apply this patch on top (which also includes the patches from bug 745132 and bug 745133).

If you'd like to test on your own, you could use the following setup:
- get the "stunnel" utility
- stunnel -p a-file-with-server-cert-and-key.pem -I imap-server-hostname -r 143 -d 9453 -f -C NULL-MD5
  (this will run an SSL gateway in front of your imap server's port 143,
  listening on port 9453, 
  allowing only a no-security cipher which is disabled in Mozilla clients)
- change the configuration of your mail account to connect to SSL port 9453

With stock Thunderbird, no error message is shown.
With these patches applied, Thunderbird will tell you "no cypher overlap".

Comment 32

[Kai Engert (:KaiE:)]

> With stock Thunderbird, no error message is shown.
> With these patches applied, Thunderbird will tell you "no cypher overlap".

This is just one example for a scenario where such error messages are helpful.

Here is a list of other scenarios where Tunderbird currently doesn't show any user feedback, but simply gives the impression of stalling and not working:

- the server only allows older SSL procotol versions that we disable in
the client

- the connection to the mail server is being attacked, someone trying to
make use of the TLS renegotiation attack, and we disallow it, because we
detect that the attacker is unable to fulfil the requirements of the
protocol

- the user has configured the incorrect port number to the server, and
we try to speak SSL/TLS to a plaintext server, and we get protocols
errors like SSL_ERROR_RX_RECORD_TOO_LONG

- there's a problem with proxy configuration, or the proxy is offline

- we try to use a modern SSL/TLS feature (e.g. compression etc.), but
the feature is broken on the server, a protocol error occurrs and we
abort the connection

- a mail server could ask for authentication using an SSL client
certificate, but the client doesn't have it installed, or the server
reports an access denied (e.g. because the user's cert is expired)

Comment 33

[Ben Bucksch (:BenB)]

Kai, thanks for this list. Many of these situations are very realistic.

Comment 34

[Kai Engert (:KaiE:)]

FYI, Bug 783974 has an implementation of logging to the error console.
(insufficient IMHO, but better than nothing)

Comment 35

[Kai Engert (:KaiE:)]

I've sacrificed my saturday in the hope that I have now provided a set of patches that make everyone happy.

Bug 783974 has the error console logging, which has the base for producing required error messages. I have addressed all comments from bsmith and I hope it will get an r+ as is.

In bug 785426, the most recent patch (v6) should be what everyone has been asking for. It provides a callback API that applications can use by implementing a contract ID.

In bug 745132 and bug 745133 I've provided an implementation of the callback API in mail code.

I would appreciate some support to get bug 785426 landed as is, after having spent so much time on this issue. Thanks.

Comment 37

[gumof]

https://bugzilla.mozilla.org/show_bug.cgi?id=790924

Comment 38

[Ben Bucksch (:BenB)]

Brian, ping. You had promised us a patch for TB 17.

Comment 39

[Ben Bucksch (:BenB)]

Brian? We had an agreement.

Comment 40

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 41

[Wayne Mery (:wsmwk)]

(In reply to David :Bienvenu from comment #18)
> this is blocked on bug 754369.
> I rather thought there were patches to fix that issue, but I don't see any in that bug.

bug 754369 was recently marked P5. Not very promising