Closed Bug 969938 Opened 6 years ago Closed 6 years ago

test_ev_certs/generate.py and test_getchains/generate.py generate CA certificates with id-kp-OCSPSigning EKU

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #969931 +++

> CA_eku = ("extendedKeyUsage = critical, serverAuth, clientAuth, " +
>           "1.3.6.1.5.5.7.3.9\n")

1.3.6.1.5.5.7.3.9 is id-kp-OCSPSigning. The new EKU checking code in insaniyy::pkix enforces the constraint that an id-kp-OCSPSigning certificate cannot be a CA certificate. The whole point of a delegated OCSP response signing certificate is to prohibit the OCSP response signer from issuing certificates.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla30
Attached patch Part 1: Fix generation code (obsolete) — Splinter Review
This fixes the generation logic in generate.py. The patch is untested because of bug 969931. The test certificates need to be regenerated. Again, I did not do this yet because of bug 969931.
Attachment #8372926 - Flags: review?(cviecco)
Assignee: brian → cviecco
Blocks: 938046
Summary: test_ev_certs/generate.py generates CA certificates with id-kp-OCSPSigning EKU → test_ev_certs/generate.py and test_getchains/generate.py generate CA certificates with id-kp-OCSPSigning EKU
Assignee: cviecco → brian
Instead of reviewing the certificates themselves, just review the steps I used to regenerate them (See bug 969992 comment 1):

sudo apt-get install python-pexpect
export OBJDIR=<path to objdir>

cd security/manager/ssl/tests/unit/test_ev_certs
PATH=$OBJDIR/dist/bin:$PATH LD_LIBRARY_PATH=$OBJDIR/dist/bin ./generate.py
cd ../test_getchains
PATH=$OBJDIR/dist/bin:$PATH LD_LIBRARY_PATH=$OBJDIR/dist/bin ./generate.py
hg qnew update-certs.patch
Attachment #8372948 - Flags: review?(cviecco)
Attached patch FixSplinter Review
Part 1 had a spurious change to pkixcheck.cpp in it that didn't below. While fixing that, I took the opportunity to combine both patches together. See previous comment about reviewing the binary file changes.
Attachment #8372926 - Attachment is obsolete: true
Attachment #8372948 - Attachment is obsolete: true
Attachment #8372926 - Flags: review?(cviecco)
Attachment #8372948 - Flags: review?(cviecco)
Attachment #8372949 - Flags: review?(cviecco)
Comment on attachment 8372949 [details] [diff] [review]
Fix

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

Brian, can you point me to the place where it says a CA cert cannot have OCSP signing EKU enforced?
and if that is true, the final goal was not to require EKU of signers be a superset of what it signes?

(the patch if we agree on where that happend will be r+)
(In reply to Camilo Viecco (:cviecco) from comment #4)
> Brian, can you point me to the place where it says a CA cert cannot have
> OCSP signing EKU enforced?
> and if that is true, the final goal was not to require EKU of signers be a
> superset of what it signes?
> 
> (the patch if we agree on where that happend will be r+)

That isn't explicitly called out in the spec. But, it is absolutely unsafe for a CA certificate to assert the id-kp-OCSPSigning EKU, because if it did have that EKU, then it could sign OCSP responses for itself, that would be considered valid.
Also, like I mentioned in another bug, it would be better if we didn't add any EKU at all to the CA certificates for these tests, because we're not testing EKU processing in these tests. Instead we should avoid adding extra info to the certs, so it is easier to understand what the test is testing.
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #6)
> Also, like I mentioned in another bug, it would be better if we didn't add
> any EKU at all to the CA certificates for these tests, because we're not
> testing EKU processing in these tests. Instead we should avoid adding extra
> info to the certs, so it is easier to understand what the test is testing.

Is not better to argue to opposite? we should include all the recomendations/current uses of CA so that we test as close as possible to what is deployed? This sounds like an argument to do on irc. 

For bugzilla (EV) the intermediate has: KU: signer, cert signer and crl signer; EKU: SSL Server, SSL Client, Code signing, Email protection and Timestamping; Subject Key ID, Certificate Auth Key Id.

Google intermediate: Key Usage: cert signer, crl signer; (NO EKU); Subject Key ID, Certificate Auth Key Id.

Also you said: 
But, it is absolutely unsafe for a CA certificate to assert the id-kp-OCSPSigning EKU, because if it did have that EKU, then it could sign OCSP responses for itself, that would be considered valid.

That seems like a bug in processing the OCSP response what does libpkix and classic do?
Attachment #8372949 - Flags: review?(cviecco) → review+
https://hg.mozilla.org/mozilla-central/rev/a2a1caee392b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.