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

Status: RESOLVED FIXED

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

Description

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

Attachment: fix-typos-in-signature-field-in-tests.patch

See the patch.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

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.

Comment 2

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

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.

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2fd971f1a91b

Comment 4

[Camilo Viecco (:cviecco)]

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?)

Comment 5

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

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

Comment 6

[Camilo Viecco (: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.