Closed Bug 1050546 Opened 6 years ago Closed 5 years ago

telemetry for baseline requirements compliance: sections 9.2.1: subject alternative name extension and 9.2.2: subject common name field

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file, 2 obsolete files)

We need to measure baseline requirements compliance for certificates issued by authorities in our root program. This bug will handle sections 9.2.1: subject alternative name extension and 9.2.2: subject common name field.

In particular:

9.2.1 Subject Alternative Name Extension
Certificate Field: extensions:subjectAltName
Required/Optional: Required
Contents: This extension MUST contain at least one entry.
Each entry MUST be either a dNSName containing the Fully-Qualified Domain Name or an iPAddress containing the IP address of a server. ...

Wildcard FQDNs are permitted.

... Also as of the Effective Date, the CA SHALL NOT issue a certificate with an Expiry Date later than 1 November 2015 with a subjectAlternativeName extension or Subject commonName field containing a Reserved IP Address or Internal Server Name. Effective 1 October 2016, CAs SHALL revoke all unexpired Certificates whose subjectAlternativeName extension or Subject commonName field contains a Reserved IP Address or Internal Server Name.

9.2.2 Subject Common Name Field
Certificate Field: subject:commonName (OID 2.5.4.3)
Required/Optional: Deprecated (Discouraged, but not prohibited)
Contents:
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 9.2.1).
Attached patch patch (obsolete) — Splinter Review
I put this telemetry in AuthCertificate, but I suppose it could go in NSSCertDBTrustDomain if you think that's the right place for it. My thinking was this really should only apply to verifying TLS connections, not other miscellaneous verifications that happen (e.g. due to UI indicators).
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8471764 - Flags: review?(rlb)
Comment on attachment 8471764 [details] [diff] [review]
patch

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

Location of the telemetry probe is fine.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +740,5 @@
> +                                     bool commonNameInSubjectAltNames)
> +{
> +  if (!commonName) {
> +    // 1 means no common name present
> +    Telemetry::Accumulate(Telemetry::BR_9_2_2_SUBJECT_COMMON_NAME, 1);

This is not an error. Maybe the the description of the histogram is then incorrect?
Also why not return as soon you accumuate to simplify the logic in this function?)

@@ +770,5 @@
> +  // This only applies to certificates issued by authorities in our root
> +  // program.
> +  bool isBuiltIn = false;
> +  SECStatus rv = IsCertBuiltInRoot(rootNode->cert, isBuiltIn);
> +  if (rv != SECSuccess || !isBuiltIn) {

since this is void, a PR_LOG would be nice to catch when IsCertBuildInRoot is failing

@@ +792,5 @@
> +  }
> +
> +  CERTGeneralName* subjectAltNames =
> +    CERT_DecodeAltNameExtension(arena, &altNameExtension);
> +  PORT_Free(altNameExtension.data);

Do we need to free even if its on the arena?

@@ +814,5 @@
> +    if (currentName->type == certDNSName) {
> +      altName.Assign(reinterpret_cast<char*>(currentName->name.other.data),
> +                     currentName->name.other.len);
> +      nsDependentCString altNameWithoutWildcard(altName);
> +      if (altNameWithoutWildcard.Find("*.") == 0) {

do we care for misbehaving CA's(what whould you expect for:
foo.*.bar.com or *.200.234.33.44)?

@@ +888,5 @@
> +      }
> +    }
> +    currentName = CERT_GetNextGeneralName(currentName);
> +  } while (currentName && currentName != subjectAltNames);
> +

The following block of if conditions are hard to parse. (and you need to ensure each condition twice). Why not move this to a function with early returns?
Attachment #8471764 - Flags: review?(cviecco) → review-
Comment on attachment 8471764 [details] [diff] [review]
patch

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

r=me

