Closed Bug 1136278 Opened 6 years ago Closed 6 years ago

Refactor signature and signature algorithm tests

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox40 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

Our testing of signature algorithm acceptance, public key acceptance, and signature verification is limited. For example, we don't test the lack of acceptance of DSS certificates at all, nor do we test anything related to ECC (within the mozilla::pkix test suite).

Additionally, I noticed that in the one case where we test that we reject DSS certificates, the DSS public key is not encoded correctly. In particular, it is lacking its parameters.
Assignee: nobody → brian
Status: NEW → ASSIGNED
Attachment #8568669 - Flags: review?(dkeeler)
Target Milestone: --- → mozilla39
OS: Windows 8.1 → All
Hardware: x86_64 → All
Comment on attachment 8568669 [details] [diff] [review]
Part 1: Refactor algorithm identifiers in tests

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

::: security/pkix/test/gtest/pkixbuild_tests.cpp
@@ +64,5 @@
>    }
>  
>    ScopedTestKeyPair reusedKey(CloneReusedKeyPair());
>    ByteString certDER(CreateEncodedCertificate(
> +                       v3, sha256WithRSAEncryption(), serialNumber, issuerDER,

Note that I made all of the well-known algorithm IDs functions so that we don't get into issues with the undefined/unpredictable ordering of calling constructors of global values. It's slightly less efficient, but it doesn't matter for this test code, and we need it to be this way so we can use them within the arrays of parameterized test parameters.

::: security/pkix/test/lib/pkixtestalg.cpp
@@ +26,5 @@
> +
> +#include "pkixder.h"
> +
> +// python DottedOIDToCode.py --prefixdefine PREFIX_1_2_840_10040 1.2.840.10040
> +#define PREFIX_1_2_840_10040 0x2a, 0x86, 0x48, 0xce, 0x38

Note: I'm hoping this new way of defining OID constants will work within the core of mozilla::pkix too, to make the code shorter, clearer, and easier to verify the correctness of. Will do that in another bug, sometime.
Comment on attachment 8568669 [details] [diff] [review]
Part 1: Refactor algorithm identifiers in tests

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

LGTM with comments addressed.

::: security/pkix/test/lib/pkixtestalg.cpp
@@ +26,5 @@
> +
> +#include "pkixder.h"
> +
> +// python DottedOIDToCode.py --prefixdefine PREFIX_1_2_840_10040 1.2.840.10040
> +#define PREFIX_1_2_840_10040 0x2a, 0x86, 0x48, 0xce, 0x38

My one comment on this would be that it seems like we shouldn't have to specify the "PREFIX_1_2_840_10040" argument. Do we need to be more flexible than just taking the OID prefix, changing all '.' to '_', and prepending "PREFIX_"? (although, I'm not sure it would be worth it if it requires nontrivial changes to DottedOIDToCode.py)

@@ +96,5 @@
> +           RSA_PKCS1(), TestDigestAlgorithmID::SHA1,
> +           SimpleAlgID({ PREFIX_1_2_840_113549, 1, 1, 5 }), true);
> +}
> +
> +// RFC 3279 Section 2.2.1

Looks like this should be "RFC 4055 Section 5" instead.

::: security/pkix/tools/DottedOIDToCode.py
@@ +140,5 @@
>  
>          // python DottedOIDToCode.py --alg ecdsa-with-SHA512 1.2.840.10045.4.3.4
>          static const uint8_t alg_ecdsa_with_SHA512[] = {
>            0x30, 0x0a, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x04, 0x03, 0x04
>          };

nit: it would be nice to have an example of the new --prefixdefine functionality
Attachment #8568669 - Flags: review?(dkeeler) → review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> > +// python DottedOIDToCode.py --prefixdefine PREFIX_1_2_840_10040 1.2.840.10040
> > +#define PREFIX_1_2_840_10040 0x2a, 0x86, 0x48, 0xce, 0x38
> 
> My one comment on this would be that it seems like we shouldn't have to
> specify the "PREFIX_1_2_840_10040" argument. Do we need to be more flexible
> than just taking the OID prefix, changing all '.' to '_', and prepending
> "PREFIX_"? (although, I'm not sure it would be worth it if it requires
> nontrivial changes to DottedOIDToCode.py)

I'd like to keep this the way it is, until we know if/how we will use --prefixdefine in future patches. In this patch, I always used that form. However, all the OID arcs actually do have real names. Given the limitations with initializer list syntax I've encountered for Android/B2G builds (see next patch), I'm now doubtful I'm going to continue using the prefixdefine mechanism beyond the use in pkixtestalg.cpp for a while. But, if/when we do end up replacing the other OID processing in security/pkix/lib with it, we may prefer to use the names of the arcs instead of the dotted OID of the arc with underscores.

I made the other changes you suggested.
Attached patch Part 1(b): Fix GCC-based builds (obsolete) — Splinter Review
It turns out, despite the "Using C++ in Mozilla Code" page saying that initializer lists are allowed, initializer lists don't work well enough in GCC. And, they work very badly on Android because STLPort doesn't have <initializer_list>, which means that you can't construct a std::basic_string using an initializer list or define a function that takes an initializer list parameter.

This patch fixes the build bustage caused by Part 1 and will be folded into Part 1.
Attachment #8570230 - Flags: review?(dkeeler)
This patch refactors the way the SPKI structures in tests are encoded. This facilitates upcoming patches that expand the test coverage of signature verification in path building so that all algorithms are being tested. In particular, the DSS factoring facilitates further testing that DSS signatures are rejected properly, not just DSS public keys in the end-entity certificate.
Attachment #8570235 - Flags: review?(dkeeler)
This is the final version of the final patch in this bug.

I was originally going to add some more tests in this bug, based on the refactorings I've already posted. However, now I'm planning to add those tests in another bug. That way, the work on bug 1134456 can get unblocked sooner. So, everything posted here should be ready to go.
Attachment #8570235 - Attachment is obsolete: true
Attachment #8570235 - Flags: review?(dkeeler)
Attachment #8570657 - Flags: review?(dkeeler)
Summary: Expand and improve mozilla::pkix signature and signature algorithm tests → Refactoir signature and signature algorithm tests
Summary: Refactoir signature and signature algorithm tests → Refactor signature and signature algorithm tests
Comment on attachment 8570657 [details] [diff] [review]
Part 2: Refactor test SPKI generation [v2]

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

LGTM.

::: security/pkix/test/lib/pkixtestnss.cpp
@@ +248,5 @@
>  
> +  ByteString p(DSS_P());
> +  ByteString q(DSS_Q());
> +  ByteString g(DSS_G());
> +  

nit: trailing whitespace

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +1117,5 @@
> +TestKeyPair::TestKeyPair(const TestPublicKeyAlgorithm& publicKeyAlg,
> +                         const ByteString& spk)
> +  : publicKeyAlg(publicKeyAlg)
> +  , subjectPublicKeyInfo(TLV(der::SEQUENCE,
> +                             publicKeyAlg.algorithmIdentifier + 

nit: trailing whitespace
Attachment #8570657 - Flags: review?(dkeeler) → review+
Attachment #8568669 - Attachment is obsolete: true
Attachment #8570230 - Attachment is obsolete: true
Attachment #8570232 - Attachment is obsolete: true
Attachment #8585853 - Flags: review+
You need to log in before you can comment on or make changes to this bug.