verify OCSP response signature in libpkix without decoding and reencoding

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Duplicating bug 342461 so it would be fixed in PKIX library.
(Assignee)

Updated

11 years ago
Priority: -- → P3
Whiteboard: PKIX
Priority: P3 → P2
Summary: verify signature on an OCSP response without intermediate decoding and encoding → verify OCSP response signature in libpkix without decoding and reencoding
Version: unspecified → trunk
(Assignee)

Comment 1

10 years ago
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.
Attachment #291305 - Flags: review?(kengert)

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
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

10 years ago
(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?
(Assignee)

Comment 5

10 years ago
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
Attachment #291305 - Attachment is obsolete: true
Attachment #293751 - Flags: review?(kengert)
Attachment #291305 - Flags: review?(kengert)

Comment 6

10 years ago
Comment on attachment 293751 [details] [diff] [review]
Patch v2

looks good to me, thanks Alexei!

r=kengert
Attachment #293751 - Flags: review?(kengert) → review+
(Assignee)

Comment 7

10 years ago
Patch is commited
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.