373367 - verify OCSP response signature in libpkix without decoding and reencoding

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Alexei Volkov]

Duplicating bug 342461 so it would be fixed in PKIX library.

Comment 1

[Alexei Volkov]

Attachment: libpkix ocsp code cleanup.

the cleanup removed a lot of duplications and fixes the problem reported in the bug.

Comment 2

[Kai Engert (:KaiE:)]

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]

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:)]

(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]

Attachment: 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:)]

Comment on attachment 293751 [details] [diff] [review]
Patch v2

looks good to me, thanks Alexei!

r=kengert

Comment 7

[Alexei Volkov]

Patch is commited