Closed Bug 1245280 Opened 8 years ago Closed 8 years ago

don't fall back to subject common name for name information for new certificates

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(2 files)

The current implementation of certificate/hostname name matching in mozilla::pkix (implemented in bug 1063281) will fall back to using the subject common name in certificates if the subject alternative name extension isn't present or doesn't contain any dNSNames or iPAddresses. However, according to section 9.2.2 of the baseline requirements, "If present, this field [i.e. the subject CN] MUST contain a single IP address or Fully-Qualified Domain Name that is one of the values contained in the Certificate’s subjectAltName extension". (Also note that according to section 9.2.1, the subjectAltName extension is required.) Thus, falling back to the subject CN shouldn't be necessary, since any information there must also be in the subjectAltName extension. Furthermore, this will obviate many issues with using the subject CN for naming information (e.g. implicitly determining if the CN is intended to be a FQDN or IP address, dealing with multiple CNs, etc.).

However, to prevent considerable compatibility issues, we'll probably have to do a few things:
* Only enforce this for "new" certificates (i.e. certificates with a "notBefore" after a certain date)
* Only enforce this for certificates issued from roots in Mozilla's CA program
This is sort-of halfway between a feedback request and a review request. There are still a couple of open questions:
* exactly what date do we want to use as the cut-off? (the current date is near the release date for 48, I think. Kathleen has proposed using May 31, 2016)
* currently this is not enforced for imported roots. Should there be an option to enforce in all cases?
Assignee: nobody → dkeeler
(In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> * exactly what date do we want to use as the cut-off? (the current date is
> near the release date for 48, I think. Kathleen has proposed using May 31,
> 2016)

Hmm, a few questions I have:
 - Have we communicated with CAs that we intend to do this?
 - Do the BR_9_2_* numbers suggest we should be OK with doing this sooner rather than later?

I prefer earlier, but I get the feeling some CAs are only capable of moving at a glacial pace...

> * currently this is not enforced for imported roots. Should there be an
> option to enforce in all cases?

I guess it depends on why we're doing this?
 - If it's mainly to enforce BR compliance and reduce the chance of name matching issues for public certs, then I guess not.
 - If we want to eventually remove the fallback code altogether, then yes, of course.

If we do want to enforce this for imported roots though, we're probably going to have to implement at least a web console warning or something for quite a long time, unless we want people to come screaming at us when we break their private PKI or whatever. Might help to have telemetry on these non-public certs as well (no idea if this would pass privacy review though).
Comment on attachment 8732387 [details]
MozReview Request: bug 1245280 - add policy mechanism to optionally enforce BRs for falling back to subject CN r?Cykesiopka,mgoodwin

https://reviewboard.mozilla.org/r/37443/#review38075

I've only done a brief pass for feedback, but this looks good to me so far.

::: netwerk/base/security-prefs.js:48
(Diff revision 1)
>  pref("security.OCSP.require", false);
>  pref("security.OCSP.GET.enabled", false);
>  
>  pref("security.pki.cert_short_lifetime_in_days", 10);
>  
> +// security.pki.name_matching_policy controls how the platform matches

"security.pki.name_matching_policy" here and below don't match "security.pki.name_matching_mode" as used everywhere else.

::: netwerk/base/security-prefs.js:50
(Diff revision 1)
> +// 0: only use name information from the subject alternative name extension
> +// 1: fall back to the subject common name for certificates valid before 23
> +//    August 2016 if necessary (as in, if the subject alternative name
> +//    extension is either not present or does not contain any DNS names or IP
> +//    addresses)
> +// 2: always fall back to the subject common name if necessary

IIRC most of our existing prefs use 0 to represent "off" or whatever the least strict option is. Might make sense to do that here as well and swap the order.

::: security/certverifier/BRNameMatchingPolicy.h:7
(Diff revision 1)
> +#ifndef mozilla_psm__BRNameMatchingPolicy_h
> +#define mozilla_psm__BRNameMatchingPolicy_h

Nit: According to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices these should just be |BRNameMatchingPolicy_h|.

::: security/certverifier/BRNameMatchingPolicy.cpp:27
(Diff revision 1)
> +      fallBackToCommonName = AUGUST_23_2016 < notBefore
> +                           ? FallBackToSearchWithinSubject::No
> +                           : FallBackToSearchWithinSubject::Yes;

This feels like a Yoda comparison to me, but I guess this comparison is natural in the sense of chronological time. Either way is fine though.

::: security/certverifier/BRNameMatchingPolicy.cpp:35
(Diff revision 1)
> +      break;
> +    case Mode::DoNotEnforce:
> +      fallBackToCommonName = FallBackToSearchWithinSubject::Yes;
> +      break;
> +    default:
> +      MOZ_CRASH("Unexpected Mode");

I guess this case should've been caught earlier, but do we really need to crash for this?

::: security/manager/ssl/tests/unit/test_baseline_requirements_subject_common_name.js:14
(Diff revision 1)
> +
> +"use strict";
> +
> +do_get_profile();
> +
> +do_get_profile(); // must be called before getting nsIX509CertDB

Get ALL the profiles ;). This duplicate do_get_profile() call or the one above should be removed.

