Closed Bug 1035470 Opened 6 years ago Closed 6 years ago

Some mozilla::pkix unit tests use a hash algorithm OID instead of a signature algorithm OID in the signature field of test certificates

Categories

(Core :: Security: PSM, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

Comment on attachment 8451968 [details] [diff] [review]
fix-typos-in-signature-field-in-tests.patch

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

::: security/pkix/test/gtest/pkixcert_extension_tests.cpp
@@ +51,5 @@
>      return nullptr;
>    }
>  
> +  return CreateEncodedCertificate(arena, v3,
> +                                  SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION,

Is this a case your strongly-typed enum change would have caught, and if so, why aren't we using it?

https://bugzilla.mozilla.org/show_bug.cgi?id=1002933

I guess all of the nss types are in one big bucket.
Attachment #8451968 - Flags: review+
Thanks for the quick review turnaround. Unfortunately, I didn't notice until after I pushed the patch that you (mmc) reviewed it instead of cviecco, so the commit message still has r=cviecco instead of r=mmc:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd971f1a91b

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #1)
> > +  return CreateEncodedCertificate(arena, v3,
> > +                                  SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION,
> 
> Is this a case your strongly-typed enum change would have caught, and if so,
> why aren't we using it?
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1002933
> 
> I guess all of the nss types are in one big bucket.

Right, NSS only has one OID enum. I actually found this bug because I'm writing a patch that introduces separate strongly-typed enumerations for signature algorithms and hash algorithms as output from the X.509 certificate parser, and that new version of the parser found this problem. But, in generally we don't do the strongly-typed enum stuff for the test data generation stuff in test/**/*. In fact, we don't want to, because we often want to be able to create malformed certificates. But, that flexibility means that sometimes we have to be careful!

Thanks for the quick review turnaround.
https://hg.mozilla.org/mozilla-central/rev/2fd971f1a91b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 8451968 [details] [diff] [review]
fix-typos-in-signature-field-in-tests.patch

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

So the documentation for CreateEncodedCertificate and  TBSCertificate is incorrect? (ie how come this change still works?)
(In reply to Camilo Viecco (:cviecco) from comment #4)
> So the documentation for CreateEncodedCertificate and  TBSCertificate is
> incorrect? (ie how come this change still works?)

Could you please quote the documentation you are referring to?
Flags: needinfo?(cviecco)
I misread http://mxr.mozilla.org/mozilla-central/source/security/pkix/test/lib/pkixtestutil.cpp#709 where the documentation I was reading was referring to the output of the call, not the parameters.
Flags: needinfo?(cviecco)
Attachment #8451968 - Flags: review?(cviecco) → review+
You need to log in before you can comment on or make changes to this bug.