The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 15

Status

()

Core
Security: PSM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

10 Branch
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 unaffected, firefox14- wontfix, firefox15+ fixed, firefox-esr10 unaffected, status1.9.2 unaffected)

Details

(Whiteboard: [aurora: 2 (or 3) patches])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Depends on: 738458
(Assignee)

Comment 1

5 years ago
Created attachment 608474 [details] [diff] [review]
Patch v1
Assignee: nobody → kaie
Attachment #608474 - Flags: review?(rrelyea)
(Assignee)

Comment 2

5 years ago
FWIW, the code here was automatically produced using the utility attached to bug 329017.
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);
}
Created attachment 608879 [details] [diff] [review]
[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.
Attachment #608879 - Flags: review?(kaie)

Comment 5

5 years ago
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.
Attachment #608879 - Flags: review+
(Assignee)

Comment 6

5 years ago
Comment on attachment 608474 [details] [diff] [review]
Patch v1

postponing review request - error code is not yet final
Attachment #608474 - Flags: review?(rrelyea)
(Assignee)

Comment 7

5 years ago
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.
Attachment #608879 - Flags: review?(kaie) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 610106 [details] [diff] [review]
add latest localizable error strings [tentative, depends on 3.13.4]
Attachment #608474 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
Created attachment 610115 [details] [diff] [review]
[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.
Attachment #610115 - Flags: review?(wtc)
(Assignee)

Updated

5 years ago
Attachment #610106 - Attachment description: tentative patch v4 → add latest localizable error strings [tentative, depends on 3.13.4]

Comment 11

5 years ago
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;
Attachment #610115 - Flags: review?(wtc) → review+

Comment 12

5 years ago
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".
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
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
Summary: Show a useful error message for sites using MD5 certificates (and add missing NSS error codes until 3.13.4 to PSM) → Show a useful error message for sites using MD5 certificates (and add missing NSS error codes until 3.13.5 to PSM)
(Assignee)

Updated

5 years ago
tracking-firefox14: --- → ?
(Assignee)

Comment 15

5 years ago
Created attachment 617716 [details] [diff] [review]
[c] Include new error strings until NSS 3.13.5
Attachment #610106 - Attachment is obsolete: true
Attachment #617716 - Flags: review?(bsmith)
(Assignee)

Updated

5 years ago
Whiteboard: [checkin 3 patches]
(Assignee)

Comment 16

5 years ago
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).
Attachment #617716 - Flags: review?(honzab.moz)
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.
Attachment #617716 - Flags: review?(honzab.moz)
Attachment #617716 - Flags: review?(bsmith)
Attachment #617716 - Flags: review+
Attachment #617716 - Flags: approval-mozilla-aurora?
status1.9.2: --- → unaffected
status-firefox-esr10: --- → unaffected
status-firefox12: --- → unaffected
status-firefox13: --- → unaffected
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox15: --- → ?
(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?
(Assignee)

Updated

5 years ago
Attachment #608879 - Attachment description: Simplify nsNSSErrors and improve signatures → [a] Simplify nsNSSErrors and improve signatures
(Assignee)

Updated

5 years ago
Attachment #610115 - Attachment description: patch: fallback to NSS' internal strings → [b] patch: fallback to NSS' internal strings
(Assignee)

Updated

5 years ago
Attachment #617716 - Attachment description: Include new error strings until NSS 3.13.5 → [c] Include new error strings until NSS 3.13.5
(Assignee)

Comment 19

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #608879 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #610115 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 20

5 years ago
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
(Assignee)

Updated

5 years ago
Whiteboard: [checkin 3 patches] → [aurora: 2 (or 3) patches]
https://hg.mozilla.org/mozilla-central/rev/5b8f9effd8ca
https://hg.mozilla.org/mozilla-central/rev/b5caf71c96d0
https://hg.mozilla.org/mozilla-central/rev/bb2d256dcfac
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
status-firefox15: affected → fixed
tracking-firefox15: ? → +
status-firefox12: unaffected → ---
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.
Attachment #608879 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #608879 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-

Updated

5 years ago
Attachment #610115 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

5 years ago
Attachment #617716 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
status-firefox14: affected → wontfix
tracking-firefox14: ? → -

Updated

5 years ago
Blocks: 758314

Comment 23

5 years ago
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.)
You need to log in before you can comment on or make changes to this bug.