name constraint checking logic does not comply with RFC 3280

RESOLVED FIXED in 3.9

Status

NSS
Libraries
P1
major
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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.
(Assignee)

Comment 1

14 years ago
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.
(Assignee)

Comment 2

14 years ago
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.
Status: NEW → ASSIGNED
No longer depends on: 208038
Priority: -- → P1
Target Milestone: --- → 3.9
(Assignee)

Comment 3

14 years ago
Created attachment 124788 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Attachment #124788 - Flags: superreview?(wtc)
Attachment #124788 - Flags: review?(relyea)
(Assignee)

Comment 4

14 years ago
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).
Flags: blocking1.4?
Target Milestone: 3.9 → 3.8.1

Comment 5

14 years ago
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).
Attachment #124788 - Flags: review?(relyea) → review+

Comment 6

14 years ago
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] == '.').
(Assignee)

Comment 7

14 years ago
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.  
Target Milestone: 3.8.1 → 3.9

Comment 8

14 years ago
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.
(Assignee)

Comment 9

14 years ago
Created attachment 125068 [details] [diff] [review]
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
(Assignee)

Comment 10

14 years ago
Created attachment 125073 [details] [diff] [review]
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.
Attachment #124788 - Attachment is obsolete: true
(Assignee)

Comment 11

14 years ago
Created attachment 125074 [details]
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

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

r=wtc.

Should we fix SECITEM_CompareItem(), too?
Attachment #125068 - Flags: review+

Comment 13

14 years ago
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.
(Assignee)

Comment 14

14 years ago
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

14 years ago
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

14 years ago
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.
Attachment #125073 - Flags: review+

Comment 17

14 years ago
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.)
(Assignee)

Comment 18

14 years ago
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

14 years ago
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.
Attachment #125073 - Flags: approval1.4?
(Assignee)

Comment 20

14 years ago
Created attachment 125608 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Attachment #125608 - Flags: review?(wtc)

Comment 21

14 years ago
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 */
>+    }
(Assignee)

Comment 22

14 years ago
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

14 years ago
Agreed.  Should we fix the cert reference leak by adding
a CERT_DestroyCertificate call to the caller of
CERT_CompareNameSpace?
(Assignee)

Updated

14 years ago
Depends on: 208649

Comment 24

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

moving approval request forward.
Attachment #125073 - Flags: approval1.4? → approval1.4.x?
(Assignee)

Comment 25

14 years ago
This bug has been fixed in NSS on the trunk for NSS 3.9.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Flags: blocking1.4?
Resolution: --- → FIXED

Comment 26

14 years ago
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?
(Assignee)

Comment 27

14 years ago
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

14 years ago
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.
Attachment #125073 - Flags: approval1.4.x? → approval1.4.x-

Updated

14 years ago
Attachment #125073 - Flags: approval1.4.2?

Updated

14 years ago
Attachment #124788 - Flags: superreview?(wchang0222)

Comment 29

14 years ago
Nelson, is this pretty safe for 1.4.2?
(Assignee)

Comment 30

14 years ago
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

14 years ago
clearly wtc felt that this patch should go in 1.4...

Comment 32

14 years ago
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.)
(Assignee)

Comment 33

14 years ago
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.
Attachment #125608 - Flags: review?(wchang0222)

Comment 34

13 years ago
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
Attachment #125073 - Flags: approval1.4.2?
You need to log in before you can comment on or make changes to this bug.