1261936 - stop using the subject common name in certificate verification error messages

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

As of bug 1245280, Firefox doesn't use the subject common name for name information (on Nightly - on release this will happen for new certificates (issued after August 23, 2016)). The error messages as provided by PSM in the case of hostname mismatches shouldn't do so either.

Comment 1

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

Attachment: MozReview Request: bug 1261936 - stop using the subject common name in certificate verification error messages r?Cykesiopka

Review commit: https://reviewboard.mozilla.org/r/44395/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44395/

Comment 2

[:Cykesiopka]

Comment on attachment 8738259 [details]
MozReview Request: bug 1261936 - stop using the subject common name in certificate verification error messages r?Cykesiopka

https://reviewboard.mozilla.org/r/44395/#review41257

LGTM.

::: security/manager/ssl/TransportSecurityInfo.cpp:712
(Diff revision 1)
> -                        nsIX509Cert* ix509,
> -                        nsINSSComponent *component,
> -                        bool wantsHtml,
> -                        nsString &returnedMessage)
> -{
> -  const char16_t *params[1];
> +                        nsINSSComponent* component, bool wantsHtml,
> +                        nsString& returnedMessage)
> +{
> +  // Prepare a default "not valid for <hostname>" string in case anything
> +  // goes wrong (or in case the certificate is not valid for any hostnames).
> +  nsString notValidForHostnameString;

Nit: I believe nsAutoString is preferred for stack strings because of the internal 64 char buffer.
(Then again, the base value of "certErrorMismatch" is already 42 chars, so maybe it doesn't matter.)

::: security/manager/ssl/TransportSecurityInfo.cpp:745
(Diff revision 1)
> -    const char *stringID;
> -    if (wantsHtml)
> +    const char* stringID;
> +    if (wantsHtml) {
>        stringID = "certErrorMismatchSingle2";
> -    else
> +    } else {
>        stringID = "certErrorMismatchSinglePlain";
> +    }

Nit: Ternary if might look nicer here.

::: security/manager/ssl/tests/unit/head_psm.js:645
(Diff revision 1)
>  
>  // Utility functions for adding tests relating to certificate error overrides
>  
>  // Helper function for add_cert_override_test. Probably doesn't need to be
>  // called directly.
> -function add_cert_override(aHost, aExpectedBits, aSecurityInfo) {
> +function add_cert_override(aHost, aExpectedBits, aExpectedErrorRegexp,

Optional: Maybe do |aExpectedErrorRegexp = null| or |aExpectedErrorRegexp = undefined| here to make it clearer that aExpectedErrorRegexp is optional?

::: security/manager/ssl/tests/unit/head_psm.js:647
(Diff revision 1)
>  
>  // Helper function for add_cert_override_test. Probably doesn't need to be
>  // called directly.
> -function add_cert_override(aHost, aExpectedBits, aSecurityInfo) {
> +function add_cert_override(aHost, aExpectedBits, aExpectedErrorRegexp,
> +                           aSecurityInfo) {
> +  aSecurityInfo.QueryInterface(Ci.nsITransportSecurityInfo);

Is this line necessary?

Comment 3

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

https://reviewboard.mozilla.org/r/44395/#review41257

Thanks!

> Nit: I believe nsAutoString is preferred for stack strings because of the internal 64 char buffer.
> (Then again, the base value of "certErrorMismatch" is already 42 chars, so maybe it doesn't matter.)

Ok - hostnames are sometimes short.

> Nit: Ternary if might look nicer here.

Agreed.

> Optional: Maybe do |aExpectedErrorRegexp = null| or |aExpectedErrorRegexp = undefined| here to make it clearer that aExpectedErrorRegexp is optional?

Sounds good.

> Is this line necessary?

Apparently not.

Comment 4

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

Comment on attachment 8738259 [details]
MozReview Request: bug 1261936 - stop using the subject common name in certificate verification error messages r?Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44395/diff/1-2/

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=859db8016ab1

Comment 6

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

Comment on attachment 8738259 [details]
MozReview Request: bug 1261936 - stop using the subject common name in certificate verification error messages r?Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44395/diff/2-3/

Comment 7

[Pulsebot]

https://hg.mozilla.org/mozilla-central/rev/111872969899

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/111872969899