::: security/manager/ssl/tests/unit/test_baseline_requirements_subject_common_name.js:26
(Diff revision 1)
> +
> +function loadCertWithTrust(certName, trustString) {
> +  addCertFromFile(gCertDB, `test_baseline_requirements/${certName}.pem`, trustString);
> +}
> +
> +function checkCert(cert, expectedResult) {

Nit: Consider renaming this to "checkCertAtTime" or something.
At least in my case, I'm so used to checkCert() meaning "check the cert with time now" that I was initially confused how the test could possibly work with the validity periods of the test certs.
Attachment #8732387 - Flags: review?(cykesiopka.bmo)
Attachment #8732387 - Flags: feedback+
(In reply to :Cykesiopka from comment #3)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> > * exactly what date do we want to use as the cut-off? (the current date is
> > near the release date for 48, I think. Kathleen has proposed using May 31,
> > 2016)
> 
> Hmm, a few questions I have:
>  - Have we communicated with CAs that we intend to do this?

The communication is in progress and will go out soon. See https://groups.google.com/d/msg/mozilla.dev.security.policy/wVhRt63bTpU/nQl2ETtjAgAJ (Things for CAs to Fix)

>  - Do the BR_9_2_* numbers suggest we should be OK with doing this sooner
> rather than later?

Yes, the telemetry is pretty encouraging.

> I prefer earlier, but I get the feeling some CAs are only capable of moving
> at a glacial pace...
> 
> > * currently this is not enforced for imported roots. Should there be an
> > option to enforce in all cases?
> 
> I guess it depends on why we're doing this?
>  - If it's mainly to enforce BR compliance and reduce the chance of name
> matching issues for public certs, then I guess not.
>  - If we want to eventually remove the fallback code altogether, then yes,
> of course.

It would be nice to eventually remove it altogether, but I think it will be a long time until we can. (I guess we can keep gathering telemetry and see how things go.)

> If we do want to enforce this for imported roots though, we're probably
> going to have to implement at least a web console warning or something for
> quite a long time, unless we want people to come screaming at us when we
> break their private PKI or whatever. Might help to have telemetry on these
> non-public certs as well (no idea if this would pass privacy review though).

Let's start with public roots since we have the data and we're pretty sure we can make this change.
https://reviewboard.mozilla.org/r/37443/#review38075

Thanks for the feedback. I'll polish this up and submit for a real review soon.

> "security.pki.name_matching_policy" here and below don't match "security.pki.name_matching_mode" as used everywhere else.

Good catch.

> IIRC most of our existing prefs use 0 to represent "off" or whatever the least strict option is. Might make sense to do that here as well and swap the order.

Sounds good.

> Nit: According to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices these should just be |BRNameMatchingPolicy_h|.

Fixed.

> This feels like a Yoda comparison to me, but I guess this comparison is natural in the sense of chronological time. Either way is fine though.

Yeah, I think you're right here. I switched it.

> I guess this case should've been caught earlier, but do we really need to crash for this?

My reasoning here is that if we ever hit this, we've messed up the logic of going from the value in the pref to an enum choice. That's something that would be good to halt on so we realize it's happened.

> Get ALL the profiles ;). This duplicate do_get_profile() call or the one above should be removed.

Heh, yeah.

