738457 - Show a useful error message for sites using MD5 certificates (and add missing NSS error codes until 3.13.5 to PSM)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert (:KaiE:)]

Show a useful error message for sites using MD5 certificates (and add missing NSS error codes until 3.13.4 to PSM)

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 2

[Kai Engert (:KaiE:)]

FWIW, the code here was automatically produced using the utility attached to bug 329017.

Comment 3

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

What is the difference between what getDefaultErrorStringName returns and what PR_ErrorToName returns? i.e. Why not convert the function to use PR_ErrorToName, like so?:

const char *
nsNSSErrors::getDefaultErrorStringName(PRErrorCode err)
{
  return PR_ErrorToName(err);
}

Comment 4

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

Attachment: [a] Simplify nsNSSErrors and improve signatures

Kai, I already had a patch in my tree that simplifies nsNSSErrors, but I never got around to verifying whether it was correct. (See the previous comment.) Perhaps this patch would be useful to you.

Comment 5

[Wan-Teh Chang]

Comment on attachment 608879 [details] [diff] [review]
[a] Simplify nsNSSErrors and improve signatures

r=wtc.  I think this patch should work.  It has two changes:
1. Use PRErrorCode instead of PRInt32 to represent error codes.
2. Use the PR_ErrorToName function, which recently started to
   support NSS error codes thanks to Elio.

Comment 6

[Kai Engert (:KaiE:)]

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

postponing review request - error code is not yet final

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 608879 [details] [diff] [review]
[a] Simplify nsNSSErrors and improve signatures

Thanks, this patch is useful, and I tested that the mapping from error codes to error names works, together with the md5 bug.

However, this patch is not sufficient for fixing this bug, because we don't use PR_ErrorToString outside of nspr/nss.

I'll attach an additional shorter patch, which adds the full error strings to the .properties file - after code and wording in bug 738454 are completed.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: add latest localizable error strings [tentative, depends on 3.13.4]

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 608879 [details] [diff] [review]
[a] Simplify nsNSSErrors and improve signatures

This patch also has the benefit that any error codes from NSS will be displayed with their error code name, even if the full description is not available in PSM. Maybe the other patch should be enhanced, so that the NSS builtin error string is shown, if PSM doesn't have a localized string yet.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: [b] patch: fallback to NSS' internal strings

> Maybe the other patch should be enhanced, so that the NSS
> builtin error string is shown, if PSM doesn't have a localized string yet.

Here's an additional, third patch, to implement the fallback.

Comment 11

[Wan-Teh Chang]

Comment on attachment 610115 [details] [diff] [review]
[b] patch: fallback to NSS' internal strings

Review of attachment 610115 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.  I suggest a change.

::: security/manager/ssl/src/nsNSSErrors.cpp
@@ +106,5 @@
> +    returnedMessage.AppendASCII(PR_ErrorToString(err, PR_LANGUAGE_EN));
> +    returnedMessage.Append(NS_LITERAL_STRING("\n"));
> +  }
> +  
> +  if (nss_error_id_str)

Why don't you test if (id_str || nss_error_id_str)
here to match the test on line 83 above?

Actually, I think we can do everything inside the
original if (id_str || nss_error_id_str) block, as
follows:

  if (id_str || nss_error_id_str)
  {
    nsString defMsg;
    nsresult rv;
    ...
    if (NS_SUCCEEDED(rv))
    {
      returnedMessage.Append(defMsg);
    }
    else
    {
      // no localized string available, use NSS' internal
      returnedMessage.AppendASCII(PR_ErrorToString(err, PR_LANGUAGE_EN));
    }
    returnedMessage.Append(NS_LITERAL_STRING("\n"));

    nsCString error_id(nss_error_id_str);
    ...
  }

  return NS_OK;

Comment 12

[Wan-Teh Chang]

Comment on attachment 610115 [details] [diff] [review]
[b] patch: fallback to NSS' internal strings

"use NSS' internal" sounds incomplete.
I suggest "use NSS's internal strings".

Microsoft Manual of Style recommends "use the internal strings of NSS".

Comment 13

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #11)
> 
> Why don't you test if (id_str || nss_error_id_str)
> here to match the test on line 83 above?

Because id_str is never used inside that second block. That second block assumes that nss_error_id_str is set.


> Actually, I think we can do everything inside the
> original if (id_str || nss_error_id_str) block, as
> follows:


