208047 - name constraint checking logic does not comply with RFC 3280

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

When a CA cert contains a nameConstraints extension, the names in all
subordinate certs are checked against those constraints as part of cert chain
validation.  The code in NSS that does this is in function CERT_CompareNameSpace
and its subordinate functions, especially cert_CompareNameWithConstraints, both
in nss/lib/certdb/genname.c.  The logic used in those functions, especially in 
cert_CompareNameWithConstraints, is very different from what is specified in 
RFC 3280.  

The test for directoryNames may find matches where it should not.  The tests for
all other types of names are very likely not to find matches where they should.  
Depending on the type of constraint (permitted or excluded subtree), this may
result in false positive or negative outcomes for the validity of certs in a
chain.  

The existing code has (at least) the following issues:

I1. Directory names match Attribute Types And Values (ATAVs, a.k.a., AVAs) in
any order, ignoring the hierarchical sequencing imposed by Relative
Distinguished Names.  Thus C=US, ST=CA, O=AOL  matches  C=US, O=AOL, ST=CA  
even when each of those 3 AVAs is in a separate RDN. 

I2. constraints on DNSnames, RFC822names and URI hostnames all expect the
constraint GeneralName to be a regular expression with "*" as a wildcard.
No type of name constraint is defined to use regular expressions in this way,
so this logic is improper for all those types.  In addition, URI hostname
parsing fails to strip away user names and port numbers prior to checking.

I3. IPAddress constraints are not recognized.  Neither are OtherNames, 
X400 Addresses, EDInames, and Registered OIDs.

I4. All unrecognized constraint GeneralName types lead to failure to match.

I5. When a nameConstraint contains a permitted subtree, then subordinate 
certs may contain ONLY the types of names given in that permitted subtree. 
This seems to necessitate some "wild card" definition for all types of 
general names.  

Open questions about name constraints as defined in RFC 3280:

Q1.  Is the behavior in I5 above proper?  Or should a name of a type not 
found in the permitted subtree be considered not constrained by that subtree?

Q2.  Consider a constraint containing a permitted subtree containing a base
GeneralName with a context-dependent tag (identifying the type of GeneralName)
but with a zero-length value, or (for directoryName) containing a value that
is a sequence of zero length.  

Q2A. How is such a constraint interpreted?  

Q2B. For what types (if any) is it considered a wildcard, matching all
GeneralNames of that type?

Q2C. What meanings does such a constraint have for other types of GeneralName?

Q3. For constraint subtree GeneralNames of types OtherName, X400name,
EDIPartyName and RegisteredID, which we do not use or understand, is it 
permissible to "ignore" these types; that is, to never match them to any
name in an excluded subtree, and to always match them to any name of the 
same type (including same OID for OtherName) for permitted subtrees?  

