Bug 1515342 - Checks for invalid bit strings, r=jcj

Differential Revision: https://phabricator.services.mozilla.com/D15061

5 files changed, 63 insertions, 33 deletions

diff --git a/cpputil/nss_scoped_ptrs.h b/cpputil/nss_scoped_ptrs.h
index 24116b63f..3ee7c9e1e 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 <class T>
@@ -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
 
diff --git a/cpputil/scoped_ptrs_util.h b/cpputil/scoped_ptrs_util.h
index dc6e1d752..d0a42ee0b 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 de44444ca..bb5a79c14 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 944117909..a5301f15c 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<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};
 
@@ -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 c186551a2..70ae42b27 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;