Closed Bug 1018411 Opened 9 years ago Closed 9 years ago

Factor out the parsing of signed data structures into a reusable function in mozilla::pkix::der

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: briansmith, Assigned: briansmith)

Details

Attachments

(2 files)

The structure of signed OCSP responses, signed certificates, signed CMS messages, etc. is the same, and we should factor out the code for parsing this kind of structure into its own function.
Attachment #8431847 - Flags: review?(dkeeler)
Comment on attachment 8431847 [details] [diff] [review]
factor-out-SignedData.patch

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

Cool. I just have one suggestion in terms of what error to set.

::: security/pkix/lib/pkixder.cpp
@@ +101,5 @@
> +
> +  if (input.GetSECItem(siBuffer, mark, signedData.data) != Success) {
> +    return Failure;
> +  }
> + 

nit: trailing whitespace

@@ +113,5 @@
> +        != Success) {
> +    return Failure;
> +  }
> +  if (signedData.signature.len == 0) {
> +    return Fail(SEC_ERROR_OCSP_BAD_SIGNATURE);

Hmmm - but now this isn't always for OCSP, right? How about using SEC_ERROR_BAD_SIGNATURE, and then OCSP-specific code that calls this can check for that error specifically and then upgrade it to the more specific SEC_ERROR_OCSP_BAD_SIGNATURE?

@@ +123,5 @@
> +  // various places, so we enforce it too in order to simplify this code. If we
> +  // find compatibility issues, we'll know we're wrong and we'll have to figure
> +  // out how to shift the bits around.
> +  if (unusedBitsAtEnd != 0) {
> +    return Fail(SEC_ERROR_OCSP_BAD_SIGNATURE);

Same

::: security/pkix/lib/pkixder.h
@@ +46,5 @@
>  #include "secerr.h"
>  #include "secoidt.h"
>  #include "stdint.h"
>  
> +typedef struct CERTSignedDataStr CERTSignedData;

Why is this necessary? Doesn't certt.h do this for us? Is certt.h not exported?

::: security/pkix/lib/pkixocsp.cpp
@@ +456,1 @@
>      return der::Failure;

We could check for SEC_ERROR_BAD_SIGNATURE and upgrade to SEC_ERROR_OCSP_BAD_SIGNATURE here.
Attachment #8431847 - Flags: review?(dkeeler) → review+
Oh, also, it might be good to throw together a gtest or two.
I addressed all your review comments.

I think it would be more convenient for improving the tests if we could land this refactoring first. For example, I think we probably do have a problem with parsing ECC-signed OCSP responses because we don't parse non-NULL parameters in AlgorithmIdentifier. I am working on that issue and that's one reason I factored out this code. Let me know what you think.
Attachment #8432165 - Flags: review+
Flags: needinfo?(dkeeler)
Ok - I suppose we do already have some coverage for this.
Flags: needinfo?(dkeeler)
Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/44d8c7a29df1
Target Milestone: --- → mozilla32
https://hg.mozilla.org/mozilla-central/rev/44d8c7a29df1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.