Q4. For directoryName constraints, When an RDN in a cert's name contains
more AVAs than are found in the corresponding constraint's RDN, does the 
name match the constraint or not?  Example, using brackets to denote RDN
boundaries:
constraint name:  [C=US] [ST=CA] [O=AOL]
cert name:        [C=US] [ST=CA, L=Mountain View] [O=AOL] [OU=...
Does this name match this constraint?

PROPOSAL:

I will shortly attach a patch to this bug that proposes to resolve the above
questions as follows:

A1. Issue 5 above is presumed to be correct.  When a nameConstraint contains a
permitted subtree, then subordinate certs may contain ONLY the types of names
given in that permitted subtree.  Any name type for which no matching name 
type exists in the permitted subtree is not permitted.

A2. Constraints with zero length DNSnames, RFC822names, or URI hostnames are
treated as wildcards, matching any and all names of their respective types.

A3. Constraint GeneralNames of types X400name, EDIPartyName and RegisteredID 
are "ignored", not matching any names in excluded subtrees, matching all names
of the same type in permitted subtrees.  For constraints of type OtherName, 
in excluded subtrees they match no names, and in permitted subtrees, they match
any name with the same type-id OID.  

Constraints of other types of GeneralNames, not defined in RFC 3280, always
cause a matching name type to fail the constraint, matching in excluded subtrees
and not matching in permitted subtrees.

A4. As long as all the AVAs in all the respective RDNs find matches in a cert
name, then that name is considered to match, even if the name contsins other
AVAs in those same RDNs.  The example in Q5 above is considered to match,
regardless of whether the constraint is in the excluded or permitted subtree.

Your review of the proposed answers above and of the attachments is invited.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Addition to A3 in the above proposal.  For IPAddress constraints, the
constraint's GeneralName actually contains an IP address and a subnet mask. 
Wildcarding is accomplished using a zero subnet mask.  So, IMO, no further
definition of a wildcard for IPAddress constraints is necessary.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Marking P1 for 3.9.  Removing dependency on 208038 because the patches for this
bug and that do not overlap.  

I decided to have a zero-length IPaddress constraint be a wildcard, too.
There is a cert "in the wild" that seems to expect this.  See 
http://bugzilla.mozilla.org/show_bug.cgi?id=204555#c23 for an example.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch implements above proposal

This patch 
1. deletes static functions compareNameToConstraint (which evaluated simple
regular expressions) and secItem2String (the new code operates on SECItems 
without needing to convert them to zStrings first).  
2. Adds these new functions:
   compareDNSN2C
   compareRFC822N2C
   compareIPaddrN2C
   parseUriHostname
3. Significantly changes cert_CompareNameWithConstraints, which now calls the
five new functions named above.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I am requesting that this bug block moz 1.4.  My rationalle is as follows.
I believe moz 1.4 will be the last release of mozilla to support encrypted
and/or signed email.  This is because, AFAIK, neither firebird nor thunderbird
have any support for users' own personal certs (and private keys) and no such
support is planned.  Therefore, the bugs in moz 1.4's secure email will be
around a long time.  Taking this fix means moz 1.4's secure email will continue
to be useful even after such time as name constraints become commonplace in CA
certs (which I believe is forthcoming).

Comment 5

[Robert Relyea]

Comment on attachment 124788 [details] [diff] [review]
patch implements above proposal

None of my comments should block this patch from checking as is.

It would be nice if the table for compare DNSN2C included a 4th column which
explains which test accomplishes the desired result. Example:
.foo.bar.com foo.bar.com nomatch  name->len < constraint< len

.foo.bar.com .foo.bar.com should be added to the table for completeness (the
DNS name in this case is invalid, but the code will match it: offset=0).

Comment 6

[Wan-Teh Chang]

Comment on attachment 124788 [details] [diff] [review]
patch implements above proposal

I have the same question about the compareDNSN2C function.
It's not clear to me why we need to handle the case

    .foo.bar.com     www..foo.bar.com	   no match

which fails the (name->data[offset - 1] == '.') + (constraint->data[0] == '.')
== 1
test, while many other invalid DNS names (for example,
www..baz.foo.bar.com) would match.  If we don't need to
handle that case, we can simplify the test to
(name->data[offset - 1] == '.') || (constraint->data[0] == '.').

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

re: comment 6. Both tests are necessary.  We can either combine them in a way
that eliminates at least one invalid DNS name pattern, or in a way that 
eliminates no invalid patterns.  Is eliminating the one pattern objectionable?

I am working on another version of this patch that changes the meaning of the
value returned by cert_CompareNameWithConstraints from "The name does (or does
not) match one of the constraints" to "the name is (or is not) acceptable for 
passing the name constraints validity test".  This should simplify the logic 
and reduce the number of return paths, which is desirable for the trunk.  It
will not be any more or less correct than this existing patch, however, so if
patch 124788 is accepable, I'd like to check it in on the NSS 3.8 branch for moz
1.4.  

Comment 8

[Wan-Teh Chang]

Nelson wrote:
> Is eliminating the one pattern objectionable?

No, it's not objectionable.  I was just wondering
why only that pattern is eliminated.  When you eliminate
one pattern but not the others, it makes the code
reviewer wonder whether there is some significance
about that pattern that he missed.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: prerequisite for patch above

The patch to genname.c in this bug adds a call function SECITEM_ItemsAreEqual.
That function is also vulnerable to a NULL pointer crash, if either item's 
data pointer is NULL.  This patch corrects that, as well as making that
function faster when item lengths are not equal.  This patch is prerequisite 
to the patch to genname.c

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2, from dff -u, pretty unreadable, since this is a rewrite

Like the first version, this patch 
1. deletes static functions compareNameToConstraint (which evaluated simple
regular expressions) and secItem2String (the new code operates on SECItems 
without needing to convert them to zStrings first).  
2. Adds these new functions:
   compareDNSN2C
   compareRFC822N2C
   compareIPaddrN2C
   parseUriHostname
3. Significantly changes cert_CompareNameWithConstraints, which now calls the
five new functions named above.  

This patch incorporates review feedback.  It also removes the temporary arenas
and calls to the copy functions, which were completely unneeded.  

Checked in on NSS trunk, rev 1.12.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2, just the new source code, not a diff, easier to read

