Bug 1321998 - don't overrun OIDs in alg1485 part 2, r=ttaubert

Differential Revision: https://nss-review.dev.mozaws.net/D366

3 files changed, 30 insertions, 1 deletions

diff --git a/cmd/pp/pp.c b/cmd/pp/pp.c
index 9f33d10a4..d6e276834 100644
--- a/cmd/pp/pp.c
+++ b/cmd/pp/pp.c
@@ -84,6 +84,8 @@ main(int argc, char **argv)
                 if (!inFile) {
                     fprintf(stderr, "%s: unable to open \"%s\" for reading\n",
                             progName, optstate->value);
+                    PORT_Free(typeTag);
+                    PL_DestroyOptState(optstate);
                     return -1;
                 }
                 break;
@@ -93,6 +95,8 @@ main(int argc, char **argv)
                 if (!outFile) {
                     fprintf(stderr, "%s: unable to open \"%s\" for writing\n",
                             progName, optstate->value);
+                    PORT_Free(typeTag);
+                    PL_DestroyOptState(optstate);
                     return -1;
                 }
                 break;
diff --git a/gtests/certdb_gtest/alg1485_unittest.cc b/gtests/certdb_gtest/alg1485_unittest.cc
index b7c659414..ef6733092 100644
--- a/gtests/certdb_gtest/alg1485_unittest.cc
+++ b/gtests/certdb_gtest/alg1485_unittest.cc
@@ -10,6 +10,7 @@
 
 #include "nss.h"
 #include "scoped_ptrs.h"
+#include "prprf.h"
 
 namespace nss_test {
 
@@ -89,4 +90,23 @@ INSTANTIATE_TEST_CASE_P(ParseAVAStrings, Alg1485ParseTest,
                         ::testing::ValuesIn(kAVATestStrings));
 INSTANTIATE_TEST_CASE_P(CompareAVAStrings, Alg1485CompareTest,
                         ::testing::ValuesIn(kAVACompareStrings));
+
+TEST_F(Alg1485Test, ShortOIDTest) {
+  // This is not a valid OID (too short). CERT_GetOidString should return 0.
+  unsigned char data[] = {0x05};
+  const SECItem oid = {siBuffer, data, sizeof(data)};
+  char* result = CERT_GetOidString(&oid);
+  EXPECT_EQ(result, nullptr);
+}
+
+TEST_F(Alg1485Test, BrokenOIDTest) {
+  // This is not a valid OID (first bit of last byte is not set).
+  // CERT_GetOidString should return 0.
+  unsigned char data[] = {0x81, 0x82, 0x83, 0x84};
+  const SECItem oid = {siBuffer, data, sizeof(data)};
+  char* result = CERT_GetOidString(&oid);
+  EXPECT_EQ(15U, strlen(result));
+  EXPECT_EQ(0, strncmp("OID.UNSUPPORTED", result, 15));
+  PR_smprintf_free(result);
+}
 }
diff --git a/lib/certdb/alg1485.c b/lib/certdb/alg1485.c
index cf3f85a3d..bab23be1c 100644
--- a/lib/certdb/alg1485.c
+++ b/lib/certdb/alg1485.c
@@ -733,6 +733,10 @@ CERT_GetOidString(const SECItem* oid)
                 break;
             }
         }
+        /* There's no first bit set, so this isn't valid. Bail.*/
+        if (last == stop) {
+            goto unsupported;
+        }
         bytesBeforeLast = (unsigned int)(last - first);
         if (bytesBeforeLast <= 3U) { /* 0-28 bit number */
             PRUint32 n = 0;
@@ -756,8 +760,9 @@ CERT_GetOidString(const SECItem* oid)
                     n |= last[0] & 0x7f;
                     break;
             }
-            if (last[0] & 0x80)
+            if (last[0] & 0x80) {
                 goto unsupported;
+            }
 
             if (!rvString) {
                 /* This is the first number.. decompose it */