As to where to put the checks, my feeling, as usual, is that we should do the measurement where we're ultimately going to do the check.  Ideally, adding the check would just replace the Telemetry::Accumulate call with "return failure".  But AuthCertificate seems like a sensible place to add BR checks, so putting the telemetry there is fine.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +736,5 @@
>  
> +void
> +AccumulateSubjectCommonNameTelemetry(const char* commonName,
> +                                     bool subjectAltNamesPresent,
> +                                     bool commonNameInSubjectAltNames)

Little unclear why this is a separate method while the more complicated SAN stuff isn't.  It seems like it might be clearer just to fold this in at the bottom of GatherBaselineRequirementsTelemetry.

@@ +741,5 @@
> +{
> +  if (!commonName) {
> +    // 1 means no common name present
> +    Telemetry::Accumulate(Telemetry::BR_9_2_2_SUBJECT_COMMON_NAME, 1);
> +  } else if (!subjectAltNamesPresent || !commonNameInSubjectAltNames) {

These two variables are treated as one.  (You might as well pass (subjectAltNamesPresent && commonNameInSubjectAltNames).)  Are you sure you don't want to distinguish between the cases (A) No SAN and (2) SAN present, but doesn't contain CN?

In any case, it seems like it would be a little clearer to re-order the logic to start with the positive case:

> if (CN && SAN && CN_in_SAN) { 0 }
> else if (CN) { 2 }
> else { 1 }

@@ +756,5 @@
> +  }
> +}
> +
> +// certList consists of a validated certificate chain. The end-entity
> +// certificate is first and the root (trust anchor) is last.

Could be helpful to summarize the BRs that are being checked here.

@@ +758,5 @@
> +
> +// certList consists of a validated certificate chain. The end-entity
> +// certificate is first and the root (trust anchor) is last.
> +void
> +GatherBaselineRequirementsTelemetry(ScopedCERTCertList& certList)

Should this argument be "const ScopedCERTCertList&"?  We shouldn't be doing anything to modify it here.

@@ +775,5 @@
> +    return;
> +  }
> +  ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
> +  CERTCertificate* cert = endEntityNode->cert;
> +  mozilla::pkix::ScopedPtr<char, PORT_Free_string> commonName(

Would it be useful to put this in ScopedNSSTypes.h?

@@ +816,5 @@
> +                     currentName->name.other.len);
> +      nsDependentCString altNameWithoutWildcard(altName);
> +      if (altNameWithoutWildcard.Find("*.") == 0) {
> +        altNameWithoutWildcard.Assign(altName.get() + 2);
> +      }

There needs to be another case here for the case where altNameWithoutWildcard.Find("*.") > 0, since that would indicate a malformed DNS name ("foo.*.example.com").

@@ +883,5 @@
> +          commonNameStr.Length() - dotIndex - 1);
> +        nsDependentCString altNameRest(altName.get() + 2, altName.Length() - 2);
> +        if (commonNameRest.Equals(altNameRest)) {
> +          commonNameInSubjectAltNames = true;
> +        }

You could just check that commonNameStr ends in altName, with the "*" removed (but the dot still there!).