This is the new source code found in patch v2.	This form should be easier to 
read and review, since it is a rewrite, the diff -u form isn't that useful.
Note that attachment 125068 [details] [diff] [review] is still a prerequisite to patch v2, as it was to 
patch v1.

Comment 12

[Wan-Teh Chang]

Comment on attachment 125068 [details] [diff] [review]
prerequisite for patch above

r=wtc.

Should we fix SECITEM_CompareItem(), too?

Comment 13

[Wan-Teh Chang]

Comment on attachment 125068 [details] [diff] [review]
prerequisite for patch above

>+    if (!a->data || !b->data) {
>+        /* avoid null pointer crash. */
>+	return (PRBool)(a->data == b->data);
>+    }

We may want to augment this with an assertion:

      PORT_Assert(a->data && b->data);

because at that point both a->len and b->len
are nonzero and it is an error for a SECItem to
have a null 'data' and a nonzero 'len'.  This
function has no way to report an error, hence
the suggestion of using an assertion.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Suggested patch for SECITEM_CompareItem:


Index: secitem.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/util/secitem.c,v
retrieving revision 1.8
diff -u -5 -r1.8 secitem.c
--- secitem.c   6 Jun 2003 04:51:26 -0000       1.8
+++ secitem.c   6 Jun 2003 22:06:47 -0000
@@ -141,10 +141,15 @@
 SECITEM_CompareItem(const SECItem *a, const SECItem *b)
 {
     unsigned m;
     SECComparison rv;

+    if (!a || !a->len || !a->data)
+        return (!b || !b->len || !b->data) ? SECEqual : SECLessThan;
+    if (!b || !b->len || !b->data)
+       return SECGreaterThan;
+
     m = ( ( a->len < b->len ) ? a->len : b->len );

     rv = (SECComparison) PORT_Memcmp(a->data, b->data, m);
     if (rv) {
        return rv;

Comment 15

[Wan-Teh Chang]

Comment on attachment 125073 [details] [diff] [review]
patch v2, from dff -u,  pretty unreadable, since this is a rewrite

Please attach an incremental patch that addresses the case
of comparing a directory name with only email address
constraints.  Then I will mark this patch review+.

Comment 16

[Wan-Teh Chang]

Comment on attachment 125073 [details] [diff] [review]
patch v2, from dff -u,  pretty unreadable, since this is a rewrite

Nelson explained to me why the patch correctly handles
the case of comparing a directory name with only email
address constraints.

r=wtc.

Comment 17

[Wan-Teh Chang]

Nelson, is it possible for a directory name to have only
email AVAs?  That is, all the AVAs of all the RDNs have
the tag SEC_OID_PKCS9_EMAIL_ADDRESS or SEC_OID_RFC1274_MAIL.

If this is possible, and if all the constraints are email
address constraints, then phase 2 may not handle this case
correctly.  (All the name constraint checking has been
done in phase 1.  Phase 2 should not alter the result.)

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

It is possible for a directory name to contain only emailaddress type attributes.
(I would say that a well formed directory address should not contain only those
attributes, but it is possible.)  

It is possible for a name constraints extension to contain only GeneralNames of
type rfc822name.   If a name constraints extension containes a "permitted
subtree" that consists only of rfc822Names, then according to points I5 and A1
above, 
no directory name would be permitted, because no directoryname was defined in
the permitted subtree.  

Is there some other case being overlooked?

Comment 19

[Wan-Teh Chang]

Comment on attachment 125073 [details] [diff] [review]
patch v2, from dff -u,  pretty unreadable, since this is a rewrite

Requesting Mozilla 1.4 approval.  This is a rewrite of the
cert_CompareNameWithConstraints function.

The risk of this patch is low, as explained below.

1. Logic: the current code does not implement the RFC
correctly in many cases.  The new code vastly improves
the conformance to the RFC.

2. Crashes: the current code already has the crash fixes
that we recently checked into the Mozilla 1.4 branch.  The
new code takes the same precautions against crashes.  So
the risk of crashes should be comparable.

The reward of this patch is that we will significantly improve
our handling of the "name constraints" certificate extensions,
from "very broken" to "mostly compliant".  We will address
the remaining non-compliances in future patches.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Attachment: incremental patch, to be applied on top of previous v2 patch

This patch makes a few corrections per discussion with an IETF PKIX luminary.
1. An email constraint consisting of a single dot (".") matches all email
boxes.
2. An empty IPAddress constraint is invalid and matches no IPAddress
generalName.
3. (the biggie). If the constraint's permitted subtree contains no names of a
certain type, then all names of that type are permitted (unless explicitly 
excluded in the excluded subtree).  This is a reversal of our previous policy.

Comment 21

[Wan-Teh Chang]

Comment on attachment 125608 [details] [diff] [review]
incremental patch, to be applied on top of previous v2 patch

1. In compareRFC822N2C, should this be removed?

>     if (!constraint->len)
>         return SECSuccess;

Since "." is the wildcard, I am wondering if we still
need to treat a constraint of length zero as a wildcard.

2. In CERT_CompareNameSpace, we shouldn't dup the cert
because the caller of this function doesn't destroy the
cert.

>+    if (rv != SECSuccess) {
>+	return (PORT_GetError() == SEC_ERROR_EXTENSION_NOT_FOUND)
>+	       ? NULL  /* success, space is unconstrained. */
>+	       : CERT_DupCertificate(cert); /* failure, some other error */
>+    }

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Regarding comment 21, part 1, I think the answer depends on whether we want the
cert chain reported in bug 204555 to pass or not.  If we want that chain to 
pass the test, then we must interpret a zero-length RFC822 name constraint as
a wildcard.

Regarding comment 21, part 2.  I think you've uncovered a cert reference leak,
yet another bug, in the original code.  

When returning a non-NULL cert pointer, the pointer should either always be a
new cert reference, or never be a new cert reference.  It should either always
or never be proper for the caller of this function to destroy that reference.  

Prior to this change, the only way for this function to return a non-NULL cert
pointer was to return the value that is returned by CERT_FindCertByName, which
does bump the ref count on that cert. In other words, CERT_FindCertByName
returns a new cert reference, so I dup'ed the cert handle to ensure that a new
cert reference is returned in this path, too.  

As you know, there is another bug filed about the problems with the
CERT_CompareNameSpace API.  I think it is appropriate for this function to
remain consistent, returning a new cert reference for all non-null return 
values, until that other bug is fixed.  When that bug is fixed, there will
no longer be any need to return a new cert reference (since the value returned
will always be one of the arguments passed into the function).  At that time,
it would be good to remove the DupCert call.  Agreed?

Comment 23

[Wan-Teh Chang]

Agreed.  Should we fix the cert reference leak by adding
a CERT_DestroyCertificate call to the caller of
CERT_CompareNameSpace?

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 125073 [details] [diff] [review]
patch v2, from dff -u,  pretty unreadable, since this is a rewrite

moving approval request forward.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

This bug has been fixed in NSS on the trunk for NSS 3.9.

Comment 26

[Asa Dotzler [:asa]]

Has there been any negative feedback about this since it landed on the trunk?
How critical is this to get into Mozilla 1.4.1?

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

No negative feedback has come to me.  

Considering the absence of secure email in *bird, I'd surely like to see the
last mozilla release to feature secure email to be as compliant as we can make it.

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 125073 [details] [diff] [review]
patch v2, from dff -u,  pretty unreadable, since this is a rewrite

This is not going to make 1.4.1.  Please re-request aproval after 1.4.1 ships
if you'd like to get this in for 1.4.2.

Comment 29

[Mike Kaply [:mkaply]]

Nelson, is this pretty safe for 1.4.2?

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Michael,  I would not recommend that you try to pick and choose inidivual 
patches to NSS for moz 1.4.2.  This is because of interdependencies that 
may exist bewteen patches in various files.  A patch like this is likely to
be "safe" only if  you take other patches on which it depends.  I do not
recall, now, what patches this patch depends upon, and I do not know 
what patches are already in the moz 1.4.x tree.

The NSS group produces various Betas and releases of NSS< and I could 
recommend that you consider moving moz 1.4.2 to one of those well defined
points rather than cherry picking individual patches.

Comment 31

[Mike Kaply [:mkaply]]

clearly wtc felt that this patch should go in 1.4...

Comment 32

[Wan-Teh Chang]

Michael, it's been months since I requested 1.4.2 approval.
At this point I agree with Nelson that it is safer for
Mozilla 1.4.2 to upgrade to the next NSS 3.8.x stable release.
If you are interested, we can look into that.  (NSS 3.8.3 is
very close to completion.)

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 125608 [details] [diff] [review]
incremental patch, to be applied on top of previous v2 patch

Removing review request.  Patch was checked in 6 months ago.

Comment 34

[Mike Kaply [:mkaply]]

Comment on attachment 125073 [details] [diff] [review]
patch v2, from dff -u,  pretty unreadable, since this is a rewrite

We took NSS 3.9 for 1.4.2