> Nit: Consider renaming this to "checkCertAtTime" or something.
> At least in my case, I'm so used to checkCert() meaning "check the cert with time now" that I was initially confused how the test could possibly work with the validity periods of the test certs.

I changed it to "checkCertOn25August2016" to be clear what it actually does.
Attachment #8732387 - Flags: feedback+ → review?(cykesiopka.bmo)
Comment on attachment 8732387 [details]
MozReview Request: bug 1245280 - add policy mechanism to optionally enforce BRs for falling back to subject CN r?Cykesiopka,mgoodwin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37443/diff/1-2/
Whiteboard: [psm-assigned]
Comment on attachment 8732387 [details]
MozReview Request: bug 1245280 - add policy mechanism to optionally enforce BRs for falling back to subject CN r?Cykesiopka,mgoodwin

https://reviewboard.mozilla.org/r/37443/#review39127

So something that I really should've noticed at the feedback stage (sorry!) is that technically the changes here aren't enforcing the BRs...
I'm going to withhold r+ for now to confirm that I'm not misunderstanding. The changes mostly look fine otherwise.

BRv1.3.3 7.1.4.2.2.a says:
> If present, this field MUST contain a single IP address or Fully Qualified
> Domain Name that is one of the values contained in the Certificate’s
> subjectAltName extension (see Section 7.1.4.2.1)
... and it doesn't look like we're enforcing this.

However, based on https://wiki.mozilla.org/SecurityEngineering/mozpkix-testing#Things_for_CAs_to_Fix entry number 9, it looks like that's not what we're trying to achieve anyways.
I guess we should just avoid saying we're enforcing the BRs and instead say we're avoiding falling back to the CN.

The references to the BRs should probably be removed to reflect this.

::: security/certverifier/CertVerifier.h:10
(Diff revision 2)
> +#include "BRNameMatchingPolicy.h"
>  #include "mozilla/Telemetry.h"
>  #include "pkix/pkixtypes.h"
>  #include "OCSPCache.h"
>  #include "ScopedNSSTypes.h"

Optional: Might want to sort these.

::: security/manager/ssl/SSLServerCertVerification.cpp:434
(Diff revision 2)
>                                         strlen(hostName));
>      if (result != Success) {
>        PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
>        return SECFailure;
>      }
> -    result = CheckCertHostname(certInput, hostnameInput);
> +    // Use a lax policy so as to not generate potentially spurrious name

Nit: Typo: s/spurrious/spurious/.

