557830 - Make cert_DecodeNameConstraintSubTree() produce a correctly-formed linked list when there are multiple names

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Matt McCutchen]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100401 Fedora/3.5.9-1.custom.fc12 Shiretoko/3.5.9
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.3a4pre) Gecko/20100404 Minefield/3.7a4pre

The certificate viewer should decode name constraints extensions (RFC 5280 section 4.2.1.10).  "openssl x509 -text" does.

Reproducible: Always

Steps to Reproduce:
1. Open the certificate manager.
2. Select the "Authorities" tab, click "Import", and select the attached certificate.
3. In the "Downloading Certificate" dialog, click "View".
4. Select the "Details" tab and select the "Certificate Name Constraints" field.
Actual Results:  
The field value box shows:
Critical
Size: 25 Bytes / 200 Bits
30 17 a0 15 30 13 82 11 66 65 64 6f 72 61 70 72 
6f 6a 65 63 74 2e 6f 72 67 

Expected Results:  
The field value box should show something similar to the OpenSSL output:
Critical
Permitted:
  DNS:fedoraproject.org

Comment 1

[Matt McCutchen]

A case for SEC_OID_X509_NAME_CONSTRAINTS would have to be added here:

https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCertHelper.cpp#1640

Comment 2

[Kai Engert [:KaiE:]]

reassign bug owner.
mass-update-kaie-20120918

Comment 4

[Wim Lewis]

Attachment: 557830-fix-constraints-linkage.patch

Comment 5

[Wim Lewis]

Attachment: 557830-multiline-names.patch

Comment 6

[Wim Lewis]

Attachment: 557830-show-name-constraints.patch

Comment 7

[Wim Lewis]

I've attached a series of small patches that implement this enhancement. They're all independent and can be applied alone, but fix-constraints-linkage and multiline-names fix deficiencies that become visible once you use show-name-constraints.

The default cert store contains a cert for the Hellenic Academic and Research Institutions which can be used as a quick test; certs with other forms of name constraint can be easily made using openssl.

Comment 8

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

Hi Wim, thanks for working on this. A couple of things to mention, though:

When a bug needs to make changes to both NSS and gecko, we generally use two separate bugs (since NSS is a separate project, essentially). So, you might move attachment 8621761 [details] [diff] [review] to its own bug if there's something to fix in NSS.

That said, though, gecko doesn't use NSS to parse or enforce name constraints. The new name constraint implementation is in security/pkix/lib/pkixnames.cpp. If we used NSS to decode and display name constraints, any differences in the two implementations would lead to confusion since one library may accept input that the other doesn't. See bug 1024781 for an example of a similar situation.

Speaking more generally, while the certificate viewer does rely largely on NSS to decode and display information, since mozilla::pkix is doing the actual enforcing, we should move away from NSS and towards the new library. In short, while these patches look like good work, I don't believe they are a good addition to the code base while they use NSS.

Comment 9

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

Comment on attachment 8621762 [details] [diff] [review]
557830-multiline-names.patch

See comment 8.

Comment 10

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

Comment on attachment 8621763 [details] [diff] [review]
557830-show-name-constraints.patch

See comment 8.

Comment 11

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

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> Speaking more generally, while the certificate viewer does rely largely on
> NSS to decode and display information, since mozilla::pkix is doing the
> actual enforcing, we should move away from NSS and towards the new library.

David,

With all due respect, I disagree that the cert viewer should use mozilla::pkix. When I designed mozilla::pkix, I designed it to give the yes/no answer to the question "Is this cert valid in the given context?" with as little complication and with the highest amount of assurance of correctness as possible. In particular, I tried to avoid any complications that go beyond answering that question. I think it would be a mistake to try to extend mozilla::pkix to try to answer questions beyond that one.

I think, instead, it makes more sense in the certificate viewer to parse the certificate and certificate extensions using JavaScript code--preferably JS code that could also be used on the web. I believe dkeeler already wrote some code to do simpler things in the cert-explainer tool I've heard about. It would be awesome of the code for decoding parts of the certificate that is used in cert-explainer could be used in the Gecko cert viewer too.

Note, in particular, that the certificate viewer probably should try to display malformed certificates and ideally even highlight the bad parts. But, we don't want mozilla::pkix to have to deal with malformed input except by rejecting it right away, to avoid bugs.

Also note that, ideally, as much of the code in Gecko would be sandboxed. mozilla::pkix is optimized to be used in the non-sandboxed parent process with minimal overhead, but the cert viewer should (eventually) execute in a sandboxed child process in cases where size/speed is not a concern.

Obviously, having a JS parser separate from the mozilla::pkix parser would risk the two becoming out of sync. But, being out of sync wouldn't be a super serious problem. And, because of the need to display malformed certificates, you really need a separate parser anyway. For example, it should be possible to display a certificate that has a syntactically-invalid notAfter, eventually with the error in notAfter highlighted. But, we don't want to change mozilla::pkix so that it can keep parsing a certificate after it encounters a syntax error, because that would make it much more complicated and I think we wouldn't feel so confident that it is correct if we did that.

Comment 12

[Wan-Teh Chang]

Comment on attachment 8621761 [details] [diff] [review]
557830-fix-constraints-linkage.patch

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

::: security/nss/lib/certdb/genname.c
@@ +705,5 @@
>  	    first = last = current;
> +	} else {
> +	  current->l.prev = &(last->l);
> +	  last->l.next = &(current->l);
> +	  last = current;

Wim: thanks a lot for the patch. I think your patch is correct.
It means the current cert_DecodeNameConstraintSubTree function
is broken unless the |subTree| array has only one element, right?

Comment 13

[Wan-Teh Chang]

Attachment: 557830-fix-constraints-linkage.patch v2

Wim: I fixed the indentation problem in your patch and
moved the common "last = current" statement to the
outside of the if-else statement. Please review. Thanks.

Comment 14

[Wim Lewis]

Comment on attachment 8624995 [details] [diff] [review]
557830-fix-constraints-linkage.patch v2

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

(In reply to Wan-Teh Chang from comment #12)
> Wim: thanks a lot for the patch. I think your patch is correct.
> It means the current cert_DecodeNameConstraintSubTree function
> is broken unless the |subTree| array has only one element, right?

Yes. The `next` pointers are correct but the `prev` pointers are incorrect if there are multiple name constraints.

The v2 patch looks good to me and I've built & tested it successfully. Thanks!

Comment 15

[Wan-Teh Chang]

Comment on attachment 8624995 [details] [diff] [review]
557830-fix-constraints-linkage.patch v2

Patch checked in to the NSS hg repository:
https://hg.mozilla.org/projects/nss/rev/29f85320542b

Comment 16

[Kai Engert [:KaiE:]]

Is it necessary to mention the fix in NSS release notes? (Given this is a PSM bug, it cannot show up in the query for fixed bugs in NSS 3.20.)

Comment 17

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

Since this bug has a patch that was checked in to NSS, let's move it to the right component and update it to reflect what actually changed as a result of it. Bug 757832 (previously resolved as a duplicate of this one) can take care of any further action we might take to improve the certificate viewer in Firefox.