> (commonNameStr.Find(altNameWithoutStar) == commonNameStr.Length() - altNameWithoutStar.Length()

It would also be good to have this done up within the wildcard branch above, to make sure this doesn't somehow get done against some other weird name.

@@ +902,5 @@
> +    // 5 means there's a DNS name entry with a non-fully-qualified domain name
> +    Telemetry::Accumulate(Telemetry::BR_9_2_1_SUBJECT_ALT_NAMES, 5);
> +  }
> +  if (!nonDNSNameOrIPAddressPresent && !malformedDNSNameOrIPAddressPresent &&
> +      !nonFQDNPresent) {

I would combine these into an if/else if/else, avoiding the need for all the negations in the end here.

::: toolkit/components/telemetry/Histograms.json
@@ +6675,5 @@
> +  "BR_9_2_1_SUBJECT_ALT_NAMES": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 8,
> +    "description": "Baseline Requirements section 9.2.1: subject alternative names extension (0: ok, 1 or more: error)"

It would be helpful to refer to the source file where the error codes are defined (SSLServerCertVerification.cpp).
Attachment #8471764 - Flags: review?(rlb) → review+
(In reply to Camilo Viecco (:cviecco) from comment #2)
> Comment on attachment 8471764 [details] [diff] [review]
> ::: security/manager/ssl/src/SSLServerCertVerification.cpp
> @@ +740,5 @@
> > +    Telemetry::Accumulate(Telemetry::BR_9_2_2_SUBJECT_COMMON_NAME, 1);
> 
> This is not an error. Maybe the the description of the histogram is then
> incorrect?

I updated the description in Histograms.json

> @@ +792,5 @@
> > +  }
> > +
> > +  CERTGeneralName* subjectAltNames =
> > +    CERT_DecodeAltNameExtension(arena, &altNameExtension);
> > +  PORT_Free(altNameExtension.data);
> 
> Do we need to free even if its on the arena?

I added a comment to explain the situation. Long story short: it's not in an arena and we can't use a ScopedSECItem.

> @@ +814,5 @@
> do we care for misbehaving CA's(what whould you expect for:
> foo.*.bar.com or *.200.234.33.44)?

Added a case for the former. The latter already wouldn't produce valid hostnames, so I think it's already handled.

> @@ +888,5 @@
> > +      }
> > +    }
> > +    currentName = CERT_GetNextGeneralName(currentName);
> > +  } while (currentName && currentName != subjectAltNames);
> > +
> 
> The following block of if conditions are hard to parse. (and you need to
> ensure each condition twice). Why not move this to a function with early
> returns?