::: security/manager/ssl/tests/unit/test_baseline_requirements_subject_common_name.js:26
(Diff revision 2)
> +  addCertFromFile(gCertDB, `test_baseline_requirements/${certName}.pem`,
> +                  trustString);
> +}
> +
> +function checkCertOn25August2016(cert, expectedResult) {
> +  // (new Date("25 August 2016")).getTime() / 1000

Optional: Not that it really matters in this case, but the result of this expression depends on the system time zone.
Consider using ISO 8601 like you did before, just so people in different time zones don't get confused.

::: security/manager/ssl/tests/unit/test_baseline_requirements_subject_common_name.js:36
(Diff revision 2)
> +  // This is only enforced if the root is a built-in. In debug builds, we can
> +  // treat an imported root as a built-in.

Nit: Maybe something like "The pref is respected only if the root [...]"?
At least for me, it wasn't immediately clear what "This" referred to, so although I understood the meaning, reading the sentence felt a bit weird.

::: security/manager/ssl/tests/unit/test_baseline_requirements_subject_common_name.js:45
(Diff revision 2)
> +    checkCertOn25August2016(certFromFile("cn-contains-extra-entry-recent"),
> +                            PRErrorCodeSuccess);
> +    checkCertOn25August2016(certFromFile("cn-contains-extra-entry-old"),

Hmm, the names of these two test certs feel misleading since both only have one CN entry as the only source of naming information.
"no-san-recent" and "no-san-old" maybe?

::: security/pkix/lib/pkixnames.cpp:244
(Diff revision 2)
> +  }
> +
>    const Input* subjectAltName(cert.GetSubjectAltName());
>    Input subject(cert.GetSubject());
>  
>    // For backward compatibility with legacy certificates, we fall back to

Nit: Probably want to do s/we fall back/we may fall back/.
Attachment #8732387 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8732387 [details]
MozReview Request: bug 1245280 - add policy mechanism to optionally enforce BRs for falling back to subject CN r?Cykesiopka,mgoodwin

https://reviewboard.mozilla.org/r/37443/#review40089

Looks good to me.
Attachment #8732387 - Flags: review?(mgoodwin) → review+
Comment on attachment 8732387 [details]
MozReview Request: bug 1245280 - add policy mechanism to optionally enforce BRs for falling back to subject CN r?Cykesiopka,mgoodwin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37443/diff/2-3/
Attachment #8732387 - Flags: review?(cykesiopka.bmo)
https://reviewboard.mozilla.org/r/37443/#review39127

Good point. I added some documentation about what the rationale was and what's going on. Long story short is we're enforcing a consequence of this requirement without actually enforcing the requirement itself.

> Nit: Typo: s/spurrious/spurious/.

Good catch.

> Optional: Not that it really matters in this case, but the result of this expression depends on the system time zone.
> Consider using ISO 8601 like you did before, just so people in different time zones don't get confused.

Good idea.

> Nit: Maybe something like "The pref is respected only if the root [...]"?
> At least for me, it wasn't immediately clear what "This" referred to, so although I understood the meaning, reading the sentence felt a bit weird.

I reordered things and changed the documentation. Let me know what you think.

> Hmm, the names of these two test certs feel misleading since both only have one CN entry as the only source of naming information.
> "no-san-recent" and "no-san-old" maybe?

Sounds good. I also added some more tests.

> Nit: Probably want to do s/we fall back/we may fall back/.

Good idea.
Attachment #8732387 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8732387 [details]
MozReview Request: bug 1245280 - add policy mechanism to optionally enforce BRs for falling back to subject CN r?Cykesiopka,mgoodwin

https://reviewboard.mozilla.org/r/37443/#review40303

Looks good!

::: security/manager/ssl/tests/unit/test_baseline_requirements_subject_common_name.js:48
(Diff revision 3)
> +  // name matching will fall back to using the subject common name if necessary
> +  // (i.e. if there is no subject alternative name extension or it does not
> +  // contain any dNSName or iPAddress entries). Thus, since imported roots are
> +  // not in general treated as built-ins, these should all successfully verify
> +  // regardless of the value of the pref.
> +  Services.prefs.setIntPref("security.pki.name_matching_mode", 0);

Optional: Add some do_print() calls each time we test a specific mode so that if this test ever fails in automation, it's less annoying to find out which exact subtest failed.

::: security/manager/ssl/tests/unit/test_baseline_requirements_subject_common_name.js:118
(Diff revision 3)
> +    checkCertOn25August2016(certFromFile("san-contains-no-hostnames-recent"),
> +                            SSL_ERROR_BAD_CERT_DOMAIN);
> +    checkCertOn25August2016(certFromFile("san-contains-no-hostnames-old"),
> +                            SSL_ERROR_BAD_CERT_DOMAIN);
> +
> +    Services.prefs.clearUserPref("security.test.built_in_root_hash");

Very optional: Register a cleanup function to clear both prefs ("best practice").
Comment on attachment 8732387 [details]
MozReview Request: bug 1245280 - add policy mechanism to optionally enforce BRs for falling back to subject CN r?Cykesiopka,mgoodwin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37443/diff/3-4/
https://hg.mozilla.org/mozilla-central/rev/dc40f46fae48
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Drive by: We've been gathering metrics now for nearly 3 years as to when a certificate needs to exercise the "common name" fallback, both in the case of publicly trusted roots (aka BR compliant) and enterprise roots.

I don't have the numbers in front of me, but I was actually looking at it just a few weeks ago, and the numbers for enterprise roots are still within the "danger zone". I mention this because, much like the SHA-1, I think there's a real risk of breakage here if you're not segmenting out the two.

It may be worthwhile to limit this check to BR roots (if you haven't) and either gather telemetry or more actively communicate to enterprise (and AV vendors and proxy vendors) of this intent.
Apologies, I missed that you did capture that via the isBuiltin policy. I'll see if we can get https://crbug.com/308330 landed soon, so that Mozilla doesn't have to take the first mover penalty.

(Also, feel free to reach out on coordinating any BR-enforcement changes; we're strongly supportive of them, per https://crbug.com/102949 )
Depends on: 1261680
Thanks, Ryan. I'll be sure to step up the communication in the future.
I did a canary pass for this change on today's Nightly. In summary, 286 sites are broken of roughly 290k TLS-enabled sites, culled from the Alexa top 1 million site list.

More info here:
https://tlscanary.mozilla.org/runs/2016-04-04-10-56-25/index.htm

1. Go to tab labeled "Fields"
2. Select "site_info.rank"
3. Select "cert_info.commonName"
4. Select "cert_info.subjectAltName"
5. Go to tab labeled "Grid: 286 sites"
6. Sort column by "site_info.rank"
7. Note errors

All errors here seem consistent with this change. Certificates that have a commonName that is not listed in the subjectAltName field are rejected with error message "ssl_error_bad_cert_domain."
That's expected, and is due to the CA violating Mozilla's program requirements. See Bug 1261919
Also Bug 1261680
(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #20)
> I did a canary pass for this change on today's Nightly. In summary, 286
> sites are broken of roughly 290k TLS-enabled sites, culled from the Alexa
> top 1 million site list.
> 
> More info here:
> https://tlscanary.mozilla.org/runs/2016-04-04-10-56-25/index.htm

Matt, Thank you for doing this. 

I noticed that many of the certs are old, but (unfortunately) some are not so old. We need to identify the CAs that are still issuing this type of cert. We don't need to fail for certs that were issued a long time ago.

David, can you update the code to only fail after a date like August 1, 2015, so we can identify the newer certs that are still not following the BRs?
Attached file canary.csv
I filtered the canary list for certificates with a notBefore after 1 August 2015. I'm also working on identifying these CAs through certificate transparency (which is where I got the information to file bug 1262610).
Depends on: 1267026
I am sorry to post to this list in a sarcastic manner, I understand that there a lot of people putting a lot of hard work into this but:

I have a really cool idea.

Instead of just SSL_ERROR_BAD_CERT_DOMAIN...and including a link to basic SSL issues...

Could you state really what the reason is?  I spent at least 20 hours figuring out why Firefox 50 was broken, while everything else worked.

I guess I see here from this bug:  "However, according to section 9.2.2 of the baseline requirements, "If present, this field [i.e. the subject CN] MUST contain a single IP address or Fully-Qualified Domain Name that is one of the values contained in the Certificate’s subjectAltName extension". (Also note that according to section 9.2.1, the subjectAltName extension is required.) "

I found more here:  https://www.godaddy.com/help/can-i-request-a-certificate-for-an-intranet-name-or-ip-address-6935

And here:  https://www.digicert.com/internal-names.htm

And finally here:  https://cabforum.org/wp-content/uploads/Guidance-Deprecated-Internal-Names.pdf

I guess I get it, but why does adding an alternative name with the internal hostname work?  I understand the need for this removal from public CA's but I would trust internal PKI vs another company signing my stuff for some things.
I forgot to link this bug also:  https://bugzilla.mozilla.org/show_bug.cgi?id=1324096
I don't fully understand the cause for concern, but I presume that it is legitimate. Is it something like if someone could change their legal name to bugzilla.mozilla.org and ask for a certificate, they should be granted it, whereas they couldn't get an x509 cert to say "DNS:bugzilla.mozilla.org" in the SubjectAltName? Practically speaking I've never heard of such an attack, but I can imagine there may have been, and there is always the principle of the thing to consider. 

I humbly ask that you take a lesson from the Chrome 58 fiasco playing out in intranets across America this week (mine can't be the only one), and ask that Firefox at least goes for one or more release cycles where these sites get some kind of less-than-green "Not so secure" icon, with an informative mouse-over text, while still actually operating. That is, unless that would not be accurate, in which case, why bother making the change at all?
Jack, are you seeing errors related to this in Firefox for certificates issued by an internal CA? If so, please open a new bug with as much detail as you can share: https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Security%3A%20PSM
Flags: needinfo?(jack)
I misread the bug, I'm sorry. I thought this was a pending change comparable to one in chrome that just happened.
Flags: needinfo?(jack)
You need to log in before you can comment on or make changes to this bug.