Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Bug 1515342 - Checks for invalid bit strings, r=jcj
Differential Revision: https://phabricator.services.mozilla.com/D15061

--HG--
extra : rebase_source : 60993649c1a956b8aa08ff9fe94e65d5cca794ea
extra : absorb_source : 78922a8a76a41c9f3673014baca32830efefcfa8
extra : histedit_source : d3596996ee297b30da7c4f58033f5a7d4ea31fae
  • Loading branch information
martinthomson committed Sep 27, 2019
1 parent a7f1ff5 commit 2fee826
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 33 deletions.
32 changes: 16 additions & 16 deletions cpputil/nss_scoped_ptrs.h
Expand Up @@ -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) {
Expand All @@ -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 <class T>
Expand All @@ -63,28 +63,28 @@ struct ScopedMaybeDelete {

#define SCOPED(x) typedef std::unique_ptr<x, ScopedMaybeDelete<x> > 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

Expand Down
5 changes: 5 additions & 0 deletions cpputil/scoped_ptrs_util.h
Expand Up @@ -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__
6 changes: 3 additions & 3 deletions gtests/common/testvectors/curve25519-vectors.h
Expand Up @@ -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},

Expand Down
51 changes: 38 additions & 13 deletions gtests/der_gtest/der_quickder_unittest.cc
Expand Up @@ -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<SECItem> {};
public ::testing::WithParamInterface<TemplateAndInput> {};

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};

Expand All @@ -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);
Expand All @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/util/quickder.c
Expand Up @@ -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;
Expand Down

0 comments on commit 2fee826

Please sign in to comment.