107491 - PSM error messages leave a lot to be desired

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

[e-mail] (Robert Joop) wrote that he gets the following error 
messages:

> with mozilla 0.9.5 i get "an unknown SSL error (-8101)",

> i get
> "You cannot connect to <FQDN> because of an unknown SSL error (-12269)"
> when i use an expired client certificate.

> i get
> "You cannot connect to <FQDN> because of an unknown SSL error (-12227)"
> when i press cancel when asked for a client certificate.

Since error strings are defined for ALL of SSL, NSS, and NSPR errors, 
these dialogs ought to report error strings.  

(Yes, I know, it is additional localization burden, but we should still 
do it, IMO.)

Comment 1

[Stephane Saux]

Agreed in principle.
Rangan, we may want to store the strings in the resource file with the name of
the string the error code (or some basic variant of that.)

Comment 2

[Stephane Saux]

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

Comment 3

[Stephane Saux]

adding useful comments from the dupe 108626

In nsNSSIOLayer::nsHandleSSLError() we only handle a few of the errors defined
in sslerr.h

Some of the errors we should clearly handle are:
 74 SSL_ERROR_REVOKED_CERT_ALERT            = (SSL_ERROR_BASE + 18),
 75 SSL_ERROR_EXPIRED_CERT_ALERT            = (SSL_ERROR_BASE + 19),

So that we can do correct validation when doing client auth.
See NSS bug 108250.

Comment 4

[Rangan Sen]

Attachment: patch

This patch should make error handling during ssl connection finer grained.
The idea is to handle all the error codes that might occur here [switch - case]
but try to optimize the localization overhead by grouping some of the cases
[where possible, without losing relevance] to use the same error string [for
example errors like malformed response/bad response/unknown response
type/status from ocsp server are combined into a single "unknown or malformed
response from ocsp server" kind of message. But for 'unknown cert' error from
ocsp server, we should have an unique error message]. Also, whenever more that
one error cases map into one message, the error code is provided with the
message to ensure no loss of info.
The default: case is still there, but ideally, we should never run into it.

Comment 5

[Javier Delgadillo]

Comment on attachment 64028 [details] [diff] [review]
patch

r=javi for the code.  Someone else should review the text and make sure it's
appropriate and "user-friendly"

Comment 6

[Rangan Sen]

cc-ing Sean...
Sean, Could you pl review the text ...

Comment 7

[Sean Cotter]

>Could not establish a secure connection.

In general, we've been trying to avoid "secure" in the interface, in favor of
words like "encrypted" or "authenticated". So I would prefer to see this phrase
replaced with one of the following, depending what's actually going on:

- Could not establish an encrypted connection. (Authentication is implied in
this case. Most users won't know that, but I think just "encrypted" is enough here.)
- Could not establish an authenticated connection. (when encryption is not involved)
- Could not establish an encrypted and/or authenticated connection. (if there
are cases where you don't know exactly)


>There is a problem with your database [Error Code: %S]. Contact your system
admistrator.

Should we specify "certificate database" if that is the case here? Might help
the system admin to know.


>%S has received an incorrect or enexpected message

Change to this:

%S has received an incorrect or unexpected message.


In general, I think we should include the abbreviation "CRL" in error messages
about CRLs, since that's how they're often referred to in the interface and
elsewhere:

>Certificate Revocation List from the CA certifying %S is past its next update
date. Please update it.

Change to this, for clarity:

Certificate Revocation List (CRL) from the CA certifying %S is past its Next
Update date. Please update the CRL.


>Certificate Revocation List from the CA certifying %S is yet to be valid.
Please check in system clock.

Change to this, for clarity:

>Certificate Revocation List (CRL) from the CA certifying %S is not yet valid.
Please check your system clock.


>Certificate Revocation List from the CA certifying %S has an invalid signature.

Change to this:

Certificate Revocation List (CRL) from the CA certifying %S has an invalid
digital signature.


>Certificate Revocation List from the CA certifying %S is not valid.

Change to this:

Certificate Revocation List (CRL) from the CA certifying %S is not valid.


>Error trying to validate Certificate from %S using OSCP

For all errors that include this phrase, lowercase "certificate". Also for
these, "Unauthorized" should be one word without a hyphen. Also, it's not clear
to me what Future Reponse and Old Response mean. Is there some better way of
describing these?

Comment 8

[Rangan Sen]

Attachment: updated patch

New patch - addresses cotters comments.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Several comments on the patch of 01/14.

1. The patch contains two separate cases for case SEC_ERROR_EXPIRED_CERTIFICATE.
The first such case label immediately follows case SSL_ERROR_BASE_CERT_DOMAIN.
Just delete that particular case label.

2. SSL_ERROR_CLOSE_NOTIFY_ALERT is not a "connection reset".  The message 
"The connection was reset by <peer>" is misleading in this case.  It would
be better to say "<peer> has closed the connection."

3. All the "Contact your system administrator." strings might as well say 
"Say yer prayers, varmint."  Home users (who are their own system admins)
find that unhelpful, and most corporate sysadmins aren't much help with 
bad mac or corrupt DB errors.  For DB problems, it might be better to direct 
the user to a URL on netscape.com that explains the problem and suggests 
solutions.  For bad MAC errors, it might be best to contact the _servers_
administrator.  

Comment 10

[Rangan Sen]

Attachment: update [checked in]

Updated with Nelson's comments.
For Comment#3, in case of db problems, removed the "contact your System
Administrator" string for now - till I can find which url to direct the user to
in such cases.

Does anyone have a suggestion on this url should be?

Comment 11

[Christopher Blizzard (:blizzard)]

+  char buf[80];

80 seems a bit excessive to me but it probably won't harm anything.

Other than that, looks fine to me.  sr=blizzard

Comment 12

[Christopher Blizzard (:blizzard)]

Comment on attachment 64916 [details] [diff] [review]
update [checked in]

sr=blizzard

Comment 13

[Rangan Sen]

Patch checked in. Note that this patch handles only ssl/security errors [not
nspr errors - any nspr errors would still go to the default handler]

Comment 14

[Rangan Sen]

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

Comment 15

[John Unruh]

Verified.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

If this bug is fixed, then why do we continue to receive reports about 
-8101 errors??

Comment 17

[Rangan Sen]

This patch handles the error cases during a ssl connection, and -8101 has been 
handled. It would be pretty wiered if this still shows up - does is it happen 
during a ssl connection setup or somewhere else?

Comment 18

[John Unruh]

You can still see -8101 when visiting this site, which is probably missing part 
of the Verisign chain. https://www.cu-webssl.net

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Users report that N 7.0 displays the error message
'Could not establish an encrypted connection because certificate presented by 
www.cu-webssl.net is invalid or corrupted. Error Code:-8101'.
-8101 is SEC_ERROR_INADEQUATE_CERT_TYPE and the error string for it is 
"Certificate type not approved for application."

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

I am reopening this bug.  The changes made previously for this bug probably
improved things by producing better errors in some limited situations and
for some error codes, but did not really eliminate the problem.

PSM's error reporting STILL leaves a lot to be desired.  There are still 
way too many messages that boil down to "it didn't work: -<number>"  
and too many that say "it failed because of an unknown error", even
though the error code that triggered them was distinctly meaningful.

One has merely to search bugzilla for bugs about error messages with the 
strings -81 or -122 to see how common this is.  

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 23

[Matthias Versen [:Matti]]

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

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Just to be clear about something: This bug isn't *BLOCKED* by anything.
It's a simple matter of having better error messsages for the existing 
error codes.  No changes to any other code are necessary for this to 
be fixed.  The error codes are documented in various pages, including
http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslerr.html
This bug just needs someone to work on PSM.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 26

[Ralf Hauser]

Attachment: trustcenter.de.Error.png

Another one, it is particularly cumbersome if firebird doesn't work with the
main SSL cert issuer of the german part of the world.

May well be that they are at fault, but if firebird neither helps them nor me
with a useful error message, I am forced to open MSIE and might stick to that  
:(

Comment 27

[Matthias Versen [:Matti]]

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

Comment 28

[Kai Engert (:KaiE:)]

I want to start by looking at error messages that are produced when there is a failure during network activity.

For whatever reasons, the original authors of error reporting did not make an attempt to handle all existing error messages. They chose a subset. In addition, they did not simply use the error messages mentioned by Nelson, but used other messages, allowed reviewers to change the error message. Different error codes have been grouped to show the same error message. Some error messages are plain text, other's dynamically add the peer's host name, many add the numerical error message.

There is no consistency, and it is a mess.

I don't feel like looking at each and every error code and discuss whether the error message is appropriate.

I really would prefer to replace the existing code with something new, for all error codes, consistently showing the error message as defined in NSS documentation, adding both hostname and error number.

Such a change will be simple to do and I can easily produce such new code from the documentation.

But this has the disadvantage of losing any error message improvements that might have been done for some particular error codes.

Comment 29

[Kai Engert (:KaiE:)]

Attachment: Patch v4

This patch illustrates a new approach that I'm proposing in bug 329017.

It has an error message for each NSPR/NSS error code.
Please read bug 329017 for the background explanation.

This patch replaces existing error message with those contained in the NSS library. We might want to merge the messages, removed by this patch, into the NSS files contained in directory mozilla/security/nss/cmd/lib/

Comment 30

[Kai Engert (:KaiE:)]

Attachment: Screen Shot that illustrates Patch v4

Comment 31

[Simon Fraser [no longer active]]

Is improving the wording of the messages this bug, or another bug?

Comment 32

[Kai Engert (:KaiE:)]

(In reply to comment #31)
> Is improving the wording of the messages this bug, or another bug? 

I propose we start by ensuring that we have at least some appropriate error message for each error code in PSM.

I propose we automatically include the wordings located at some master location. I hope the discussion in bug 329017 will bring us a decision, where that master location will be.

Once we see clear on that, I propose we use a separate bug to improve the wordings.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

5 year old PSM bug.  Festering.

Comment 34

[timeless]

Comment on attachment 213633 [details] [diff] [review]
Patch v4

Are there common fields?

could we define %1$s to mean <server> and %2$s to mean <brand> and so on?

i.e. each replacement string would have a fixed position, for any value we don't have we fill it in with "**localization error**" all numbers are passed as strings.

that would enable the messages to be slightly flexible.

otherwise, let's just get this into cvs and improve it later (in fact my suggestion above could be done later).

Comment 35

[timeless]

Comment on attachment 213633 [details] [diff] [review]
Patch v4

Are there common fields?

could we define %1$s to mean <server> and %2$s to mean <brand> and so on?

i.e. each replacement string would have a fixed position, for any value we don't have we fill it in with "**localization error**" all numbers are passed as strings.

that would enable the messages to be slightly flexible.

otherwise, let's just get this into cvs and improve it later (in fact my suggestion above could be done later).

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213633 [details] [diff] [review]
Patch v4

>+SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION_8151=Certificate contains unknown critical extension.

I think this patch is on the right track.  I think it would be the start of 
a big improvement.

I do NOT think that PSM should delay committing this change merely because 
this new table of error messages cannot be automatically (re)generated from 
NSS sources.  

Let automated (re)generation be phase 2, the second step, the NEXT 
improvement.  But get the new error codes in now.  Please!

I would also suggest NOT putting the error numbers into the name strings.  
e.g. SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION_8151 might better be just
     SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION or even 
     SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION_STRING (to avoid duplication).

Comment 37

[Kai Engert (:KaiE:)]

One reason why I delayed this patch: I wasn't sure whether we really want to overwrite (and remove) existing application defined error strings.

In addition to the error message table automatically generated from NSS code, I believe we should allow the application to override these strings - in case the NSS error message is too geeky for the application owners.

I propose PSM does two string lookups. It should first try whether an application defined string is available. If that fails, it will try to use the NSS error message.

I want to go through my patch and look at the existing error message this patch removes. I will try to keep better error strings.

I want to keep the error reporting as general as possible. I want to avoid host names and brand strings in the middle of those strings, because I want to avoid manual tweaking of strings. This is the motivation for displaying them separately.

There may be error codes that are not related to a hostname. I want to check that at runtime, and only display the hostname section if one is related.

Will work on that now, I really hope I can work on this til completion now.

Comment 38

[Kai Engert (:KaiE:)]

Attachment: Patch v5

New patch.

It makes the error strings defined by NSS available to the application. Currently we use it during SSL connections only - but in the future can leverage them for error messages at other situations.

It keeps all current strings explicitly defined by PSM. Only if no string is defined by PSM, it will use the NSS default error string. (In a second phase we should verify for each override which string is better)

Whenever an SSL error occurs, we will always display:
- the host name the problem occurred with
- the numeric error code
- either the PSM string or the NSS string

Comment 39

[Kai Engert (:KaiE:)]

Attachment: v5 screenshot, sample 1, OLD style

Comment 40

[Kai Engert (:KaiE:)]

Attachment: v5 screenshot, sample 1, NEW style

Comment 41

[Kai Engert (:KaiE:)]

Attachment: v5 screenshot, sample 2, OLD style

Comment 42

[Kai Engert (:KaiE:)]

Attachment: v5 screenshot, sample 2, NEW style

Comment 43

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 241409 [details] [diff] [review]
Patch v5

+SSL_ERROR_RX_UNEXPECTED_CLIENT_KEY_EXCH=SSL received an unexpected Cllient Key Exchange handshake message.

typo: Cllient

Comment 44

[Jo Hermans]

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

Comment 45

[Kai Engert (:KaiE:)]

(In reply to comment #43)
> typo: Cllient

Thanks, I filed bug 359280 to get this fixed in NSS.

Comment 46

[Kai Engert (:KaiE:)]

Comment on attachment 241409 [details] [diff] [review]
Patch v5

Obsoleting patch, we want some additional tweaks.

Comment 47

[Kai Engert (:KaiE:)]

Attachment: Patch v6

Comment 48

[Kai Engert (:KaiE:)]

Comment on attachment 247181 [details] [diff] [review]
Patch v6

With this Patch v6, fatal SSL errors that occur in the context of a browser will now be shown as an error page. I'll attach a screen shot next.
In all other contexts, where no browser window is available, we'll continue to show the error as a dialog alert.

Comment 49

[Kai Engert (:KaiE:)]

Attachment: error page screenshot for patch v6

Comment 50

[Kai Engert (:KaiE:)]

Attachment: Patch v6 fixed

there was a variable init missing, fixed

Comment 51

[Kai Engert (:KaiE:)]

Comment on attachment 247182 [details]
error page screenshot for patch v6

This is obsolete, because it uses a negative error code, that the UI owners don't like. This was also an argument for postponing this patch, because I didn't have a better solution.

But now I believe I have an idea that is acceptable to everybody. Instead of appending the numeric code, let's add the error symbol used in the source code (e.g. SSL_ERROR_BAD_CERT_ALERT).

Comment 52

[Kai Engert (:KaiE:)]

Attachment: Screen Shot for patch v7

Comment 53

[Kai Engert (:KaiE:)]

Attachment: Patch v7

Comment 54

[Kai Engert (:KaiE:)]

Attachment: Patch v7 - mozilla/security/manager portion

Because the patch requires changes in multiple modules, I'm separating patch v7 into several attachments, so I can independently request reviews.

This portion contains the changes to the security module.

Comment 55

[Kai Engert (:KaiE:)]

Attachment: Patch v7 - docshell and netwerk portions

In order to integrate the new class of errors into the existing error page code, some changes are required in docshell and netwerk.

I hope these changes are ok. I would probably be good to avoid using -0x2000 and -0x3000 directly, but I haven't found a good solution yet to avoid them, because the numbers are defined in the optional NSS source code. The discussion in bug 362488 has not yet come to a solution.

Christian, could you please have a look?

Comment 56

[Kai Engert (:KaiE:)]

Attachment: Patch v7 - netError.dtd in dom and browser

Mike, would you be ok with this wording? And the screenshot attached in this bug? You probably remember our discussion about avoiding numeric error codes in the UI. I hope I have found a salomonic compromise, which uses the symbolic name of the error - this is ensure we will be able to make use the of error message, even if the main wording is in a foreign language.

Neil, the same wording will also be required in the dom directory, which will be used for SeaMonkey. Are you ok with adding that new string, and I propose we keep the dom and browser wordings in synch?

Comment 57

[Kai Engert (:KaiE:)]

Comment on attachment 252824 [details] [diff] [review]
Patch v7 - netError.dtd in dom and browser

Neil, please see the previous comment.

Comment 58

[Kai Engert (:KaiE:)]

Attachment: Patch v7 - netError.dtd in dom and browser

Comment 59

[Kai Engert (:KaiE:)]

Attachment: Patch v7 - netError.dtd in dom and browser

Thanks for Stephen Donner who made me aware of a typo. Third attempt of attaching this patch :-)

Comment 60

[Kai Engert (:KaiE:)]

Attachment: Patch v7 - netError.dtd in camino [checked in]

Comment 61

[Kai Engert (:KaiE:)]

Attachment: Screen Shot for patch v7

Updated screenshot to correct typo.

Comment 62

[[e-mail]]

Comment on attachment 252821 [details] [diff] [review]
Patch v7 - mozilla/security/manager portion

>+static const char *
>+getDefaultErrorStringName(PRInt32 err)
>+{
>+  const char *id_str = nsnull;
>+
>+  switch (err)
>+  {
>+    case SSL_ERROR_EXPORT_ONLY_SERVER: id_str = "SSL_ERROR_EXPORT_ONLY_SERVER"; break;
Alternatively could you list the error numbers in the .properties file? (nsIStringBundle has a formatStringFromID but it's just a wrapper that converts the numeric ID to a string).

>+      nsString defMsg;
>+      rv = component->GetPIPNSSBundleString(id_str, defMsg);
>+      if (NS_SUCCEEDED(rv))
>+      {
>+        returnedMessage.Append(defMsg);
>+        returnedMessage.Append(NS_LITERAL_STRING(" "));
>+      }
>+
>+      returnedMessage.Append(NS_LITERAL_STRING("\n("));
>+      returnedMessage.AppendASCII(nss_error_id_str);
>+      returnedMessage.Append(NS_LITERAL_STRING(")\n"));
Is this i18n-safe? Also are you allowed to use AppendLiteral(")\n"); etc?

Comment 63

[[e-mail]]

Forget that, I just twigged you use the id string in the error message.

Comment 64

[[e-mail]]

Although from an i18n point of view it might be better to embed the string in the message in which case you could format it by id anyway ;-)

Comment 65

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 252822 [details] [diff] [review]
Patch v7 - docshell and netwerk portions

Please add -p8 to your diff options?  It'll make your diffs a lot easier to review....

>Index: mozilla/docshell/base/nsDocShell.cpp

>+    else if (NS_ERROR_NSS_FAILURE == aError) {
>+        nsCOMPtr<nsISupports> securityInfo;
>+        aFailedChannel->GetSecurityInfo(getter_AddRefs(securityInfo));

You sure aFailedChannel can't be null here?

>+        if (securityInfo)
>+        {
>+            nsCOMPtr<nsITransportSecurityInfo> tsi(do_QueryInterface(securityInfo));
>+            if (tsi)
>+            {
>+                error.AssignLiteral("nssFailure");
>+                tsi->GetErrorMessage(getter_Copies(messageStr));
>+            }
>+        }

And if not, then what?  Should there be an error message that you use if you get no nsITransportSecurityInfo?

>Index: mozilla/netwerk/base/public/nsNSSError.h
>+ * Failure at NSS library level.
>+ */
>+#define NS_ERROR_NSS_FAILURE \
>+    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY, 2)

We really can't subdivide this further to give useful error messages?

>Index: mozilla/netwerk/base/src/nsSocketTransport2.cpp
>+        if ((errorCode >= (-0x2000) && errorCode < ((-0x2000) + 1000))

I really dislike the magic numbers.  Is there anything we can do about that?

Rest looks ok; in general I think biesi can r+sr this stuff.

Comment 66

[Kai Engert (:KaiE:)]

Attachment: Patch v8 - docshell and netwerk portions

explanation follows

Comment 67

[Kai Engert (:KaiE:)]

Attachment: Patch v8 - mozilla/security/manager portion

explanation follows

Comment 68

[Kai Engert (:KaiE:)]

(In reply to comment #65)
> (From update of attachment 252822 [details] [diff] [review])
> Please add -p8 to your diff options?  It'll make your diffs a lot easier to
> review....

ok


> >Index: mozilla/netwerk/base/public/nsNSSError.h
> >+ * Failure at NSS library level.
> >+ */
> >+#define NS_ERROR_NSS_FAILURE \
> >+    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY, 2)
> 
> We really can't subdivide this further to give useful error messages?


Well, in the initial implementation it wasn't absolutely necessary, because I had intended to use other means to obtain the exact message.

But because of your other inspiring comments, I'm now using a different approach.

I removed the above error code and file.

We'll now use
  NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY, <specific NSS error code>)


> >Index: mozilla/netwerk/base/src/nsSocketTransport2.cpp
> >+        if ((errorCode >= (-0x2000) && errorCode < ((-0x2000) + 1000))
> 
> I really dislike the magic numbers.  Is there anything we can do about that?


Ok, I solved it differently. I introduced a new service, provided by PSM, that can be used to answer the above question.

In addition I implemented a mechanism to query the (localized) error message for a given code.


> >Index: mozilla/docshell/base/nsDocShell.cpp
> 
> >+    else if (NS_ERROR_NSS_FAILURE == aError) {
> >+        nsCOMPtr<nsISupports> securityInfo;
> >+        aFailedChannel->GetSecurityInfo(getter_AddRefs(securityInfo));
> 
> You sure aFailedChannel can't be null here?


I suspect it is very likely we'll have the failed channel available - because those security errors can only get produced if we have actually started a connection.

However, I agree we should play safe and test for null. Changed in the patch.

This introduces the likelihood we need to obtain the error message from somewhere else - instead of the channel. Which is why I added the other function to the service, as explained above.


> >+        if (securityInfo)
> >+        {
> >+            nsCOMPtr<nsITransportSecurityInfo> tsi(do_QueryInterface(securityInfo));
> >+            if (tsi)
> >+            {
> >+                error.AssignLiteral("nssFailure");
> >+                tsi->GetErrorMessage(getter_Copies(messageStr));
> >+            }
> >+        }
> 
> And if not, then what?  Should there be an error message that you use if you
> get no nsITransportSecurityInfo?


Right. 

The new implementation is:
If channel is available, use prepared error message, that will contain the hostname the error is related to.
Else use the error message without host name.

Comment 69

[Kai Engert (:KaiE:)]

Comment on attachment 253867 [details] [diff] [review]
Patch v8 - docshell and netwerk portions

(In reply to comment #65)
> Rest looks ok; in general I think biesi can r+sr this stuff.

Christian, could you please have a look?

Comment 70

[Robert Relyea]

Comment on attachment 253868 [details] [diff] [review]
Patch v8 - mozilla/security/manager portion

r+

Some minor comments:

Your code that changes SSL error numbers to SSL Error names could be done with a table (see the sample code in mozilla/security/nss/cmd/lib in the Error code stuff. This table could be automatically generated from sslerr.h secerr.h.

At some point we should add the SEC errors as well (the current patch just has NSPR and SSL errors).

neither of these are serious enough to hold up the existing patch.

bob

Comment 71

[Kai Engert (:KaiE:)]

(In reply to comment #70)
> Your code that changes SSL error numbers to SSL Error names could be done with
> a table (see the sample code in mozilla/security/nss/cmd/lib in the Error code
> stuff. This table could be automatically generated from sslerr.h secerr.h.

In fact, the code I'm adding is already automatically generated. The code I am using is in bug 329017.

However, the intention is to allow the strings to be localized. So it is necessary to put the error strings into a properties file.


> At some point we should add the SEC errors as well (the current patch just has
> NSPR and SSL errors).

Hmm, I think you overlooked them. They are there already.


> neither of these are serious enough to hold up the existing patch.

Great, thanks!

Comment 72

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 253867 [details] [diff] [review]
Patch v8 - docshell and netwerk portions

+++ mozilla/docshell/base/nsDocShell.cpp	3 Feb 2007 04:15:07 -0000
+        if (securityInfo)
+            tsi = do_QueryInterface(securityInfo);

do_QI is nullsafe... you could avoid this if

Have you considered using nsIErrorService? I.e. using the appropriate format for the .properties file and just calling registerErrorBundle in PSM. Docshell could then use nsIStringBundleService::FormatStatusMessage to get the message.

You'd still need GetXPCOMFromNSSError though...

+++ mozilla/netwerk/base/public/nsINSSErrorsService.idl	3 Feb 2007 04:15:07 -0000
+    void getXPCOMFromNSSError(in PRInt32 aNSPRCode,
+                              out nsresult aXPCOMErrorCode);

Please make this an actual return value, i.e.:
  nsresult getXPCOMFromNSSError(in PRInt32 aNSPRCode);

(and @return in the comment)

Similar for getErrorMessage, and that should also use AString instead of wstring

+#define NS_NSS_ERRORS_SERVICE_CONTRACTID "@mozilla.org/nss_errors_service;1"

contract IDs shouldn't live in interfaces generally, move this to nsNetCID.h.

+++ mozilla/netwerk/base/src/nsSocketTransport2.cpp	3 Feb 2007 04:15:07 -0000

+                nsresult nss_xpcom_code;
+                nsresult conversion_status =

hm, this file/module tends to use the nssXPCOMCode / conversionStatus style for variable names...

+                  nsserr->GetXPCOMFromNSSError(errorCode, &nss_xpcom_code);
+
+                if (NS_SUCCEEDED(conversion_status))
+                  rv = nss_xpcom_code;

please use 4-space indentation for consistency with the rest of the file

r+sr=me, though I'd like to see the new patch too if you change to nsIErrorService

Comment 73

[Kai Engert (:KaiE:)]

(In reply to comment #72)

Christian, thanks a lot for your review.
I addressed all your required change requests.


> do_QI is nullsafe... you could avoid this if

"if" removed


> Please make this an actual return value, i.e.:
>   nsresult getXPCOMFromNSSError(in PRInt32 aNSPRCode);
> (and @return in the comment)

done


> Similar for getErrorMessage, and that should also use AString instead of
> wstring

done


> +#define NS_NSS_ERRORS_SERVICE_CONTRACTID "@mozilla.org/nss_errors_service;1"
> contract IDs shouldn't live in interfaces generally, move this to nsNetCID.h.

moved, and added a comment about the required interface


> hm, this file/module tends to use the nssXPCOMCode / conversionStatus style for
> variable names...

renamed both variables


> please use 4-space indentation for consistency with the rest of the file

done


> Have you considered using nsIErrorService? I.e. using the appropriate format
> for the .properties file and just calling registerErrorBundle in PSM. Docshell
> could then use nsIStringBundleService::FormatStatusMessage to get the message.
> You'd still need GetXPCOMFromNSSError though...

I did not (yet) address your optional suggestion to convert to nsIErrorService, because it would require another review cycle of the security code as well. Thanks for making this proposal optional at this time. I will look at it after getting this first cycle done.

Comment 74

[Kai Engert (:KaiE:)]

Attachment: Patch v9 - docshell and netwerk portions [checked in]

v9 = Patch v8 after addressing Christian's minor change requests.

Comment 75

[Kai Engert (:KaiE:)]

Attachment: Patch v9 - mozilla/security/manager portion [checked in]

Christian's request to change the string type in the interface required a minor syntax change in the implementation. No other changes in this new patch, I'm carrying forward r=rrelyea

Comment 76

[Kai Engert (:KaiE:)]

Attachment: Screen Shot v9

Here is an updated Firefox screenshot. This reflects the latest patches. The differences from v7 are:
- the error ID is now in lowercase, in order to be less intrusive
- added a message for Firefox only that points end users to the "report broken web site" feature available in the help menu.


Let me explain this screen shot in full detail, which can look differently depending on the actual failure occurred at the SSL level.


"Secure Connection Failed"

  general heading, always same


"An error occurred during a connection to %hostname%"

  general introduction, always same


"Received an incorrect or unexpected message."

  This is the exact explanation of the experienced error,
    as dynamically provided by the security library.
  Will be a localized string, i.e. when foreign end users
    send a screenshot, admins might be unable to translate.


(ssl_error_handshake_failure_alert)

  This is the technical error identifier.
    as dynamically provided by the security library.
  Will always be displayed in its original english form,
    which allows admins to understand the error reason 
    without having to translate.
  In the past this used to be the negative error number.
  As there has been controversy about using error numbers in the UI,
    this proposal it to add a human readable error identifier.


"The page you are trying to view can not be shown because the web site uses an invalid or unsupported security protocol.
Please contact the web site owners to inform them of this problem."

  This is a general wording that explains what is going on.
  Always the same


"Alternatively, use the command found in the help menu to report this broken site."

  This string is proposed for Firefox, only.
  It implements a recommendation that Mike Connor provided during a meeting.
  It points end users to the "Help / report broken web site"
    feature to report broken sites back to Mozilla.org


Mike, could you please review / approve this initial implementation?

Comment 77

[Kai Engert (:KaiE:)]

Comment on attachment 255040 [details]
Screen Shot v9

I meant to ask for review on the patch, not the screenshot.

Comment 78

[Kai Engert (:KaiE:)]

Attachment: Patch v9 - netError.dtd in dom and browser [checked in]

Carrying forward Neil's sr on the seamonkey / dom portion.

Asking Mike to review for Firefox.

Comment 79

[Kai Engert (:KaiE:)]

Comment on attachment 255037 [details] [diff] [review]
Patch v9 - mozilla/security/manager portion [checked in]

99% of this patch is improving our error messages. This has been waiting to get landed very long. I think we should not delay this longer.

As explained earlier, this patch is able to report the error in the classic style, using a dialog (as we still require it for e.g. Thunderbird), and in the new style, as an error page.

I would like to land this patch now and hook up error page reporting later, once we get code review on the error page wording patch from the Firefox owners.

Comment 80

[Kai Engert (:KaiE:)]

Comment on attachment 255036 [details] [diff] [review]
Patch v9 - docshell and netwerk portions [checked in]

This patch to docshell/webshell/netwerk can be safely divded into two separate portions.

Portion 1 is adding new interfaces that the crypto module implements. I would like to land this portion now, as it is required to land the error message improvements in the crypto code.

Portion 2 is adding and enabling the new error page, which is not yet approved, so I'm going to delay this portion.

Comment 81

[Mike Connor [:mconnor]]

Comment on attachment 255041 [details] [diff] [review]
Patch v9 - netError.dtd in dom and browser [checked in]

So, this is somewhat better, but I'm pretty sure the wording is not consistent with other error messages.

That said, its an improvement, and we're going to do a general string review/cleanup on trunk in a little while.

r=me on that basis.

Comment 82

[Kai Engert (:KaiE:)]

Comment on attachment 255041 [details] [diff] [review]
Patch v9 - netError.dtd in dom and browser [checked in]

Thanks for the review Mike. By now I've checked in all patches to the trunk.


The now completed work has been an important step on our journey to improve the error messages.

We now have strings available for all the errors that can happen.


The code we added enabled those new error wordings for SSL connections.


But there remains work to do. There are many other places in PSM where an error can occur. And these also should be changed to make use of the new wordings.

Because of that I'm going to leave this bug open.

I'm thinking we should convert this bug into a tracker bug, and discuss further improvement patches in separate bugs.

Comment 83

[u60234]

Comment on attachment 255037 [details] [diff] [review]
Patch v9 - mozilla/security/manager portion [checked in]

I think I found a couple of typos in the new error strings:

>Index: mozilla/security/manager/locales/en-US/chrome/pipnss/nsserrors.properties
>===================================================================

>+SSL_ERROR_RX_UNEXPECTED_CLIENT_KEY_EXCH=SSL received an unexpected Cllient Key Exchange handshake message.

Client Key Exchange

>+SSL_ERROR_DECOMPRESSION_FAILURE_ALERT=SSL peer was unable to succesfully decompress an SSL record it received.

successfully 

>+SEC_ERROR_UNRECOGNIZED_OID=Unrecognized Object IDentifier.

Is this capitalization correct?

Comment 84

[Cédric Corazza]

I found another one :

>+SEC_ERROR_CRL_EXPIRED=The CRL for the certificate's issuer has expired.  Update it or check your system data and time.

date and time

Comment 85

[Kai Engert (:KaiE:)]

Thanks for catchings these string errors. I filed bug 371024.

Comment 86

[Kai Engert (:KaiE:)]

Typos fixed in both NSS and PSM on trunk.

Comment 87

[Nelson Bolyard (seldom reads bugmail)]

A lot of people seem to think that the PSM error string work is already 
done, on the trunk.  They think that once the current trunk becomes a 
released version of firefox, all the error string troubles will have gone
away, and NSS's error strings will finally be displayed for all security
errors.  

But it ain't so.

I'm running a recent trunk build.  Today, in response to a request for 
help from a web site administrator, I visited a server with a cert
with a bad Key Usage extension.  NSS returned the error code sec_error_inadequate_key_usage, for which NSS's error string is 
"Certificate key usage inadequate for attempted operation."

But PSM displayed this:

> Secure Connection Failed
>
> An error occurred during a connection to 131.107.193.14.
> Could not establish an encrypted connection because the site uses a
> certificate that is invalid or corrupted. (sec_error_inadequate_key_usage)
>
> The page you are trying to view can not be shown because the web site uses an
> invalid or unsupported security protocol.
>
>     * Please contact the web site owners to inform them of this problem.

The error code sec_error_inadequate_key_usage is not correctly explained by
either :
- "invalid or corrupted"
or
- "web site uses an invalid or unsupported security protocol."

The difference between what I saw (with the trunk) and what he saw (with FF2)
was the addition of these parts:

> (sec_error_inadequate_key_usage)
>
> The page you are trying to view can not be shown because the web site uses an
> invalid or unsupported security protocol.
>
>     * Please contact the web site owners to inform them of this problem.

Sadly, "invalid or unsupported security protocol" is misleading (a key 
usage extension is not a protocol) and doesn't help the web site admin to 
determine what was wrong with his cert.

Comment 88

[Daniel Veditz [:dveditz]]

"(sec_error_inadequate_key_usage)" seems pretty specific, adding it seems like a huge improvement in pointing towards the problem.

Comment 89

[Kai Engert (:KaiE:)]

Nelson, the reason why we displayed the string
  "Could not establish an encrypted connection because the site 
   uses a certificate that is invalid or corrupted"
is bug 355643. 
Are you able to help out with that?

The above should be done to fix it and display the correct error explanation.


The exact error description is usually technical. So in order to explain what happened in end-user-speak, we have another section at the end. This section is currently "not" individualized. It is currently the same for all NSS error codes.

The wording
  "The page you are trying to view can not be shown because the web site 
   uses an invalid or unsupported security protocol.
  * Please contact the web site owners to inform them of this problem."
was my initial attempt to provide it.

Proposals on how to improve this are welcome.

Comment 90

[Nelson Bolyard (seldom reads bugmail)]

Kai: We MUST get rid of this invariant phrase: 
   "the web site uses an invalid or unsupported security protocol."
It is misleading and wrong for the vast majority of SSL errors.  

In current mozilla projects (FF,TB,SM, others) the only "valid supported 
security protocols" are SSL v3.0 or v3.1 (TLS 1.0).  The phrase "invalid or 
unsupported security protocol" means "some protocol other than SSL v3.0 and 
SSL v3.1".  Since the vast majority of SSL errors do NOT indicate the use 
of some other security protocol, we shouldn't say that all SSL errors are
due to some other security protocol.

If you're looking for a phrase that means "it didn't work well enough to
say it's secure", let me suggest something like these:

"the server sent data that was unacceptable or whose authenticity could 
not be verified" 
or "the server did not send data whose authenticity could be verified" 
or "data with verifiable authenticity was not received from the server"  
or "verifiable authentic data was not received from the server" 
or "the communication could not be verified as authentic"     (I like this one)
or "the authenticity of the received data could not be verified" 
or "it didn't work well enough to call it secure"
or "it didn't work".

Are these messages ever displayed in contexts where they might not be talking
about web servers?  Could this message be caused by attempting to read a 
signed email?  Might the "peer" be "your email correspondent", rather than
"the web site" or "the server"?  NSS uses the phrase "the peer" in cases 
where the message's peer might be any of these.

The phrase "Please contact the web site owners to inform them of this problem."
is FINE with me.  :-)

Comment 91

[Kai Engert (:KaiE:)]

(In reply to comment #90)
> Kai: We MUST get rid of this invariant phrase: 
>    "the web site uses an invalid or unsupported security protocol."
> It is misleading and wrong for the vast majority of SSL errors.  

Ok, accepted.


> If you're looking for a phrase that means "it didn't work well enough to
> say it's secure", let me suggest something like these:
> 
> or "the communication could not be verified as authentic"     (I like this one)

Hmm. The communication seems a bit abstract.


> or "the authenticity of the received data could not be verified" 

I like this one best! :-)

Let me attach a patch that changes the wording.


> Are these messages ever displayed in contexts where they might not be talking
> about web servers?

No. This additional phrase is restricted to times when we render error messages as an error page in the browser. This is why it's also safe to say "the page" and "web site" in the phrase.

Comment 92

[Kai Engert (:KaiE:)]

Attachment: Error page wording change, patch v10 [checked in]

Nelson proposes a wording change and I agree.

old:
  the web site uses an invalid or unsupported security protocol

new:
  the authenticity of the received data could not be verified

Mike, could you please review the change in mozilla/browser for Firefox?

Neil, could you please review the change in mozilla/dom for SeaMonkey?

Comment 93

[Kai Engert (:KaiE:)]

Comment on attachment 268194 [details] [diff] [review]
Error page wording change, patch v10 [checked in]

Stuart, please see previous comment. Could you please review this wording change in mozilla/camino?

Comment 94

[Stuart Morgan]

Comment on attachment 268194 [details] [diff] [review]
Error page wording change, patch v10 [checked in]

r=smorgan for the Camino change.

Comment 96

[Mike Connor [:mconnor]]

Comment on attachment 268194 [details] [diff] [review]
Error page wording change, patch v10 [checked in]

I'm not sure this is user-understandable enough, but its more accurate in the end.

Comment 97

[Kai Engert (:KaiE:)]

Comment on attachment 268194 [details] [diff] [review]
Error page wording change, patch v10 [checked in]

This patch checked in to trunk.

Comment 98

[Marek Stępień [:marcoos, inactive]]

(In reply to comment #92)
> old:
>   the web site uses an invalid or unsupported security protocol
> 
> new:
>   the authenticity of the received data could not be verified

This is a semantic change in the error message, that should be notified to localizers by changing the entity names. CC'ing Pike.

Comment 99

[Kai Engert (:KaiE:)]

Attachment: Patch: Rename string ID nssFailure to nssFailure2 [checked in]

(In reply to comment #98)
> > old:
> >   the web site uses an invalid or unsupported security protocol
> > 
> > new:
> >   the authenticity of the received data could not be verified
> 
> This is a semantic change in the error message, that should be notified to
> localizers by changing the entity names. CC'ing Pike.

Good point. Fine with me. Attaching patch.
Who can do a rubberstamp review?

Comment 100

[Axel Hecht [:Pike]]

Comment on attachment 269071 [details] [diff] [review]
Patch: Rename string ID nssFailure to nssFailure2 [checked in]

rubberstamp=me

Comment 101

[Kai Engert (:KaiE:)]

Comment on attachment 269071 [details] [diff] [review]
Patch: Rename string ID nssFailure to nssFailure2 [checked in]

Thanks, patch checked in

Comment 102

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Either bug 370875 needs to be fixed or the patch from this bug that caused it needs to be backed out.

Comment 103

[Kai Engert (:KaiE:)]

(In reply to comment #102)
> Either bug 370875 needs to be fixed or the patch from this bug that caused it
> needs to be backed out.

Hopefully fixed now.

Comment 104

[Kai Engert (:KaiE:)]

Attachment: Patch: one more place where nssFailure needs to be renamed [checked in]

This change is required on top of the earlier renaming of string IDs.
While neterror.xhtml appears to use other string IDs for the <div> identifiers, it actually uses JS to dynamically concatenate that ID.

Without this patch, the current error pages say "Oops... Detailed error message not available"

Comment 106

[Simon Lucy]

rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
On an expired client cert the error message is gnomic for most of the sentient breathing population of the planet.

Quote:
Secure Connection Failed
An error occurred during a connection to some.host.com.

SSL peer was unable to negotiate an acceptable set of security parameters.

(Error code: ssl_error_handshake_failure_alert)
The page you are trying to view can not be shown because the authenticity of the received data could not be verified.

    * Please contact the web site owners to inform them of this problem.

End Quote,

The error is even misleading since it implies that its the server side which is broken rather than the client cert.



        
        
      


      
      

Comment 107

[Jo Hermans]

See bug 419069 for more explanation - the fault doe not necessarily lie with the server, but the server is the only one that can explain *why* it has send back ssl_error_handshake_failure_alert.

Comment 108

[Kai Engert (:KaiE:)]

Simon, I think there is no need to copy information from other bugs into this bug.
If you'd like to broaden the audience, you can add people to the cc list.
I propose we discuss all details of that issue in bug 419069.

I think the error message from comment 105 correctly describes the situation, as explained in bug 419069. The server has a bug. The server does not care to send detailed failure information in the handshake. The client displays exactly what it has been told by the server. I don't see a client bug.

Comment 110

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

It's unclear what the state of this bug is. There appear to be checked in patches (and there don't appear to be non-obsolete non-checked-in patches), so I'm going to assume this is fixed. Any further work can be done in follow-up bugs. If this bug was intended to become a tracking bug, a new bug should be opened to track the remaining work (a bug that has checked-in patches also acting as a tracking bug is difficult to manage given our workflow).