394919 - dNSName constraints should constrain cert Common Names in EE certs when verifying certs for SSL usage

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Presently, and since name constraints were implemented for NSS 3.9, 
constraints on dNSNames only constrain dNSNames in SANs, and never
constrain DNS names in cert subject common names.  

But NSS honors DNSnames in EE cert subject common names, when no SAN 
extension is present.  Consequently, NSS honors unconstrained DNS names
in certs without SAN extensions.  That must be fixed, at least when 
validating certs for SSL server usage.  

This impacts mozilla's root CA policy.  They'd be willing to admit
certain root CA certs IFF the name constraints worked on all DNS names
in SSL server certs.

It may be necessary to implement a test to ensure that we only attempt
to constrain Common Names when those names are (or appear to be) DNS names.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Would be nice for 3.12.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

When working on this bug, someone should look and see if email addresses 
contained within certificate subject names (not SANs) are effectively 
constrained by name constraints, or not.  If not, that is also an issue
for email certs.  

If someone determines that email addresses are, or are not, constrained,
it would be nice to note that finding here in this bug.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

In answer to comment 2, yes, email address attributes in certificate 
common names ARE constrained by email address name constraints.  This
is possible because those subject name attributes have always unambiuously
been for holding RFC 822 email addresses, and no other purpose.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 for experimenting and testing

This patch looks for the Common Name attribute in the cert being validated
and, unless it OBVIOUSLY is not a DNS name, adds that attribute string to 
the list of constrained DNS names.  It does this for all types of certs 
being validated, not only when validating an SSL cert.  It does this only 
in the "old" (pre-libPKIX) code path (which is fine, for now).  

This patch exists for people to play with it, to see how badly it breaks 
the Internet.  :)  I think it is unlikely to break the Internet much at 
all, because it only affects certs issued by intermediate CAs whose CA
certs have been constrained with respect to the DNS domains in which 
they are allowed to issue certificates.  Off hand, I don't know of any 
CAs that are thus constrained.  There probably are some (ask Frank), but
presently they are unknown to me.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Perhaps someone with access to Mozilla's "try server" can make a FF 3.x 
build with this bug's patch for testing.

Comment 8

[Johnathan Nightingale [:johnath]]

(In reply to comment #7)
> Perhaps someone with access to Mozilla's "try server" can make a FF 3.x 
> build with this bug's patch for testing.

I think you should have access - anyone with Mozilla commit privs should?

https://build.mozilla.org/sendchange.cgi

Anyhow, I ran this patch through, the builds are starting to appear now:

https://build.mozilla.org/tryserver-builds/2008-12-15_07:[e-mail]-394919/

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Doh! I submitted that patch (against mozilla-central) this weekend, but forgot to mention it here:
https://build.mozilla.org/tryserver-builds/2008-12-14_03:[e-mail]-394919/

Oh well :)

Comment 10

[Matt McCutchen]