Are you talking about where various telemetry values are accumulated based on what booleans are set? I'm not sure how that's unclear. We wouldn't want an early return, because multiple booleans could be set, and we want to collect data on them all.
(In reply to Richard Barnes [:rbarnes] from comment #3)
> Comment on attachment 8471764 [details] [diff] [review]
> ::: security/manager/ssl/src/SSLServerCertVerification.cpp
> @@ +736,5 @@
> >  
> > +void
> > +AccumulateSubjectCommonNameTelemetry(const char* commonName,
> > +                                     bool subjectAltNamesPresent,
> > +                                     bool commonNameInSubjectAltNames)
> 
> Little unclear why this is a separate method while the more complicated SAN
> stuff isn't.  It seems like it might be clearer just to fold this in at the
> bottom of GatherBaselineRequirementsTelemetry.

It gets called from different locations in GatherBaselineRequirementsTelemetry, but the logic is all the same, so I factored it out. I'll see if I can simplify the SAN logic in GatherBaselineRequirementsTelemetry.

> @@ +741,5 @@
> > +{
> > +  if (!commonName) {
> > +    // 1 means no common name present
> > +    Telemetry::Accumulate(Telemetry::BR_9_2_2_SUBJECT_COMMON_NAME, 1);
> > +  } else if (!subjectAltNamesPresent || !commonNameInSubjectAltNames) {
> 
> These two variables are treated as one.  (You might as well pass
> (subjectAltNamesPresent && commonNameInSubjectAltNames).)  Are you sure you
> don't want to distinguish between the cases (A) No SAN and (2) SAN present,
> but doesn't contain CN?

Good call.

> In any case, it seems like it would be a little clearer to re-order the
> logic to start with the positive case:
> 
> > if (CN && SAN && CN_in_SAN) { 0 }
> > else if (CN) { 2 }
> > else { 1 }

In this case my reasoning was that the lack of CN or CN in SAN was the positive case (i.e. we're testing if those conditions are true, and gathering data if so). The default, drop-through case is, "we didn't find anything wrong or interesting to note".

> @@ +756,5 @@
> Could be helpful to summarize the BRs that are being checked here.

Good call.

> @@ +758,5 @@
> > +
> > +// certList consists of a validated certificate chain. The end-entity
> > +// certificate is first and the root (trust anchor) is last.
> > +void
> > +GatherBaselineRequirementsTelemetry(ScopedCERTCertList& certList)
> 
> Should this argument be "const ScopedCERTCertList&"?  We shouldn't be doing
> anything to modify it here.

Unfortunately, most NSS functions don't take care with const vs non-const, which propagates backwards into code that calls it. I'll see what I can do, though.

> @@ +775,5 @@
> > +    return;
> > +  }
> > +  ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
> > +  CERTCertificate* cert = endEntityNode->cert;
> > +  mozilla::pkix::ScopedPtr<char, PORT_Free_string> commonName(
> 
> Would it be useful to put this in ScopedNSSTypes.h?

I think we want to avoid using this type as much as possible, so keeping it so we have to declare it like this is probably beneficial.

> @@ +816,5 @@
> > +                     currentName->name.other.len);
> > +      nsDependentCString altNameWithoutWildcard(altName);
> > +      if (altNameWithoutWildcard.Find("*.") == 0) {
> > +        altNameWithoutWildcard.Assign(altName.get() + 2);
> > +      }
> 
> There needs to be another case here for the case where
> altNameWithoutWildcard.Find("*.") > 0, since that would indicate a malformed
> DNS name ("foo.*.example.com").

Good call.

> @@ +883,5 @@
> > +          commonNameStr.Length() - dotIndex - 1);
> > +        nsDependentCString altNameRest(altName.get() + 2, altName.Length() - 2);
> > +        if (commonNameRest.Equals(altNameRest)) {
> > +          commonNameInSubjectAltNames = true;
> > +        }
> 
> You could just check that commonNameStr ends in altName, with the "*"
> removed (but the dot still there!).

Oh, yeah.

> > (commonNameStr.Find(altNameWithoutStar) == commonNameStr.Length() - altNameWithoutStar.Length()
> 
> It would also be good to have this done up within the wildcard branch above,
> to make sure this doesn't somehow get done against some other weird name.
> 
> @@ +902,5 @@
> > +    // 5 means there's a DNS name entry with a non-fully-qualified domain name
> > +    Telemetry::Accumulate(Telemetry::BR_9_2_1_SUBJECT_ALT_NAMES, 5);
> > +  }
> > +  if (!nonDNSNameOrIPAddressPresent && !malformedDNSNameOrIPAddressPresent &&
> > +      !nonFQDNPresent) {
> 
> I would combine these into an if/else if/else, avoiding the need for all the
> negations in the end here.

But if we do if/else if/else, then we won't gather information that we saw a certificate with, for instance, a non-DNSName, non-IPAddress entry as well as a non-FQDN DNSName.

> ::: toolkit/components/telemetry/Histograms.json
> @@ +6675,5 @@
> > +  "BR_9_2_1_SUBJECT_ALT_NAMES": {
> > +    "expires_in_version": "never",
> > +    "kind": "enumerated",
> > +    "n_values": 8,
> > +    "description": "Baseline Requirements section 9.2.1: subject alternative names extension (0: ok, 1 or more: error)"
> 
> It would be helpful to refer to the source file where the error codes are
> defined (SSLServerCertVerification.cpp).

Maybe, but a) the description is fairly long already and b) people can just search for "BR_9_2_1_SUBJECT_ALT_NAMES" in the source tree and c) we might move the implementation and forget to update the description.
Actually things like foo.*.bar.com will get caught by net_IsValidHostName, so we don't need to add a case there.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8471764 - Attachment is obsolete: true
Attachment #8479413 - Flags: review?(cviecco)
Comment on attachment 8479413 [details] [diff] [review]
patch v2

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

Logic is fine, but histograms would not be useful. See comment.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +926,5 @@
> +      commonNameInSubjectAltNames = true;
> +    }
> +    currentName = CERT_GetNextGeneralName(currentName);
> +  } while (currentName && currentName != subjectAltNames);
> +

