355643 - Review PSM's error message overrides - are NSS errors better?

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Kai Engert (:KaiE:)]

See bug 107491, patch v5 and onward.

We are still using the strings explicitly defined by the PSM application code, and override the NSS error messages in several cases.

We should review in which cases the NSS error messages should be prefered.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: comparison

In this file I listed all overrides currently made by PSM:
- the error codes that have an error message defined by PSM
- the message used by PSM
- what NSS error message are thereby "hidden"

I did not attempt to clean that file up, but it should be easy to read anyway.

Let me explain what this example section means:

    case SSL_ERROR_BAD_MAC_READ:
    case SSL_ERROR_BAD_MAC_ALERT:
+SSL_ERROR_BAD_MAC_READ=SSL received a record with an incorrect Message Authentication Code.
+SSL_ERROR_BAD_MAC_ALERT=SSL peer reports incorrect Message Authentication Code.
      id_str = "PSMERR_BadMac";
+PSMERR_BadMac=Received a message with incorrect Message Authentication Code. If the error occurs frequently, contact the site administrator.
      break;

For error codes SSL_ERROR_BAD_MAC_READ and SSL_ERROR_BAD_MAC_ALERT
PSM defines its own error message, which is listed as
PRMERR_BadMac.

The messages that NSS provides for those error codes, but which are hidden by the PSM override, are also shown.


The attached text file has many such sections.

We should individually review whether these overrides make sense and possibly remove some of the overrides.


At the end of the file I listed all the error codes which currently do not have a PSM override.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Kai, thanks for producing this list.  It clearly documents many cases 
where meaningful NSS error codes are overridden by comparitively 
meaninless error messages.  

One question: What message does PSM display today for all those NSS
error codes listed at the end of that file, the ones that have no
"override" error string?  Nothing?  "for an unknown reason?"

Comment 3

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

can NSS's error messages be translated?

Comment 4

[Kai Engert (:KaiE:)]