No, that won't work.

My proposal uses 3 steps:
- override defined in PSM? use it
- localized string available in PSM? use it
- fall back to the english string provided by NSS

Your proposal from comment 11 does this:
- override defined in PSM? use it
- fall back to the english string provided by NSS


Maybe the meaning of "override" in the current code isn't clear?
Override means, the Mozilla people don't like the default string provided by NSS, but rather defined their own wording.

You might ask, then why does PSM include those undesired error strings at all, and doesn't simply replace them in their internal table? The reason is "ease of use" and "human error". I want to table of NSS error strings in PSM to be complete. This way we can always automatically create the string table from NSS, without risk that we accidentally replace a deliberate override...


This means, we have two tables in PSM:
- the complete table with all strings from NSS
- a very small table with overrides/replacement wordings.


The proposal from comment 11 wouldn't use the localized NSS strings that are located in PSM's .properties files. We must fetch them from the string bundle. Only if that fails, because a NSS error code is very new and PSM's copy of error strings hasn't been updated yet (and therefore hasn't been localized yet), only then fall back to the english strings builtin in NSS.

Comment 14

[Kai Engert (:KaiE:)]

As there will probably be an additional update 3.13.4, which will not yet cover the md5 related work, I'm updating the summary to refer to 3.13.5

Comment 15

[Kai Engert (:KaiE:)]

Attachment: [c] Include new error strings until NSS 3.13.5

Comment 16

[Kai Engert (:KaiE:)]

Comment on attachment 617716 [details] [diff] [review]
[c] Include new error strings until NSS 3.13.5

This patch is simply adding strings (copied from NSS, so that they can be localized).

Comment 17

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

Comment on attachment 617716 [details] [diff] [review]
[c] Include new error strings until NSS 3.13.5

[Approval Request Comment]
Regression caused by (bug #): 650355

User impact if declined: Without this change, users will get a very confusing and unhelpful error message when they encounter a certificate with an MD5-based signature, which is speculated (but not verified) to be very rare. With this change, users will get an error message that might be a little more helpful.

Testing completed (on m-c, etc.): Should land on mozilla-inbound today.

Risk to taking this patch (and alternatives if risky): More work for localizers, string changes they probably didn't expect.

String changes made by this patch: See the patch. The patch is all string changes. This is why it is important to land it in Aurora right away, unless we revert the patch in bug 650355.

Comment 18

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

(In reply to Brian Smith (:bsmith) from comment #17)
> User impact if declined: Without this change, users will get a very
> confusing and unhelpful error message when they encounter a certificate with
> an MD5-based signature, which is speculated (but not verified) to be very
> rare. With this change, users will get an error message that might be a
> little more helpful.

Actually, Kai, is this true? Or, will they just get the English-language, non-localized error message from NSS?

Would we also need to land the first two patches on Aurora?

Comment 19

[Kai Engert (:KaiE:)]

mozilla-central
===============
I will check in all patches [a] + [b] + [c]


Aurora/14
=========
Patches [a] plus [b] (together) are mandatory,
they will get us the english error messages on all locales.

Patches [a] plus [b] (together) are sufficient as a minimal fix

In addition, patch [c] is nice to have, would give us the ability
to have localized versions of the new error messages.

Comment 20

[Kai Engert (:KaiE:)]

Landed into inbound.

[a]
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8f9effd8ca

[b]
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5caf71c96d0

[c]
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2d256dcfac

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/5b8f9effd8ca
https://hg.mozilla.org/mozilla-central/rev/b5caf71c96d0
https://hg.mozilla.org/mozilla-central/rev/bb2d256dcfac

Comment 22

[Alex Keybl [:akeybl]]

Comment on attachment 608879 [details] [diff] [review]
[a] Simplify nsNSSErrors and improve signatures

[Triage Comment]
See https://bugzilla.mozilla.org/show_bug.cgi?id=650355#c26. Since we're not yet ready to disable MD5 signed certificates, we'll wait until FF15 to take these changes.

Comment 23

[Wan-Teh Chang]

Comment on attachment 617716 [details] [diff] [review]
[c] Include new error strings until NSS 3.13.5

>+SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED=The certificate was signed using an signature algorithm that is disabled because it is not secure.

Kai: please fix the typo "an signature" => "a signature"
in this error message.  (Daniel Cater pointed this out in
bug 738454 comment 28.)