(In reply to comment #6)
> I think it is unlikely to break the Internet much at 
> all, because it only affects certs issued by intermediate CAs whose CA
> certs have been constrained with respect to the DNS domains in which 
> they are allowed to issue certificates.  Off hand, I don't know of any 
> CAs that are thus constrained.

That's beside the point: the motivation to fix this bug is to make such CAs viable to use in the future.

The common name only needs to be constrained if NSS would honor it, i.e., if there are no DNS subjectAltNames.  That would reduce the risk of breakage, since a CA new enough to be name-constrained is probably also new enough to put subjectAltNames in the certificates it issues.

A slightly unorthodox way to completely avoid breakage would be to check the name constraint in CERT_VerifyCertName.  That way, NSS will successfully verify a certificate with a common name that doesn't satisfy the name constraint but will not honor the common name.  That may or may not be desirable depending on the use case.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

The real and best solution to this problem is for NSS to stop honoring 
DNS names inside subject common names, and force all cert to use subject 
ALT names for DNS names, and the PKIX deities intended.  However, no 
browser is yet willing to do that.

Comment 12

[Matt McCutchen]

I would suggest entering an NSS bug to track that we eventually intend to do that.  In the meantime, what prevents us from proceeding with one of the approaches in comment #10 to make name constraints secure with minimal breakage?

Comment 13

[Matt McCutchen]

I entered bug 552346.

Comment 14

[Matt McCutchen]

CAs that want to start issuing name-constrained certificates are going to face a compatibility problem: they have to ensure that cryptographic libraries subject to this bug won't accept the certificates, otherwise there is a vulnerability.  I see two possible ways to address this problem:

1. Put all name-constrained certificates under a new root, and arrange for users to trust that root only if they have a fixed library.  This involves administrative hassle.

2. Standardize a new critical extension that is only accepted by fixed libraries, and include it in certificates with name constraints.  A single agreed-upon extension is necessary because clients will reject the certificate unless they recognize /all/ the critical extensions used by the CA.  This involves standardization work.

Does anyone know if NSS is alone in having a broken implementation of name constraints, or do other popular libraries have the same problem?  OpenSSL doesn't support name constraints at all IIRC.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #14)
> CAs that want to start issuing name-constrained certificates are going to 
> face a compatibility problem: they have to ensure that cryptographic 
> libraries subject to this bug won't accept the certificates, otherwise 
> there is a vulnerability.  

This is not a problem for the CAs. The presence of non-compliant relying 
party software does not generally prevent CAs from issuing compliant certs.
They are not liable for actions of non-compliant relying parties.

Comment 16

[Matt McCutchen]

(In reply to comment #15)
> This is not a problem for the CAs. The presence of non-compliant relying 
> party software does not generally prevent CAs from issuing compliant certs.
> They are not liable for actions of non-compliant relying parties.

According to your logic, this bug would constitute a vulnerability in all current Mozilla software and would have to be prioritized appropriately.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Jean-Marc had a suggestion that I will consider.  Essentially, when 
attempting to verify a chain for an SSL server cert (as signified by the 
requested cert usage of SSLServer), if the first (leaf) cert has no SAN,
then construct a dNSName GeneralName from its Subject Common Name and 
add that to the list of to-be-constrained names.  This will fail if the
name given is a dotted decimal IP address.  I believe that's proper, but 
is likely to lead to complaints ("but it's always worked before").

Comment 18

[Jean-Marc Desperrier]

Won't it only break only if there's both a dotted IP address *and* a name constraint in the chain ? In which case, I think it's easy to explain there's no way to check a name constraint on an IP address, so the only behavior that can be reasonably expected is a failure.

I even think we're more likely to get complaints from people from accidentally inserted a name constraint that they in fact don't want (for example one that's syntactically incorrect so that no name will match it). A preference to disable name constraint checking would solve that.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

End users don't impose name constraints.  CAs impose name constraints on 
subordinate CAs to whom they issue CA certs.  (I'm sure you know this.)
Allowing users to disable name constraints can only serve to allow users 
to accept certs that have been issued in violation of the intermediate CA's
agreement with its superior CA.  We already have cert exceptions.  I don't
think we need yet more ways to help users operate insecurely.

Comment 20

[Matt McCutchen]

(In reply to comment #17)
> Jean-Marc had a suggestion that I will consider.  Essentially, when 
> attempting to verify a chain for an SSL server cert (as signified by the 
> requested cert usage of SSLServer), if the first (leaf) cert has no SAN,
> then construct a dNSName GeneralName from its Subject Common Name and 
> add that to the list of to-be-constrained names.

That appears to be the same as my first suggestion from comment #10, except that it only applies when the usage is SSLServer.  Correct?  What do we do if the usage is certificateUsageCheckAllUsages?

Comment 21

[Matt McCutchen]

I checked RFC 5280, and it does not provide any guidance about what to do here.  It says that email addresses in the subject name must be constrained if the certificate has no subjectAltName (at all):

http://tools.ietf.org/html/rfc5280#section-4.2.1.10

NSS goes further and constrains them unconditionally (comment #3):

https://mxr.mozilla.org/mozilla/source/security/nss/lib/certdb/genname.c#1112

RFC 5280 doesn't even mention the practice of putting a domain name in a common name attribute in the subject name.  RFC 2818 (https) and a recent Internet-Draft mention this practice but do not mention name constraints:

http://tools.ietf.org/html/rfc2818#section-3.1
http://tools.ietf.org/html/draft-saintandre-tls-server-id-check-03#section-4.2.4

It may be worth getting the expected behavior clarified "upstream".

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Attachment: alternative patch v2

Bob, what do you think of this approach?

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Attachment: alternative patch v2.1

I put the new function in a different place.
This makes the patch MUCH easier to read.

Comment 24

[Matt McCutchen]

Comment on attachment 433625 [details] [diff] [review]
alternative patch v2.1

Suggestions from the peanut gallery:

- Separate the code cleanup from the actual changes for this bug.

- To avoid duplicating code, add a boolean parameter and leave the old function as a delegate to the new one.

Comment 25

[Robert Relyea]

Comment on attachment 433625 [details] [diff] [review]
alternative patch v2.1

r+ I like this approach. It matches how the CN would be used.

It's better than what I was thinking (which is to ignore CN's in SSL processing if the name constraint extention exits). This change is more compatible and looks like it will only fail in those cases where we want it to (that is the CN is used, but it's constrained).

bob

Comment 26

[Matt McCutchen]

One more useful link, to the code in CERT_VerifyCertName that accepts the common name:

https://mxr.mozilla.org/mozilla/source/security/nss/lib/certdb/certdb.c#1890

I have raised the issue on the PKIX list (I should have done this a long time ago):

http://www.ietf.org/mail-archive/web/pkix/current/msg27619.html

Comment 27

[Eddy Nigg (StartCom)]

(In reply to comment #26)
> http://www.ietf.org/mail-archive/web/pkix/current/msg27619.html

Isn't a certificate with no dnsName subjectAltName simply invalid for a web site according to RFC 5280?

Comment 28

[Matt McCutchen]

Comment on attachment 433625 [details] [diff] [review]
alternative patch v2.1

>+    /* Now extract any GeneralNames from the subject name names extension. */
>+    rv = CERT_FindCertExtension(cert, SEC_OID_X509_SUBJECT_ALT_NAME, 
>+				&altNameExtension);
[...]
>+    } else if (PORT_GetError() == SEC_ERROR_EXTENSION_NOT_FOUND) {
>+	char *cn = CERT_GetCommonName(&cert->subject);
[...]

The logic there is inconsistent with CERT_VerifyCertName.  CERT_VerifyCertName uses the common name if there is no subjectAltName /of the same type/ as the expected server name passed by the caller (either dNSName or iPAddress), but the patch constrains the common name only if there is no subjectAltName at all.  Thus, a CA could circumvent its name constraint by issuing a certificate with a subjectAltName that is empty (illegal according to RFC 5280, but does NSS reject it?) or only contains other kinds of names.

We probably need to constrain the common name as a certIPAddress if it looks like one and there is no certIPAddress in the subjectAltName, or otherwise as a certDNSName if there is no certDNSName in the subjectAltName.  But this still requires some careful reasoning before I'm convinced that CERT_GetSSLServerCertificateNames includes the common name in all cases in which it could possibly be used by CERT_VerifyCertName.

Comment 29

[Matt McCutchen]

Do we have to change the PKIX verification code too?  It looks like this might be the relevant call to CERT_GetCertificateNames:

https://mxr.mozilla.org/mozilla/source/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c#3186

What is the equivalent of CERT_VerifyCertName for PKIX?  Whatever it is, security depends on it being consistent with the name-constraint checking.

Comment 30

[Matt McCutchen]

(In reply to comment #27)
> Isn't a certificate with no dnsName subjectAltName simply invalid for a web
> site according to RFC 5280?

RFC 5280 does not mention the issue.  RFC 2818 and Saint-Andre's draft (linked in comment #21) describe it as "deprecated" and "legacy" but not invalid.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 28, first paragraph:
You're right.  The code should match the test used by CERT_VerifyCertName.

In reply to the second paragraph:
No, we should stop honoring IP addresses (v4 or v6) represented as strings (e.g. dotted decimal) in fields that we expect to contain DNS names, such as SAN dNSNames and (sadly) Subject CNs.  

In reply to comment 29:
> Do we have to change the PKIX verification code too?

It depends on who "we" are.  Who're you?

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Matt, 
As you are neither a peer nor a module owner for NSS, it is inappropriate for
you to add uninvited review marks to NSS patches.  Comments are welcome.

Comment 33

[Eddy Nigg (StartCom)]

(In reply to comment #30)
> RFC 5280 does not mention the issue.

Correct, it doesn't recognize it, neither deprecated nor otherwise. Nelson can explain probably more about it.

> RFC 2818 and Saint-Andre's draft (linked
> in comment #21) describe it as "deprecated" and "legacy" but not invalid.

Right, RFC 2818 preceded RFC 5280. I believe Peter simply tries to accommodate the fact that it's still in use. It's still a draft, perhaps he might still change it.

Comment 34

[Matt McCutchen]

Comment on attachment 433625 [details] [diff] [review]
alternative patch v2.1

Sorry about the review mark.  I thought Bugzilla would prevent me if I wasn't supposed to add it.

Comment 35

[Matt McCutchen]

(In reply to comment #31)
> In reply to the second paragraph:
> No, we should stop honoring IP addresses (v4 or v6) represented as strings
> (e.g. dotted decimal) in fields that we expect to contain DNS names, such as
> SAN dNSNames

That already works: cert_VerifySubjectAltName checks the SAN dNSNames if the expected name is a DNS name or the SAN iPAddresses if the expected name is an IP address.

> and (sadly) Subject CNs.

I think IP addresses in the subject CN should be covered by bug 552346.

> In reply to comment 29:
> > Do we have to change the PKIX verification code too?
> 
> It depends on who "we" are.  Who're you?

Nobody but a user with a strong desire for correct and usable SSL client software.  I didn't mean to be pushy.  I thought you would be better aware than I of whether having name constraints implemented correctly in the PKIX code path matters for current Mozilla software.

Comment 36

[Matt McCutchen]

Another problem: CERT_VerifyCertName will accept a Netscape SSL server name (SEC_OID_NS_CERT_EXT_SSL_SERVER_NAME), which the patch does not constrain.

One way to avoid fighting all these issues would be to change cert_VerifySubjectAltName to call CERT_GetSSLServerCertificateNames and then fold the resulting code into CERT_VerifyCertName (since it will then handle the common name as well).  This approach would ensure that the name constraint code uses the same set of names as CERT_VerifyCertName.  As an extra benefit, IP addresses in the common name (bug 553754) would get binary comparison and IPv4-IPv6 mapping.

Comment 37

[Peter Saint-Andre]

(In reply to comment #33)
> (In reply to comment #30)
> > RFC 5280 does not mention the issue.
> 
> Correct, it doesn't recognize it, neither deprecated nor otherwise. Nelson can
> explain probably more about it.
> 
> > RFC 2818 and Saint-Andre's draft (linked
> > in comment #21) describe it as "deprecated" and "legacy" but not invalid.
> 
> Right, RFC 2818 preceded RFC 5280. I believe Peter simply tries to accommodate
> the fact that it's still in use. It's still a draft, perhaps he might still
> change it.

Personally I'd love to be able to say that "a certificate with no dnsName subjectAltName [is] simply invalid for a web site" (or any application server) but I'm not yet sure if there is consensus within the IETF to make that statement. I'll be presenting about this Internet-Draft to the PKIX WG on Monday afternoon and will raise the issue there.

http://www.ietf.org/proceedings/10mar/agenda/pkix.txt

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Peter, in the context of https, it suffices for the major browsers to agree.
Perhaps that sounds arrogant, but it is the reality of the marketplace.
The browser vendors tend to discuss these things in a forum for browser 
makers (such as the CABForum) more than in the IETF.  

Matt, 
Because NSS is used in so many products (not merely Mozilla products), we maintain backwards binary compatibility to a very great degree, except when discovered vulnerabilities require that we break it, but even then we strive very hard to retain ABI compatibility.  For that reason, once we make a 
release with a public function, we never thereafter change the public 
signature of that function.  That is why we won't be adding an argument to 
the old public function.  We could add an argument to the new function, so 
that it is capable of performing both functions, and then reimplement the old function as a call to the new one (we tend to do that a lot, actually).  
We tightly control which functions are "public" in each shared library 
through the use of ".def" files, one per shared library.  It is the .def 
files, rather than header files, that ultimately determine what is public.
Or perhaps I should say that it is the intersection of what is found in the
.def files and the public header files that is considered public.

Re: SEC_OID_NS_CERT_EXT_SSL_SERVER_NAME, I think we have a rare opportunity 
here to simply and unilaterally drop support for it.  Because of our strong
commitment to preserving backwards binary compatibility (except where needed
for security reasons), we rarely drop functionality.  But in this case, I 
think we can make an exception, because I believe a strong case can be made
that NO ONE depends on this feature.  That extension was proposed by Netscape many years ago (before SubjectAltNames existed, I think), and was superseded 
by SANs.  None of the CAs who are found in NSS's list of trust root CAs, or 
who are subordinate to those trusted CA, issue certs with that cert extension, AFAIK.  In fact, AFAIK, there never have been any.  AFAIK, no open source or other popular software with which people commonly make certs supports it.  
None of NSS's own currently supported cert utilities support making certs with that extension.  The only CA-like product that ever did, AFAIK, was
http://mxr.mozilla.org/security/source/security/nss/cmd/certcgi/certcgi.c?mark=2057-2065#2057
which hasn't been supported for about 10 years now (died along with 
Netscape Communicator 4).

If you'd like to submit a patch implementing your idea of having CERT_GetSSLServerCertificateNames share code with CERT_VerifyCertName, 
please feel free.  The patch should be a CVS diff against the trunk and 
follow NSS's coding style guidelines.   I'll be happy to consider it and 
review it.  With rare exceptions, new contributors to NSS are generally 
welcome.

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 14:
> Does anyone know if NSS is alone in having a broken implementation of 
> name constraints, or 

I agree that something is broken, but it may or may not be name constraints.
The existing name constraints implementation conforms to RFC 3280.  

The problem, as I see it, the "broken part", is that browsers take and honor 
DNS names from part of the certificate that are not defined to be constrained.
I'd rather fix that than try to add constraints to more and more places, 
especially ones that were not defined (in the standards) to hold DNS names.

> do other popular libraries have the same problem? 

I'm not aware of ANY other browsers that even ATTEMPT to constrain names at
this time, so that's probably the bigger issue, over all.  Mozilla may not
get many CAs to start issuing subordinate CA certs with name constraints 
until a majority of modern relying party software supports it.

Comment 40

[Matt McCutchen]

(In reply to comment #38)
> Because NSS is used in so many products (not merely Mozilla products), we
> maintain backwards binary compatibility to a very great degree [...]
> We could add an argument to the new function, so 
> that it is capable of performing both functions, and then reimplement the old
> function as a call to the new one (we tend to do that a lot, actually).

That was what I intended to propose.

> Re: SEC_OID_NS_CERT_EXT_SSL_SERVER_NAME, I think we have a rare opportunity 
> here to simply and unilaterally drop support for it.

OK, I will enter a separate bug.

Comment 41

[Matt McCutchen]

(In reply to comment #40)
> (In reply to comment #38)
> > Re: SEC_OID_NS_CERT_EXT_SSL_SERVER_NAME, I think we have a rare opportunity 
> > here to simply and unilaterally drop support for it.
> 
> OK, I will enter a separate bug.

I entered bug 554425.

(In reply to comment #39)
> In reply to comment 14:
> > Does anyone know if NSS is alone in having a broken implementation of 
> > name constraints, or 
> 
> I agree that something is broken, but it may or may not be name constraints.
> The existing name constraints implementation conforms to RFC 3280.

What I meant by "broken" is that current NSS versions accept a critical name constraint extension but handle it insecurely.  Other tools would reject the unknown critical extension, which is fine.  Once this bug is fixed, something would have to be done about old NSS versions remaining in the wild before CAs could start issuing name-constrained certificates.  To help clarify the situation, I have entered a separate bug depending on this one for the compatibility issue: bug 554442.

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Alternative patch v 2.2 - less code duplication (checked in)

Matt, your unofficial review comments are invited.  I am about to test this.
This patch doesn't go as far as you had suggested, but I believe it 
eliminates MOST of the duplicated code, and that it addresses your 
correctness concerns.  Do you concur?

Comment 43

[Peter Saint-Andre]

(In reply to comment #38)
> Peter, in the context of https, it suffices for the major browsers to agree.
> Perhaps that sounds arrogant, but it is the reality of the marketplace.
> The browser vendors tend to discuss these things in a forum for browser 
> makers (such as the CABForum) more than in the IETF.  

Yes, I understand that reality. Unfortunately, I am not a member of the CAB Forum, so I need to communicate with people who are directly involved there. That's why discussion in this bug report is so helpful. Feel free to send me further feedback directly or on the [e-mail] list.

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

I want to capture here a response from Stefan Santesson, who was Microsoft's
Program (project? product?) manager for crypto security, in response to Matt's
inquiry to the IETF-PKIX mailing list:

> Date: Thu, 01 Apr 2010 01:59:06 +0100
> From: Stefan Santesson <[e-mail]>
> Message-ID: <[e-mail]>
> Thread-Topic: [pkix] Name constraints on domain name in common name
>
> First I fully agree with all statements that the practice to put a dns name
> inside the common name is a totally no standardized behavior.
> 
> No one that just follow RFC 5280 can be sure that the data stored in CN
> actually holds a common name value. This requires some further knowledge
> about the practice employed by the CA.
> 
> However:
> This has for very long been a common practice for web server certificates,
> where many certificates in fact use CN for this purpose.
> 
> It is also perfectly legal to discard a certificate that is used for server
> authentication if the CN does not match name constraints on dNSName.
> 
> RFC 5280 states in section 6:
>    An implementation MAY augment the algorithm presented in Section 6.1
>    to further limit the set of valid certification paths that begin with
>    a particular trust anchor.  For example, an implementation MAY modify
>    the algorithm to apply a path length constraint to a specific trust
>    anchor during the initialization phase, or the application MAY
>    require the presence of a particular alternative name form in the
>    target certificate, or the application MAY impose requirements on
>    application-specific extensions.  Thus, the path validation algorithm
>    presented in Section 6.1 defines the minimum conditions for a path to
>    be considered valid.
> 
> It IS actually common practice to apply dNSName constraints on CN when the
> certificate has server auth EKU set. This practice is implemented by the
> Microsoft path validation logic in MSCAPI. It is considered the best
> security practice considering how server certificates are issued in
> practice.
> 
> /Stefan

Comment 45

[Nelson Bolyard (seldom reads bugmail)]

Bob, Wan-Teh: review request ping

Comment 46

[Robert Relyea]

Comment on attachment 435496 [details] [diff] [review]
Alternative patch v 2.2 - less code duplication (checked in)

r+ rrelyea

I probably would have blown out in the middle if the SECITEM_CopyItem in the block around line 1129 failed (free cn and goto loser) to avoid the check for rv == success at the end of the loop, but the code is correct. It also makes clear that not getting the common name is not considered fatal in this function (which I believe is intentional, but may not seem so because we are checking rv at the end of the whole if.

As a review I probably would have prefered that the TODO: for the arena marks was implemented, but as a programmer I probably would have done what you did;)... particularly since we know that arena marking is still a noop;).

bob

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Bob,

Now, how can we "Beta" test this before it goes into a real browser release?

Checking in nss/nss.def;        new revision: 1.206; previous revision: 1.205
Checking in certdb/cert.h;      new revision: 1.80;  previous revision: 1.79
Checking in certdb/certi.h;     new revision: 1.32;  previous revision: 1.31
Checking in certdb/genname.c;   new revision: 1.38;  previous revision: 1.37
Checking in certhigh/certvfy.c; new revision: 1.71;  previous revision: 1.70

Comment 48

[Wan-Teh Chang]

Attachment: Patch for libpkix (checked in)

This patch updates the libpkix code to use the new
CERT_GetConstrainedCertificateNames function.  I
independently arrived at the same conclusion as what
Matt McCutchen noted in comment 29.

Note that I simply pass includeSubjectCommonName=PR_TRUE
to the CERT_GetConstrainedCertificateNames call in
PKIX_PL_Cert_CheckNameConstraints, without checking whether
the cert is a leaf (EE) cert.  This is OK because libpkix
only calls PKIX_PL_Cert_CheckNameConstraints under the
condition specified by RFC 5280 in section 4.2.1.10:

   Name constraints are not applied to self-issued certificates (unless
   the certificate is the final certificate in the path).  (This could
   prevent CAs that use name constraints from employing self-issued
   certificates to implement key rollover.)

See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/checker/pkix_nameconstraintschecker.c&rev=1.1&mark=219-222#219

I can't do the equivalent check of
    certUsage == certUsageSSLServer
for libpkix because the equivalent things aren't implemented yet:
http://mxr.mozilla.org/security/ident?i=cert_pi_keyusage
http://mxr.mozilla.org/security/ident?i=cert_pi_extendedKeyusage

Comment 49

[Wan-Teh Chang]

Comment on attachment 435496 [details] [diff] [review]
Alternative patch v 2.2 - less code duplication (checked in)

r=wtc.

This patch doesn't change the behavior of the exported function
CERT_GetCertificateNames, right?  As far as I can tell, it doesn't,
except in an unlikely error case (the original code returns NULL if
the subjectAltName extension exists but cannot be decoded, whereas
the new code returns the subject directory name and RFC822 names in
that case).  The new code does return NULL if the SECITEM_CopyItem
call that copies the subject common name fails, so it's a little
inconsistent in its error handling.

In certdb/genname.c, the comments for CERT_GetCertificateNames
and CERT_GetConstrainedCertificateNames could be improved:

>+/* Extract all names except Subject Common Name from a cert 
>+** in preparation for a name constraints test.
>+*/
>+CERTGeneralName *
>+CERT_GetCertificateNames(CERTCertificate *cert, PRArenaPool *arena)
>+{
>+    return CERT_GetConstrainedCertificateNames(cert, arena, PR_FALSE);
>+}
>+
> /* This function is called by CERT_VerifyCertChain to extract all
> ** names from a cert in preparation for a name constraints test.
> */
> CERTGeneralName *
>-CERT_GetCertificateNames(CERTCertificate *cert, PRArenaPool *arena)
>+CERT_GetConstrainedCertificateNames(CERTCertificate *cert, PRArenaPool *arena,
>+                                    PRBool includeSubjectCommonName)

The comments should describe the fact that the functions return
the subject as a directory name and the email addresses in the
subject as RFC822 names.  The includeSubjectCommonName argument
should also be documented.

Comment 50

[Nelson Bolyard (seldom reads bugmail)]

I'm reopening this.  If I don't, Wan-Teh's review request for his additional
patch will not show up in searches for patches for open bugs.

In answer to Wan-Teh's questions in comment 49, 
> the original code returns NULL if the subjectAltName extension exists but 
> cannot be decoded, whereas the new code returns the [already decoded] 
> subject directory name and RFC822 names in that case.

That change was both intentional and unintentional.  I intended for the code
to work as it now does.  I hadn't noticed that it originally did not, so I didn't realize I was changing it.  Had I noticed it, I would have commented 
on that in a prior bug comment.  

> This patch doesn't change the behavior of the exported function
> CERT_GetCertificateNames, right? 

I am not aware of any other changes to the behavior of that function.  
No (other) changes to that function were intended.

Comment 51

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 448914 [details] [diff] [review]
Patch for libpkix (checked in)

Alexei should review this.

Comment 52

[Wan-Teh Chang]

Comment on attachment 448914 [details] [diff] [review]
Patch for libpkix (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.7).

Checking in pkix_pl_cert.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c,v  <--  pkix_pl_cert.c
new revision: 1.26; previous revision: 1.25
done
Checking in pkix_pl_nameconstraints.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_nameconstraints.c,v  <--  pkix_pl_nameconstraints.c
new revision: 1.7; previous revision: 1.6
done

Comment 53

[Alexei Volkov]

Comment on attachment 448914 [details] [diff] [review]
Patch for libpkix (checked in)

r=alexei

Comment 54

[Robert Relyea]

Comment on attachment 448914 [details] [diff] [review]
Patch for libpkix (checked in)

Already has 2 reviews.

Comment 55

[Matt McCutchen]

I noticed a potential security problem in CERT_GetConstrainedCertificateNames.  If CERT_GetCommonName runs out of memory, it returns null and the common name won't be constrained, though it may still be used by CERT_VerifyCertName.

Someone could try to exploit this by writing a JavaScript loop that repeatedly allocates memory and attempts to connect to an SSL server that is being impersonated using a certificate that violates a name constraint, hoping that the connection will succeed and stay open for other things in the browser to talk to the impersonator.  I don't know how practical such an attack would be.

Should I file a separate bug?

Comment 56

[Wan-Teh Chang]

Yes, please file a separate bug report.  NSS 3.12.7 is
already released, so we need to restrict this bug to what
was done in NSS 3.12.7.

Comment 57

[Matt McCutchen]

Issue from comment #55 -> bug 626414