(In reply to comment #3)
> can NSS's error messages be translated?

Yes, please have a look at the patch in bug 107491.
It adds the NSS error messages to a properties file (which was produced with a helper tool attached to bug 329017)

Comment 5

[Kai Engert (:KaiE:)]

(In reply to comment #2)
> One question: What message does PSM display today for all those NSS
> error codes listed at the end of that file, the ones that have no
> "override" error string?  Nothing?  "for an unknown reason?"

It currently displays:
  Error establishing an encrypted connection to %S. Error Code: %S.

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 241417 [details]
comparison

Nelson, can you please review this comparison and make decisions which of each you like better? See the comments in the bug on how to read this file. Thanks

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Kai, ever since you wrote bug 107491 comment 28, and filed bug 329017, 
and especially since the discussions that you and I had last October/November surrounding the mozilla "summit" meetings, (shortly after this bug was filed, 
IINM) it has been my understanding that you were planning to implement a new error scheme that always reported a very specific error string, spelling out 
the error in enough detail for web site administrators (or "help desks") to 
understand the problem.  It was intended to be specific enough that it 
obviated further investigation by NSS and PSM developers to answer the usual 
question "what's wrong here?".  

The recent incident with the Microsoft QA engineer (someone clueful about
SSL) who could not understand what FireFox was complaining about, not even 
with a trunk FF build, is a perfect illustration of this need!

It might also include a generic "dumbed down" error string, since certain 
people seem to think that's helpful and necessary UI practice, and that's OK 
as long as it ALSO contains the helpful details.

I'm sure you sent me (in email, or as comments in some bug) proposals and
mock-ups for this, although I am having a difficult time finding them just
now.  One of them (more recent than that) is seen in bug 107491 comment 76.
It says (in part):

>   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, [English speeakers] might be unable to translate.

My understanding of those proposals has been that that "exact explanation"
string would be a localized version of NSS's error string for that error.

One of the main objectives of bug 107491 was to overcome the problem we
have today (a legacy of a past PSM developer and his manager, which you
acknowledged in bug 107491 comment 28) which caused entire groups of specific 
detailed NSS error strings to each be replaced with single "dumbed down" 
error strings (typically "unknown reason").  

As documented in your first attachment to this bug, in once case, 29 different 
specific NSS error codes are replaced with a single message that says only
"The other side has sent an incorrect or unexpected message."  In another
case, 15 specific messages about specific certificate errors (which might 
help a server admin fix the problem) were replaced with a single generic 
string that says "the site uses a certificate that is invalid or corrupted."
and is generally worthless as a diagnostic aid.

And who knows how many errors (I'd guess HUNDREDS) are replaced with 
"for unknown reason", or output an error number in decimal (or worse: hex!).

One good thing PSM did was to make it possible to inject site names into 
some error messages, so that generic phrases the "the peer" could be replaced 
by the DNS name from the URL.  That's a good idea, I think.  And likewise, 
it might be a good idea to be able to replace the string "peer" with 
"web site" or "remote server" or "email correspondent" depending on the 
context in which the error is being used (if that is known by the code 
issuing the error).   But the lines quoted in the "comparison" attachment
don't show any of that, so clearly that is not a reason to keep any of the 
PSM messages shown in the comparison.

In bug 107491 comment 28, you wrote:
> I don't feel like looking at each and every [PSM] error code and discuss 
> whether the error message is appropriate.
And I agreed with that sentiment very much, yet that's what you're proposing 
we do in this bug.  (Is it not?)

So, please, going forward, let's start from the position that all cases 
where PSM replaced MULTIPLE NSS error strings with a single PSM error strings
were mistakes, and eliminate them all.  

I am willing to review the cases where PSM replaced one NSS string with one 
string of its own.   If we find other single NSS strings that need to be 
better stated, we can fix them as we find them.  (I know of one string 
already that is a candidate: SEC_ERROR_BAD_DATABASE ;)  But I suggest that
we start from the position that we're going to discard ALL of PSM's old 
error strings, and use NSS's error strings instead, except in those few 
cases that we subsequently decide that NSS's error string is not good enough.

I invite discussion.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 241417 [details]
comparison

I looked at the cases of 1-for-1 error message replacement for the 
SEC_ERROR_CRL_* errors.  Most were OK.  some were not.

SEC_ERROR_CRL_INVALID 
NSS message: New CRL has an invalid format.  
(new?)
PSM message: Certificate Revocation List (CRL) from the CA certifying the site is not valid.
This is too broad.  "is not valid" could mean expired or bad signature, yet
there are separate errors for those.  Needs to say something about invalid format or syntax.  

PSM's OCSP error strings seem to be collectively missing any mention of 
the fact that the SERVER has said it's bad, not us.  For example:

SEC_ERROR_OCSP_UNAUTHORIZED_REQUEST
NSS says: The OCSP server has refused this request as unauthorized.
that's exactly right.
PSM says: Error trying to validate certificate using OCSP - unauthorized request.
The missing piece here is that the server said it's unauthorized.  This is
not a judgement that NSS is making but one that the server has made.  
The error string should be clear about that.  If people have a complaint
they should take it up with the server, not us.

SEC_ERROR_OCSP_UNKNOWN_CERT=
NSS says: The OCSP server has no status for the certificate.
IOW, the server said "Beats me!  I dunno!"
PSM says: 
"Error trying to validate certificate using OCSP - unknown certificate."
See, this sounds like NSS is saying the cert is unknown, but the server
said it.  The message MUST be clear about who made the judgement, especially
when it is not NSS.

SEC_ERROR_REVOKED_CERTIFICATE=
NSS Says: Peer's Certificate has been revoked.
PSM Says: Could not establish an encrypted connection because the site uses a certificate that has been revoked.
That's great for SSL, but what if this error occurs while validating a 
signature on a signed email.  The error message will make it sound like a
mail server problem!

All PSM's messages that begin with: "Could not establish an encrypted connection because the site uses a certificate that " have this problem.

There are a bunch of SSL errors for which a pair of NSS errors are replaceed
with a single PSM error message.  One of the two error codes ends in the 
word _ALERT.  In ALL these cases, the problem here is that the message loses
the information about which side detected the error.  Did we detect the 
error, or did the peer detect the error and tell us about it?  This is an
important distinction.  _ALERT means the other side detected it and reported 
it to us.

Guess what's wrong with this message:
The current transfer has been cancelled.
It begs the question: BY WHOM?

Now a word about my favorite bad NSS error message, which PSM does NOT 
attempt to fix today.

SEC_ERROR_BAD_DATABASE means: one of two things happened:
1) NSS searcheed for a database record but didn't find it, e.g. RECORD_NOT_FOUND.
2) NSS tried to add a new record to the database, but there was already a 
record with that same database "key" index value, so the add failed because
it would have created a duplicate database index value.  ATTEMPT_TO_CREATE_DUPLICATE  

I wouldn't call either of these things a "bad database", although a bad 
database can cause records to not be found even when they exist.  But 
the usual cause of this error is simply that the record isn't there,
not that the database is defective in any way.

Comment 9

[Kai Engert (:KaiE:)]

I finally got to this.

Nelson, thanks a lot for your review.

I walked through all the comments, and I ended up removing nearly all of the overrides.

I had 6 overrides remaining, 3 of them being related to CRLs. I compared the wording and decided that PSM's wording doesn't add value, so I removed them, too.

I will attach a patch that removes all but 3 overrides. Those which remain are:
    case SSL_ERROR_SSL_DISABLED:
      id_str = "PSMERR_SSL_Disabled";
      break;
    case SSL_ERROR_SSL2_DISABLED:
      id_str = "PSMERR_SSL2_Disabled";
      break;
    case SEC_ERROR_REUSED_ISSUER_AND_SERIAL:
      id_str = "PSMERR_HostReusedIssuerSerial";
      break;

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 268172 [details] [diff] [review]
Patch v1

I gather that somewhere there must be code (not seen in this patch) that uses the NSS error string. 
Assuming that is true: r=nelson

Comment 12

[Kai Engert (:KaiE:)]

(In reply to comment #11)
> I gather that somewhere there must be code (not seen in this patch) that uses
> the NSS error string. 

Yes. If no override is present for a code, PSM will display the string provided by NSS.

Comment 13

[Kai Engert (:KaiE:)]

Checked in, marking fixed.