1111399 - mozilla::pkix doesn't support RFC822 (email) name constraints

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Preconditions for implementing RFC822 name constraints

It turns out some websites' certificates do use RFC822 name constraints and do have relevant names. That means we need to support them. Otherwise, the fail-closed behavior of the new name constraint code will cause those websites to stop working.

This new implementation seems to conform better to the standards for RFC822 name constraints, by only applying the name constraints to emailAddress names in the certificate subject if there are is no subjectAltName in the cert. Note that this is slightly, but importantly, different than the RFC6125 rule for DNSNames; the RFC6125 rules are about the presence of an IPAddress or DNSName within the SAN, not the presence of the SAN.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Implement RFC822 name constraints

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Implement RFC822 name constraints [v2]

This is the same as the original version, but it works around a bug caused by the lack of real "enum class" support in some of our compilers. In particular, they don't like defining multiple enums with the same tags ("Yes" and "No") in the same scope.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=050c628330d0

Comment 3

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

Comment on attachment 8536236 [details] [diff] [review]
Part 1: Preconditions for implementing RFC822 name constraints

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

::: security/pkix/include/pkix/pkix.h
@@ +94,5 @@
>                        KeyPurposeId requiredEKUIfPresent,
>                        const CertPolicyId& requiredPolicy,
>                        /*optional*/ const Input* stapledOCSPResponse);
>  
> +// TODO: Document

I think this will be handled by bug 968451.

@@ +99,3 @@
>  Result CheckCertHostname(Input cert, Input hostname);
>  
> +// TODO: document RFC XXX stuff.

Are you intending to do the documentation as part of this bug or as a follow-up?

Comment 4

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

Comment on attachment 8536398 [details] [diff] [review]
Part 2: Implement RFC822 name constraints [v2]

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

Great! r=me with comments addressed.
What about tests for CheckCertRFC822Name, though? Or will you address that in a follow-up in CertVerifier/wherever appropriate?

::: security/pkix/lib/pkixnames.cpp
@@ +171,5 @@
> +  AllowWildcards = 0,
> +  DisallowWildcards = 1
> +};
> +
> +MOZILLA_PKIX_ENUM_CLASS DotlessSubdomainMatches

What's a 'dotless subdomain'? A little bit of explanatory documentation would be appreciated here.

@@ +182,5 @@
>  
>  Result MatchPresentedDNSIDWithReferenceDNSID(
> +         Input presentedDNSID,
> +         Wildcards allowWildcards,
> +         DotlessSubdomainMatches allowDotlessSubdomainMatches,

To what do allowWildcards and allowDotlessSubdomainMatches apply? Just presentedDNSID or referenceDNSID? Or both?

@@ +571,5 @@
>  
> +  // Try to match the  CN as a DNSName or an IPAddress.
> +  //
> +  // id-at-commonName        AttributeType ::= { id-at 3 }
> +  // 

nit: trailing space

@@ +645,5 @@
> +                                               Input(ipv4), referenceIDType,
> +                                               referenceID, match);
> +      }
> +    }
> +    

nit: trailing whitespace

@@ +649,5 @@
> +    
> +    // Regardless of whether there was a match, we keep going in case we find
> +    // another CN later. If we do find another one, then this match/mismatch
> +    // will be ignored, because we only care about the most specific CN.
> +  

nit: trailing whitespace

@@ +735,5 @@
>        foundMatch = InputsAreEqual(presentedID, referenceID);
>        rv = Success;
>        break;
>  
>      case GeneralNameType::rfc822Name: // fall through

nit: update comment (we're not falling through anymore, right?)

@@ +1463,5 @@
> +      case 'L': case 'l': case 'Y': case 'y':
> +      case 'M': case 'm': case 'Z': case 'z':
> +        startOfAtom = false;
> +        break;
> +      

nit: trailing whitespace

@@ +1526,5 @@
> +        if (presentedByte == '@') {
> +          break;
> +        }
> +      }
> +      

nit: trailing whitespace

@@ +1559,5 @@
> +      matches = false;
> +      return Success;
> +    }
> +    if (LocaleInsensitveToLower(presentedByte) !=
> +      LocaleInsensitveToLower(referenceByte)) {

nit: indentation

::: security/pkix/test/gtest/pkixnames_tests.cpp
@@ +2289,5 @@
> +  /////////////////////////////////////////////////////////////////////////////
> +  // Additional RFC822 name constraint tests. There are more tests regarding
> +  // the DNSName part of the constraint mixed into the DNSName constraint
> +  // tests.
> +  

nit: trailing whitespace

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8536236 [details] [diff] [review]
Part 1: Preconditions for implementing RFC822 name constraints

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

::: security/pkix/include/pkix/pkix.h
@@ +94,5 @@
>                        KeyPurposeId requiredEKUIfPresent,
>                        const CertPolicyId& requiredPolicy,
>                        /*optional*/ const Input* stapledOCSPResponse);
>  
> +// TODO: Document

Thanks. I removed this comment.

@@ +99,3 @@
>  Result CheckCertHostname(Input cert, Input hostname);
>  
> +// TODO: document RFC XXX stuff.

I removed the declaration and definition of checkRFC822Name. I will add that function in a follow-up bug. (There are no tests for it!).

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8536398 [details] [diff] [review]
Part 2: Implement RFC822 name constraints [v2]

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

Thanks for the quick review turnaround time!

::: security/pkix/lib/pkixnames.cpp
@@ +171,5 @@
> +  AllowWildcards = 0,
> +  DisallowWildcards = 1
> +};
> +
> +MOZILLA_PKIX_ENUM_CLASS DotlessSubdomainMatches

// DNSName constraints implicitly allow subdomain matching when there is no
// leading dot ("foo.example.com" matches a constraint of "example.com"), but
// RFC822Name constraints only allow subdomain matching when there is a leading
// dot ("foo.example.com" does not match "example.com" but does match
// ".example.com").

@@ +182,5 @@
>  
>  Result MatchPresentedDNSIDWithReferenceDNSID(
> +         Input presentedDNSID,
> +         Wildcards allowWildcards,
> +         DotlessSubdomainMatches allowDotlessSubdomainMatches,

allowDotlessSubdomainMatches applies to the way that presentedDNSID is matched to referenceDNSID.

allowWildcards only applies to presentedDNSID, because only presented DNS IDs can be wildcards.

@@ +735,5 @@
>        foundMatch = InputsAreEqual(presentedID, referenceID);
>        rv = Success;
>        break;
>  
>      case GeneralNameType::rfc822Name: // fall through

fixed.

@@ +1559,5 @@
> +      matches = false;
> +      return Success;
> +    }
> +    if (LocaleInsensitveToLower(presentedByte) !=
> +      LocaleInsensitveToLower(referenceByte)) {

fixed.

::: security/pkix/test/gtest/pkixnames_tests.cpp
@@ +2289,5 @@
> +  /////////////////////////////////////////////////////////////////////////////
> +  // Additional RFC822 name constraint tests. There are more tests regarding
> +  // the DNSName part of the constraint mixed into the DNSName constraint
> +  // tests.
> +  

I fixed all the trailing whitespace issues in this patch.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef8135861e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c4424920d74

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ef8135861e4
https://hg.mozilla.org/mozilla-central/rev/9c4424920d74

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

relnoted as "Added support for e-mail name constraints in certificates".