374336 - add knowledge of Extended Validation / EV Certificates to PSM

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P3)

Description

[Dão Gottwald [:dao]]

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

By the prioritization criteria for NSS, this is a P3.
I believe it makes more sense to implement this in PSM than in NSS.
I explained the reasons for that view in 
news://news.mozilla.org:23/[e-mail]

Comment 2

[Kai Engert (:KaiE:)]

Attachment: incorrect patch for discussion

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Comment 4

[Benjamin Smedberg]

blocker+ according to johnath

Comment 5

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Updated patch to make use of the newest NSS API proposal (bug 294531 attachment  281263 [details] [diff] [review]).

This combination compiles for me. (It does not yet link, because the implementation for CERT_PKIXVerifyCert is pending.)

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Patch v4

Updated patch to match bug 294531 attachment 281270 [details] [diff] [review] (dynamically register OIDs and use SecOidTag)

Comment 7

[Kai Engert (:KaiE:)]

Attachment: Patch v5

Updated patch

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 281598 [details] [diff] [review]
Patch v5

This patch operates directly on SECItem OIDs.

The latest patch in bug 294531 requires to dynamically register OIDs and use SECOidTag.

This means more work for this patch, especially for the code to load data from the test file in debug mode.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch v6

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Patch v7

This slightly improved version of the patch no longer requires the use of encoded OIDs. The compiled-in OIDs can now be in the more easily understandable dotted notation.

Comment 11

[Robert Relyea]

Comment on attachment 282461 [details] [diff] [review]
Patch v7

r- 

For the following reasons:

1) Comparisons in isEVMatch for subject and issuer should not be string compares, but comparing the DER output. It is possible to get inconsistant decoding of DN's rendered as strings. For the most safety we should just maintain the DN's as der encoded blobs, but I won't require that for checkin.

2) ensureIdentityInfoLoaded is not thread safe. Either document (and verify) that thread safety is not an issue for this function (which means pushing that documentation up the stack to it's callers), or make it thread-safe. PR_CallOnce is a good way to do this (or you could initialize in when Crypto.cpp comes up).

3) getSinglePolicyOID checks the cert for a single policy and fails if it finds more than one. Certificates, in general, however, can contain multiple policies (i.e. both hardware issued certs and approved for classified info, for instance). It is likely true that a given EE cert can only have one EV policy, so one solution is to check for a policy in your EV table for this function.

------ things I found troubling, but would not r- for:

The file parsing code in loadTestEVInfos() seems too easy to get confused or break. Since it's ifdef out except for debug, and is intended to go way, I won't push it.

bob

Comment 12

[Kai Engert (:KaiE:)]

(In reply to comment #11)
> 1) Comparisons in isEVMatch for subject and issuer should not be string
> compares, but comparing the DER output. It is possible to get inconsistant
> decoding of DN's rendered as strings. For the most safety we should just
> maintain the DN's as der encoded blobs, but I won't require that for checkin.

Thanks.
I've filed bug 400085, so I can address it in a separate step. I made it block the next NSS update, so we'll not forget it.


> 2) ensureIdentityInfoLoaded is not thread safe. Either document (and verify)
> that thread safety is not an issue for this function (which means pushing that
> documentation up the stack to it's callers), or make it thread-safe.
> PR_CallOnce is a good way to do this (or you could initialize in when
> Crypto.cpp comes up).

Yes, I agree, I must improve that code. Thanks for your CallOnce proposal.

I decided the "is initialized" state should not be a static variable.
Instead, I made it a member variable of the PSM service (nsNSSComponent).
(This makes sense, because it also triggers the cleanup on shutdown.)


> 3) getSinglePolicyOID checks the cert for a single policy and fails if it finds
> more than one. Certificates, in general, however, can contain multiple policies
> (i.e. both hardware issued certs and approved for classified info, for
> instance). It is likely true that a given EE cert can only have one EV policy,
> so one solution is to check for a policy in your EV table for this function.

Thanks for catching that.
I changed the code to skip all policy extensions that are not known as EV policies.


> ------ things I found troubling, but would not r- for:
> The file parsing code in loadTestEVInfos() seems too easy to get confused or
> break. Since it's ifdef out except for debug, and is intended to go way, I
> won't push it.

Actually, it is meant to stay in the tree, so other people can test new EV roots in their own debug builds, before the new roots get officially added/enabled.
I filed bug 400088, so we can review this debug-only code in a separate step.

Comment 13

[Kai Engert (:KaiE:)]

In order to make review as easy as possible, I will attach the next patch revision as two separate files!

The first file will be the same set of files, so an interdiff will work.

The second will will contain more changes in additional files. Why that, you ask?

While I worked on the review comments, it seemed reasonable to work on bug 400036 at the same time. There would have been overlaps, and the time until the string/feature freeze is too short.

