diff --git a/cpputil/nss_scoped_ptrs.h b/cpputil/nss_scoped_ptrs.h index 24116b63f8..3ee7c9e1ec 100644 --- a/cpputil/nss_scoped_ptrs.h +++ b/cpputil/nss_scoped_ptrs.h @@ -21,13 +21,19 @@ struct ScopedDelete { void operator()(CERTCertificateList* list) { CERT_DestroyCertificateList(list); } + void operator()(CERTDistNames* names) { CERT_FreeDistNames(names); } void operator()(CERTName* name) { CERT_DestroyName(name); } void operator()(CERTCertList* list) { CERT_DestroyCertList(list); } void operator()(CERTSubjectPublicKeyInfo* spki) { SECKEY_DestroySubjectPublicKeyInfo(spki); } + void operator()(PK11Context* context) { PK11_DestroyContext(context, true); } + void operator()(PK11GenericObject* obj) { PK11_DestroyGenericObject(obj); } void operator()(PK11SlotInfo* slot) { PK11_FreeSlot(slot); } void operator()(PK11SymKey* key) { PK11_FreeSymKey(key); } + void operator()(PK11URI* uri) { PK11URI_DestroyURI(uri); } + void operator()(PLArenaPool* arena) { PORT_FreeArena(arena, PR_FALSE); } + void operator()(PQGParams* pqg) { PK11_PQG_DestroyParams(pqg); } void operator()(PRFileDesc* fd) { PR_Close(fd); } void operator()(SECAlgorithmID* id) { SECOID_DestroyAlgorithmID(id, true); } void operator()(SECKEYEncryptedPrivateKeyInfo* e) { @@ -39,16 +45,10 @@ struct ScopedDelete { void operator()(SECKEYPrivateKeyList* list) { SECKEY_DestroyPrivateKeyList(list); } - void operator()(PK11URI* uri) { PK11URI_DestroyURI(uri); } - void operator()(PLArenaPool* arena) { PORT_FreeArena(arena, PR_FALSE); } - void operator()(PK11Context* context) { PK11_DestroyContext(context, true); } - void operator()(PK11GenericObject* obj) { PK11_DestroyGenericObject(obj); } - void operator()(PQGParams* pqg) { PK11_PQG_DestroyParams(pqg); } + void operator()(SECMODModule* module) { SECMOD_DestroyModule(module); } void operator()(SEC_PKCS12DecoderContext* dcx) { SEC_PKCS12DecoderFinish(dcx); } - void operator()(CERTDistNames* names) { CERT_FreeDistNames(names); } - void operator()(SECMODModule* module) { SECMOD_DestroyModule(module); } }; template @@ -63,28 +63,28 @@ struct ScopedMaybeDelete { #define SCOPED(x) typedef std::unique_ptr > Scoped##x +SCOPED(CERTCertList); SCOPED(CERTCertificate); SCOPED(CERTCertificateList); -SCOPED(CERTCertList); +SCOPED(CERTDistNames); SCOPED(CERTName); SCOPED(CERTSubjectPublicKeyInfo); +SCOPED(PK11Context); +SCOPED(PK11GenericObject); SCOPED(PK11SlotInfo); SCOPED(PK11SymKey); +SCOPED(PK11URI); +SCOPED(PLArenaPool); SCOPED(PQGParams); SCOPED(PRFileDesc); SCOPED(SECAlgorithmID); -SCOPED(SECKEYEncryptedPrivateKeyInfo); SCOPED(SECItem); -SCOPED(SECKEYPublicKey); +SCOPED(SECKEYEncryptedPrivateKeyInfo); SCOPED(SECKEYPrivateKey); SCOPED(SECKEYPrivateKeyList); -SCOPED(PK11URI); -SCOPED(PLArenaPool); -SCOPED(PK11Context); -SCOPED(PK11GenericObject); -SCOPED(SEC_PKCS12DecoderContext); -SCOPED(CERTDistNames); +SCOPED(SECKEYPublicKey); SCOPED(SECMODModule); +SCOPED(SEC_PKCS12DecoderContext); #undef SCOPED diff --git a/cpputil/scoped_ptrs_util.h b/cpputil/scoped_ptrs_util.h index dc6e1d7525..d0a42ee0b4 100644 --- a/cpputil/scoped_ptrs_util.h +++ b/cpputil/scoped_ptrs_util.h @@ -37,4 +37,9 @@ SCOPED(PLArenaPool); #undef SCOPED +struct StackSECItem : public SECItem { + StackSECItem() : SECItem({siBuffer, nullptr, 0}) {} + ~StackSECItem() { SECITEM_FreeItem(this, PR_FALSE); } +}; + #endif // scoped_ptrs_util_h__ diff --git a/gtests/common/testvectors/curve25519-vectors.h b/gtests/common/testvectors/curve25519-vectors.h index de44444ca5..bb5a79c14b 100644 --- a/gtests/common/testvectors/curve25519-vectors.h +++ b/gtests/common/testvectors/curve25519-vectors.h @@ -48,9 +48,9 @@ const curve25519_testvector kCurve25519Vectors[] = { 0xf4, 0xeb, 0xa4, 0xa9, 0x8e, 0xaa, 0x9b, 0x4e, 0x6a}, {0x30, 0x38, 0x30, 0x14, 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02, 0x01, 0x06, 0x09, 0x2b, 0x06, 0x01, 0x04, 0x01, 0xda, 0x47, 0x0f, 0x01, - 0x03, 0x20, 0xde, 0x9e, 0xdb, 0x7d, 0x7b, 0x7d, 0xc1, 0xb4, 0xd3, 0x5b, - 0x61, 0xc2, 0xec, 0xe4, 0x35, 0x37, 0x3f, 0x83, 0x43, 0xc8, 0x5b, 0x78, - 0x67, 0x4d, 0xad, 0xfc, 0x7e, 0x14, 0x6f, 0x88, 0x2b, 0x4f}, + 0x03, 0x20, 0x00, 0xde, 0x9e, 0xdb, 0x7d, 0x7b, 0x7d, 0xc1, 0xb4, 0xd3, + 0x5b, 0x61, 0xc2, 0xec, 0xe4, 0x35, 0x37, 0x3f, 0x83, 0x43, 0xc8, 0x5b, + 0x78, 0x67, 0x4d, 0xad, 0xfc, 0x7e, 0x14, 0x6f, 0x88, 0x2b}, {}, false}, diff --git a/gtests/der_gtest/der_quickder_unittest.cc b/gtests/der_gtest/der_quickder_unittest.cc index 944117909d..a5301f15ce 100644 --- a/gtests/der_gtest/der_quickder_unittest.cc +++ b/gtests/der_gtest/der_quickder_unittest.cc @@ -16,17 +16,35 @@ #include "secerr.h" #include "secitem.h" -const SEC_ASN1Template mySEC_NullTemplate[] = { - {SEC_ASN1_NULL, 0, NULL, sizeof(SECItem)}}; - namespace nss_test { +struct TemplateAndInput { + const SEC_ASN1Template* t; + SECItem input; +}; + class QuickDERTest : public ::testing::Test, - public ::testing::WithParamInterface {}; + public ::testing::WithParamInterface {}; +static const uint8_t kBitstringTag = 0x03; static const uint8_t kNullTag = 0x05; static const uint8_t kLongLength = 0x80; +const SEC_ASN1Template kBitstringTemplate[] = { + {SEC_ASN1_BIT_STRING, 0, NULL, sizeof(SECItem)}, {0}}; + +// Empty bitstring with unused bits. +static uint8_t kEmptyBitstringUnused[] = {kBitstringTag, 1, 1}; + +// Bitstring with 8 unused bits. +static uint8_t kBitstring8Unused[] = {kBitstringTag, 3, 8, 0xff, 0x00}; + +// Bitstring with >8 unused bits. +static uint8_t kBitstring9Unused[] = {kBitstringTag, 3, 9, 0xff, 0x80}; + +const SEC_ASN1Template kNullTemplate[] = { + {SEC_ASN1_NULL, 0, NULL, sizeof(SECItem)}, {0}}; + // Length of zero wrongly encoded as 0x80 instead of 0x00. static uint8_t kOverlongLength_0_0[] = {kNullTag, kLongLength | 0}; @@ -53,14 +71,22 @@ static uint8_t kOverlongLength_16_0[] = {kNullTag, kLongLength | 0x10, 0x00, 0x00, 0x00, 0x00}; -static const SECItem kInvalidDER[] = { - {siBuffer, kOverlongLength_0_0, sizeof(kOverlongLength_0_0)}, - {siBuffer, kOverlongLength_1_0, sizeof(kOverlongLength_1_0)}, - {siBuffer, kOverlongLength_16_0, sizeof(kOverlongLength_16_0)}, +#define TI(t, x) \ + { \ + t, { siBuffer, x, sizeof(x) } \ + } +static const TemplateAndInput kInvalidDER[] = { + TI(kBitstringTemplate, kEmptyBitstringUnused), + TI(kBitstringTemplate, kBitstring8Unused), + TI(kBitstringTemplate, kBitstring9Unused), + TI(kNullTemplate, kOverlongLength_0_0), + TI(kNullTemplate, kOverlongLength_1_0), + TI(kNullTemplate, kOverlongLength_16_0), }; +#undef TI TEST_P(QuickDERTest, InvalidLengths) { - const SECItem& original_input(GetParam()); + const SECItem& original_input(GetParam().input); ScopedSECItem copy_of_input(SECITEM_AllocItem(nullptr, nullptr, 0U)); ASSERT_TRUE(copy_of_input); @@ -69,11 +95,10 @@ TEST_P(QuickDERTest, InvalidLengths) { PORTCheapArenaPool pool; PORT_InitCheapArena(&pool, DER_DEFAULT_CHUNKSIZE); - ScopedSECItem parsed_value(SECITEM_AllocItem(nullptr, nullptr, 0U)); - ASSERT_TRUE(parsed_value); + StackSECItem parsed_value; ASSERT_EQ(SECFailure, - SEC_QuickDERDecodeItem(&pool.arena, parsed_value.get(), - mySEC_NullTemplate, copy_of_input.get())); + SEC_QuickDERDecodeItem(&pool.arena, &parsed_value, GetParam().t, + copy_of_input.get())); ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError()); PORT_DestroyCheapArena(&pool); } diff --git a/lib/util/quickder.c b/lib/util/quickder.c index c186551a2d..70ae42b270 100644 --- a/lib/util/quickder.c +++ b/lib/util/quickder.c @@ -758,7 +758,7 @@ DecodeItem(void* dest, case SEC_ASN1_BIT_STRING: { /* Can't be 8 or more spare bits, or any spare bits - * if there are no octets. */ + * if there are no octets. */ if (temp.data[0] >= 8 || (temp.data[0] > 0 && temp.len == 1)) { PORT_SetError(SEC_ERROR_BAD_DER); rv = SECFailure;