Since you are using a histogram you cannot make analysis when accumulating in more than one position. For example. Assume you get 100 on pos 0 (no error) 5 on 4, 5 on 4, and 5 on 5. Does it mean that there are 100/115 ok or 100/105 ok?

You should only accumulate once per event so you shoud either order which things you consider important OR replace you bools by and int and mark the errors as flags.
Attachment #8479413 - Flags: review?(cviecco) → review-
I think it's OK to conflate the three error cases.  It's not all that critical to be able to differentiate those errors.  It might be helpful to have a bin for the union of the three (i.e., one of 3,4,5 happened), which would allow us to compute the total number of observations.  But we can also get that directly from the telemetry system.
Comment on attachment 8479413 [details] [diff] [review]
patch v2

Carrying over rbarnes' r+. If we find this doesn't get us good data, we can amend it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7a9e8177202
Attachment #8479413 - Flags: review- → review+
Sorry about that. Looks like I should be using DNS.h anyway: http://hg.mozilla.org/mozilla-central/annotate/372ce1f36116/netwerk/dns/DNS.h#l53
Flags: needinfo?(dkeeler)
https://hg.mozilla.org/mozilla-central/rev/9d11637912b4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment on attachment 8483665 [details] [diff] [review]
patch v2 fixed

String abuse makes me sad.

>+static bool
>+TryMatchingWildcardSubjectAltName(const char* commonName,
>+                                  nsDependentCString altName)
This is just plain wrong. String parameters should be nsString&, with added const and A where possible and C where necessary.

>+  nsDependentCString altNameSubstr(altName.get() + 1, altName.Length() - 1);
nsDependentCString altNameSubstr(altName, 1);

>+  nsDependentCString commonNameStr(commonName, strlen(commonName));
nsDependentCString calls strlen for you.

>+  int32_t altNameIndex = commonNameStr.Find(altNameSubstr);
>+  // This only matches if the end of commonNameStr is the altName without
>+  // the '*'.
>+  // Consider this.example.com and *.example.com:
>+  // "this.example.com".Find(".example.com") is 4
>+  // 4 + ".example.com".Length() == 4 + 12 == 16 == "this.example.com".Length()
>+  // Now this.example.com and *.example:
>+  // "this.example.com".Find(".example") is 4
>+  // 4 + ".example".Length() == 4 + 8 == 12 != "this.example.com".Length()
>+  return altNameIndex >= 0 &&
>+         altNameIndex + altNameSubstr.Length() == commonNameStr.Length();
return StringEndsWith(commonName, altNameSubstr);

I think the whole function could be replaced by the single line
return commonName && StringEndsWith(nsDependentCString(commonName), Substring(altName, 1));
(altName could be a const nsACString& in this case; the original code needs const nsCString&)

>+    nsDependentCString altName;
You probably want this to be an nsCString...

+      nsDependentCString altNameWithoutWildcard(altName);
... but of course you were trying to work out how to create this. I'd use
nsDependentCString altNameWithoutWildcard(altName, 0);
(I was considering defaulting the constructor to 0, but I can't remember whether I asked dbaron.)

>+      if (altNameWithoutWildcard.Find("*.") == 0) {
StringBeginsWith(altNameWithoutWildcard, NS_LITERAL_CSTRING("*."))
Or, altNameWithoutWildcard.Find("*.", false, 0, 0) might work.

>+        altNameWithoutWildcard.Assign(altName.get() + 2, altName.Length() - 2);
altNameWithoutWildcard.Rebind(altName, 2);
(This is the whole point of having an nsDependentCString...)

>+          altName.Assign(buf, strlen(buf));
Assign will call strlen for you.
Hi Neil, the input is appreciated, although perhaps not its tone. Feel free to file a new bug to clean up this implementation.
Blocks: 1075976
You need to log in before you can comment on or make changes to this bug.