Although this means some new code, the new code is really trivial.
More details will come with the attachments.

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Patch v8 part A

Please use the interdiff feature to compare this patch with Patch v7.

- moved static PRBool dynamicEVOidTagsRegistered
  to nsNSSComponent.h

- introduced a new function isEVPolicy(SECOidTag policyOIDTag)
  that is a clone of old function isApprovedForEV.
  Purpose: Test whether the given OID is known to be an EV OID

- likewise, introduced new function isEVPolicyInExternalDebugRootsFile
  that is a clone of old function isEVMatchInExternalDebugRootsFile

- changed getSinglePolicyOID() into getFirstEVPolicy
  (now returning a tag), making use of above functions

- static ensureIdentityInfoLoaded() converted into a
  PR_CallOnce function. (contents unchanged)

- converted some functions to be members of classes

- the implementation of "test for EV" really belongs into nsNSSCertificate,
  that way we can access it from cert viewer, too.

  I changed nsNSSSocketInfo to simply forward the call to the associated cert.

- introduced a new helper function nsIIdentityInfo::GetValidEVPolicyOid
  This combines obtaining "is ev" status and "what's the ev oid"

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Patch v8 part B

The changes in this part of the patch are trivial, I used a small context, so the patch is printout friendly...

Most changes in mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp are passing a flag all the way down to a function (these functions should really be members of nsNSSCertificate)

The display for bug 400036 is implemented here.

I added the new function to nsIIdentityInfo, as explained in the previous comment already.

Now, both nsNSSSocketInfo and nsNSSCertificate implement that interface.

Comment 16

[Kai Engert (:KaiE:)]

Attachment: diff diff version of patch v8 part a

Bob wants something to print, here is a diff diff.

Comment 17

[Kai Engert (:KaiE:)]

After I have reviews, I'll add a minor fix to the patch.
Current function EnsureIdentityInfoLoaded() uses a plain "nsresult". That must be changed to NS_IMETHOD / NS_IMETHODIMP.

Comment 18

[Robert Relyea]

Comment on attachment 285172 [details] [diff] [review]
Patch v8 part A

r-, but for very simple issues:

1) hasValidOidTag is a PRBool returning function, but it has 2 places where it returns an nsresult. I don't think those could actually fail (unless something was really broken with PSM), but in theory a failure there could incorrectly identify a cert as EV, so the r-.

CleanupIdentityInfo appears to be removed from the declaration and it's call, but still exists, is it still used, and don't you want to free it on shutdown?

bob

Comment 19

[Robert Relyea]

OK the second issue is addressed in the second patch, so fixing the hasValidOidTag is sufficient to get r+.

Comment 20

[Robert Relyea]

Comment on attachment 285173 [details] [diff] [review]
Patch v8 part B

r+ (once patch A get's fixed).

I would like to have you address one issue, however.
I believe ProcessCertificatePolicies is not operating as you intended. As coded it will add the ev indicator on every policy oid if ev is present. I think you only want it on the ev policy oid. To do that you need to change the "if (oidtag != SECOID_UNKNOWN) " to
"if (oidtag == ev_oid_tag) "

Without the latter, your ev_oid_tag becomes simply a bool which says whether or not EV is present.

bob

Comment 21

[Kai Engert (:KaiE:)]

Attachment: Patch v9 part A [[we need BOTH patches]]

(In reply to comment #18)
> 1) hasValidOidTag is a PRBool returning function, but it has 2 places where it
> returns an nsresult. I don't think those could actually fail (unless something
> was really broken with PSM), but in theory a failure there could incorrectly
> identify a cert as EV

Good catch! I changed the function to return nsresult, so we can return the failures directly, and changed the bool result to be an out parameter.

