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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: factor-out-SignedData.patch

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.

Comment 1

[Dana Keeler (she/her) [:keeler]]

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.

Comment 2

[Dana Keeler (she/her) [:keeler]]

Oh, also, it might be good to throw together a gtest or two.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: factor-out-SignedData.patch

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.

Comment 4

[Dana Keeler (she/her) [:keeler]]

Ok - I suppose we do already have some coverage for this.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/44d8c7a29df1

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44d8c7a29df1