Closed
Bug 1035470
Opened 11 years ago
Closed 11 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)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
Attachments
(1 file)
2.74 KB,
patch
|
cviecco
:
review+
mmc
:
review+
|
Details | Diff | Splinter Review |
See the patch.
Attachment #8451968 -
Flags: review?(cviecco)
Comment 1•11 years ago
|
||
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+
Assignee | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 4•11 years ago
|
||
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?)
Assignee | ||
Comment 5•11 years ago
|
||
(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)
Comment 6•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8451968 -
Flags: review?(cviecco) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•