(In reply to comment #19)
> fixing the hasValidOidTag is sufficient to get r+.

Thanks Bob, marking as r=relyea
but feel free to have another look.

Comment 22

[Kai Engert (:KaiE:)]

Attachment: Patch v9 part B [[we need BOTH patches]]

(In reply to comment #20)
> I believe ProcessCertificatePolicies is not operating as you intended. As coded
> it will add the ev indicator on every policy oid if ev is present. I think you
> only want it on the ev policy oid. To do that you need to change the "if
> (oidtag != SECOID_UNKNOWN) " to
> "if (oidtag == ev_oid_tag) "

Good catch again! You're absoluty right, that is what I had intended, this must have been a copy/past mistake :-/

Marking as r=relyea
but feel free to have another look.

Comment 23

[Robert Relyea]

Comment on attachment 285819 [details] [diff] [review]
Patch v9 part A [[we need BOTH patches]]

r+ relyea for both patches 9A and B

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Endgame drivers approve both patches for M9 landing.

Comment 27

[Kai Engert (:KaiE:)]

Attachment: Combined patch v9 [checked in]

I updated the uuid in the idl prior to check in.

Comment 28

[Kai Engert (:KaiE:)]

Is this bug complete?
The backend code is complete, and therefore I'm resolving this as fixed.

Do we have EV now?
No. In order to enable CAs to be trusted for EV, the array myTrustedEVInfos must be populated with valid entries. We should do so in separate bugs.

Comment 29

[Kai Engert (:KaiE:)]

(In reply to comment #28)
> Is this bug complete?
> The backend code is complete, and therefore I'm resolving this as fixed.

Make that "PSM code is complete" (I hope).
We still need NSS backend support (a full implementation of PKIX_VerifyCert).

Comment 30

[[e-mail]]

Comment on attachment 285901 [details] [diff] [review]
Combined patch v9 [checked in]

>+struct nsMyTrustedEVInfo
>+{
>+  const char *dotted_oid;
>+  const char *oid_name; // Set this to null to signal an invalid structure,
>+                        // (We can't have an empty list, so we'll use a dummy entry)
>+  SECOidTag oid_tag;
>+  const char *ev_root_subject;
>+  const char *ev_root_issuer;
>+  const char *ev_root_sha1_fingerprint;
>+};

>+nsMyTrustedEVInfoClass::~nsMyTrustedEVInfoClass()
>+{
>+  delete dotted_oid;
>+  delete oid_name;
>+  delete ev_root_subject;
>+  delete ev_root_issuer;
>+  delete ev_root_sha1_fingerprint;
>+}

>+    temp_ev->ev_root_subject = strdup(subject.get());
>+    temp_ev->ev_root_issuer = strdup(issuer.get());
>+    temp_ev->ev_root_sha1_fingerprint = strdup(fingerprint.get());
>+    temp_ev->oid_name = strdup(readable_oid.get());
>+    temp_ev->dotted_oid = strdup(readable_oid.get());
You've got allocator mismatches here: you shouldn't delete the return from strdup, you should use free instead. You also need to remove those consts.

Comment 31

[[e-mail]]

Attachment: Fix allocator mismatch

Comment 32

[Kai Engert (:KaiE:)]

Comment on attachment 285982 [details] [diff] [review]
Fix allocator mismatch

r=kengert

Neil, thanks a lot for catching that! (Please note this code is used in debug mode only.)

Comment 33

[Kai Engert (:KaiE:)]

Comment on attachment 285982 [details] [diff] [review]
Fix allocator mismatch

Will people see this approval request, even though the bug is resolved fixed already?

Comment 34

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

Yes, approval flags are triaged regardless of bug state.

Comment 35

[Reed Loden [:reed]]

Comment on attachment 285982 [details] [diff] [review]
Fix allocator mismatch

This is a blocker, so there's no need for approval1.9 _once_ the tree reopens for check-ins after beta. :)

Comment 36

[[e-mail]]

Attachment: Updated for bitrot

Oops, I forgot to ask for approval when I got my review, and the patch has now bitrotted - fortunately only because of code removal, so there's no new code.

Comment 37

[Reed Loden [:reed]]

(In reply to comment #34)
> Yes, approval flags are triaged regardless of bug state.

That's not true at all. Drivers only see approval requests in open bugs. :(

Comment 38

[Damon Sicore (:damons)]

Comment on attachment 308297 [details] [diff] [review]
Updated for bitrot

a1.9+=damons

Comment 39

[Kai Engert (:KaiE:)]

I assume we want this get checked in before Beta 5.
I offer to check it in today, as soon as the tree opens.

Comment 40

[Johnathan Nightingale [:johnath]]

(In reply to comment #39)
> I assume we want this get checked in before Beta 5.
> I offer to check it in today, as soon as the tree opens.

Which it now seems to be!

Comment 41

[Kai Engert (:KaiE:)]

Johnathan, thanks for the ping.

I've checked in Neil's additional fix for allocators.
Marking fixed again.

Comment 42

[Daniel Holbert [:dholbert]]

FWIW, this bug added 139 instances of this warning:
nsIdentityChecking.cpp:418: warning: deprecated conversion from string constant to ‘char*’

These originate from this assignment:

83 static struct nsMyTrustedEVInfo myTrustedEVInfos[] = {
84   {
85     // CN=WellsSecure Public Root Certificate Authority,OU=Wells Fargo Bank NA,O=Wells Fargo WellsSecure,C=US
86     "2.16.840.1.114171.500.9",
87     "WellsSecure EV OID",
...
415     "Cg==",
416     nsnull
417   }
418 };

which, due to the const char* --> char* switch suggested in comment 30, involves setting a ton of char* pointers to string literals.

Is there any way we can fix this compiler spam?  I tried reinserting the "const" declaration, but that makes the compiler complain about the nsMyTrustedEVInfoClass destructor freeing const pointers...

Comment 43

[Daniel Holbert [:dholbert]]

I filed bug 497832 on fixing the compiler warnings mentioned in comment 42.