Closed Bug 374336 Opened 13 years ago Closed 13 years ago

add knowledge of Extended Validation / EV Certificates to PSM

Categories

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

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: dao, Assigned: KaiE)

References

Details

Attachments

(4 files, 11 obsolete files)

15.68 KB, patch
KaiE
: review+
mconnor
: approval1.9+
Details | Diff | Splinter Review
11.29 KB, patch
KaiE
: review+
mconnor
: approval1.9+
Details | Diff | Splinter Review
27.07 KB, patch
Details | Diff | Splinter Review
1.26 KB, patch
neil
: review+
damons
: approval1.9+
Details | Diff | Splinter Review
 
Flags: blocking1.9?
Priority: -- → P1
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/fM2dnQ0AXqlgvWbYnZ2dnUVZ_smonZ2d@mozilla.org
Priority: P1 → P3
Assignee: nobody → kengert
Component: Libraries → Security: PSM
Product: NSS → Core
QA Contact: libraries
Summary: add knowledge of Extended Validation / EV Certificates to NSS → add knowledge of Extended Validation / EV Certificates to PSM
Version: trunk → Trunk
Depends on: 375666
QA Contact: psm
Depends on: 386654
Attached patch incorrect patch for discussion (obsolete) — Splinter Review
No longer blocks: 366797
Depends on: 294531
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #270678 - Attachment is obsolete: true
blocker+ according to johnath
Flags: blocking1.9? → blocking1.9+
Attached patch Patch v3 (obsolete) — Splinter Review
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.)
Attachment #277517 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
Updated patch to match bug 294531 attachment 281270 [details] [diff] [review] (dynamically register OIDs and use SecOidTag)
Attachment #281264 - Attachment is obsolete: true
Attached patch Patch v5 (obsolete) — Splinter Review
Updated patch
Attachment #281271 - Attachment is obsolete: true
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.
Attachment #281598 - Attachment is obsolete: true
Attached patch Patch v6 (obsolete) — Splinter Review
Attachment #282314 - Flags: review?(rrelyea)
Attached patch Patch v7 (obsolete) — Splinter Review
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.
Attachment #282314 - Attachment is obsolete: true
Attachment #282461 - Flags: review?(rrelyea)
Attachment #282314 - Flags: review?(rrelyea)
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
Attachment #282461 - Flags: review?(rrelyea) → review-
Blocks: 400085
Blocks: 400088
(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.
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.
Attached patch Patch v8 part A (obsolete) — Splinter Review
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"
Attachment #282461 - Attachment is obsolete: true
Attachment #285172 - Flags: review?(rrelyea)
Attached patch Patch v8 part B (obsolete) — Splinter Review
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.
Attachment #285173 - Flags: review?(rrelyea)
Bob wants something to print, here is a diff diff.
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 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
Attachment #285172 - Flags: review?(rrelyea) → review-
OK the second issue is addressed in the second patch, so fixing the hasValidOidTag is sufficient to get r+.
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
Attachment #285173 - Flags: review?(rrelyea) → review+
(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.
Attachment #285172 - Attachment is obsolete: true
Attachment #285176 - Attachment is obsolete: true
Attachment #285819 - Flags: review+
(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.
Attachment #285173 - Attachment is obsolete: true
Attachment #285820 - Flags: review+
Comment on attachment 285819 [details] [diff] [review]
Patch v9 part A [[we need BOTH patches]]

r+ relyea for both patches 9A and B
Attachment #285819 - Flags: approval1.9?
Attachment #285820 - Flags: approval1.9?
Attachment #285819 - Attachment description: Patch v9 part A → Patch v9 part A [[we need BOTH patches]]
Attachment #285820 - Attachment description: Patch v9 part B → Patch v9 part B [[we need BOTH patches]]
Attachment #285819 - Flags: approval1.9? → approval1.9+
Attachment #285820 - Flags: approval1.9? → approval1.9+
Endgame drivers approve both patches for M9 landing.
Status: NEW → ASSIGNED
I updated the uuid in the idl prior to check in.
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(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).
Blocks: 400036
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.
Attached patch Fix allocator mismatch (obsolete) — Splinter Review
Attachment #285982 - Flags: review?(kengert)
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.)
Attachment #285982 - Flags: review?(kengert) → review+
Comment on attachment 285982 [details] [diff] [review]
Fix allocator mismatch

Will people see this approval request, even though the bug is resolved fixed already?
Attachment #285982 - Flags: approval1.9?
Yes, approval flags are triaged regardless of bug state.
Blocks: 400906
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. :)
Attachment #285982 - Flags: approval1.9?
Whiteboard: [patch from comment #31 needs landing after beta]
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.
Attachment #285982 - Attachment is obsolete: true
Attachment #308297 - Flags: review+
Attachment #308297 - Flags: approval1.9?
(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 on attachment 308297 [details] [diff] [review]
Updated for bitrot

a1.9+=damons
Attachment #308297 - Flags: approval1.9? → approval1.9+
I assume we want this get checked in before Beta 5.
I offer to check it in today, as soon as the tree opens.
(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!
Johnathan, thanks for the ping.

I've checked in Neil's additional fix for allocators.
Marking fixed again.
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...
Whiteboard: [patch from comment #31 needs landing after beta]
Depends on: 497832
I filed bug 497832 on fixing the compiler warnings mentioned in comment 42.
You need to log in before you can comment on or make changes to this bug.