Last Comment Bug 738457 - Show a useful error message for sites using MD5 certificates (and add missing NSS error codes until 3.13.5 to PSM)
: Show a useful error message for sites using MD5 certificates (and add missing...
Status: RESOLVED FIXED
[aurora: 2 (or 3) patches]
:
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: 10 Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: Kai Engert (:kaie)
:
:
Mentors:
Depends on: 738454 738458
Blocks: 758314
  Show dependency treegraph
 
Reported: 2012-03-22 14:10 PDT by Kai Engert (:kaie)
Modified: 2012-05-24 17:16 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
-
wontfix
+
fixed
unaffected
unaffected


Attachments
Patch v1 (11.06 KB, patch)
2012-03-22 14:19 PDT, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
[a] Simplify nsNSSErrors and improve signatures (27.85 KB, patch)
2012-03-23 14:58 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
kaie: review+
wtc: review+
akeybl: approval‑mozilla‑aurora-
Details | Diff | Splinter Review
add latest localizable error strings [tentative, depends on 3.13.4] (5.75 KB, patch)
2012-03-28 06:15 PDT, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
[b] patch: fallback to NSS' internal strings (2.10 KB, patch)
2012-03-28 06:42 PDT, Kai Engert (:kaie)
wtc: review+
akeybl: approval‑mozilla‑aurora-
Details | Diff | Splinter Review
[c] Include new error strings until NSS 3.13.5 (6.75 KB, patch)
2012-04-23 17:48 PDT, Kai Engert (:kaie)
brian: review+
akeybl: approval‑mozilla‑aurora-
Details | Diff | Splinter Review

Description Kai Engert (:kaie) 2012-03-22 14:10:35 PDT
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) 2012-03-22 14:19:56 PDT
Created attachment 608474 [details] [diff] [review]
Patch v1
Comment 2 Kai Engert (:kaie) 2012-03-22 14:33:03 PDT
FWIW, the code here was automatically produced using the utility attached to bug 329017.
Comment 3 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-22 21:07:55 PDT
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?) 2012-03-23 14:58:41 PDT
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.
Comment 5 Wan-Teh Chang 2012-03-23 15:07:42 PDT
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) 2012-03-28 03:41:59 PDT
Comment on attachment 608474 [details] [diff] [review]
Patch v1

postponing review request - error code is not yet final
Comment 7 Kai Engert (:kaie) 2012-03-28 06:13:11 PDT
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) 2012-03-28 06:15:47 PDT
Created attachment 610106 [details] [diff] [review]
add latest localizable error strings [tentative, depends on 3.13.4]
Comment 9 Kai Engert (:kaie) 2012-03-28 06:17:48 PDT
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) 2012-03-28 06:42:14 PDT
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.
Comment 11 Wan-Teh Chang 2012-03-28 13:48:10 PDT
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 2012-03-28 13:51:08 PDT
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) 2012-03-29 09:31:16 PDT
(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) 2012-04-02 14:40:48 PDT
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) 2012-04-23 17:48:36 PDT
Created attachment 617716 [details] [diff] [review]
[c] Include new error strings until NSS 3.13.5
Comment 16 Kai Engert (:kaie) 2012-04-26 06:31:11 PDT
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?) 2012-04-26 11:54:45 PDT
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?) 2012-04-26 11:58:06 PDT
(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) 2012-04-26 16:40:55 PDT
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 22 Alex Keybl [:akeybl] 2012-04-30 15:47:16 PDT
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 2012-05-24 13:23:01 PDT
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.)

Note You need to log in before you can comment on or make changes to this bug.