Last Comment Bug 373367 - verify OCSP response signature in libpkix without decoding and reencoding
: verify OCSP response signature in libpkix without decoding and reencoding
Status: RESOLVED FIXED
PKIX
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P2 normal (vote)
: 3.12
Assigned To: Alexei Volkov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-09 10:51 PST by Alexei Volkov
Modified: 2007-12-19 12:14 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
libpkix ocsp code cleanup. (60.01 KB, patch)
2007-12-03 14:59 PST, Alexei Volkov
no flags Details | Diff | Review
Patch v2 (62.83 KB, patch)
2007-12-18 14:38 PST, Alexei Volkov
kaie: review+
Details | Diff | Review

Description Alexei Volkov 2007-03-09 10:51:52 PST
Duplicating bug 342461 so it would be fixed in PKIX library.
Comment 1 Alexei Volkov 2007-12-03 14:59:36 PST
Created attachment 291305 [details] [diff] [review]
libpkix ocsp code cleanup. 

the cleanup removed a lot of duplications and fixes the problem reported in the bug.
Comment 2 Kai Engert (:kaie) 2007-12-11 18:42:35 PST
In cert_CreatePkixProcessingParams you introduced a new parameter enableOCSP, but you never check it. Well, only in some debug code. Is that really what you intended?

For the record, because it's not obvious from the patch: All changes in ocsp.c are code moves, refactoring into more independent functions. There are no logic changes.

Some of the functions in ocsp.c are no longer static, to allow you to access them from the libpkix directoy. Is using "external" references the right approach? It's ok with me, if that's the usual way we do it.

You use:
  ((SECCertificateUsage)1) << certUsage
I think there should be a single global
  #define
for this conversion, to reduce the risk of repeating errors.

In pkix_pl_OcspResponse_CallCertVerify you have if/else.
In the "if" branch, you use targetCert.
In the "else" branch, you use signerCert.
Is that correct?


You are removing a lot of pkix code and replace it with calls into the NSS legacy code.

I can't verify whether this is correct, I assume you are certain it is.
Comment 3 Alexei Volkov 2007-12-17 13:09:21 PST
Kai, thank you for the review.

> In cert_CreatePkixProcessingParams you introduced a new parameter enableOCSP,
> but you never check it. Well, only in some debug code. Is that really what you
> intended?

Yes. This should only be used for testing. Nss cert verification legacy code does not do ocsp rev check on any cert of the chain except on the leaf cert. We intend to preserve such behavior. Bug I will need to have a debug way to exercise the pkix ocsp code. I'll embrace new option of 
cert_CreatePkixProcessingParams with ifdef debug so it would be obvious that it is for debug only.

> For the record, because it's not obvious from the patch: All changes in ocsp.c
> are code moves, refactoring into more independent functions. There are no logic
> changes.

This is correct. There are no functional changes to ocsp.c. Only refactoring. I'm glad that have concurred it.

> Some of the functions in ocsp.c are no longer static, to allow you to access
> them from the libpkix directoy. Is using "external" references the right
> approach? It's ok with me, if that's the usual way we do it.

Now, after you have pointed this out, I think I'll move declarations of the functions that needed in the pkix code into ocspi.h. It will be cleaner. 

> You use:
>   ((SECCertificateUsage)1) << certUsage
> I think there should be a single global
>   #define
> for this conversion, to reduce the risk of repeating errors.

Well, there are number of places that use such shifting of 1 to get to the SECCertificateUsage value. I think, if we should do it, it should be done on the global basis through out libpkix... perhaps in a separate patch.

> 
> In pkix_pl_OcspResponse_CallCertVerify you have if/else.
> In the "if" branch, you use targetCert.
> In the "else" branch, you use signerCert.
> Is that correct?>
It is correct. It is a bad naming, be targetCert is essentially a signerCert converted into libpkix data type.

> You are removing a lot of pkix code and replace it with calls into the NSS
> legacy code.
Yep, this is the part of the cleanup. I've tested it to the extend of our ocsp test base. It passed our tests.
Comment 4 Kai Engert (:kaie) 2007-12-18 08:58:05 PST
(In reply to comment #3)
> 
> > In cert_CreatePkixProcessingParams you introduced a new parameter enableOCSP,
> > but you never check it. Well, only in some debug code. Is that really what you
> > intended?
> 
> Yes. This should only be used for testing. Nss cert verification legacy code
> does not do ocsp rev check on any cert of the chain except on the leaf cert. We
> intend to preserve such behavior. Bug I will need to have a debug way to
> exercise the pkix ocsp code. I'll embrace new option of 
> cert_CreatePkixProcessingParams with ifdef debug so it would be obvious that it
> is for debug only.


And maybe you could rename parameter enableOCSP to checkAllCertsOCSP?
Comment 5 Alexei Volkov 2007-12-18 14:38:04 PST
Created attachment 293751 [details] [diff] [review]
Patch v2

Additional changes: 
    * rename targetCert to pkixSignerCert
    * ifdef debug argument of cert_CreatePkixProcessingParams function
    * move declaration of shared function into ocspi.h
Comment 6 Kai Engert (:kaie) 2007-12-18 14:50:20 PST
Comment on attachment 293751 [details] [diff] [review]
Patch v2

looks good to me, thanks Alexei!

r=kengert
Comment 7 Alexei Volkov 2007-12-19 12:14:46 PST
Patch is commited

Note You need to log in before you can comment on or make changes to this bug.