Bug 1334054 - fix CERT_FormatName output buffer length calculation r=franziskus
authorDavid Keeler <dkeeler@mozilla.com>
Wed, 10 May 2017 06:47:34 -0700
changeset 13340 7c60f66743b60cd0f301b138fec7f798dd4cd7f4
parent 13339 159f96115053bf495a13660ce43ec984e5ff826f
child 13341 44595eb0471ab460062b978bebf36144af8cb1ad
push id2182
push userfranziskuskiefer@gmail.com
push dateWed, 10 May 2017 16:34:36 +0000
reviewersfranziskus
bugs1334054
Bug 1334054 - fix CERT_FormatName output buffer length calculation r=franziskus Summary: Before this patch, CERT_FormatName attempted to account for the length of the additional formatting in its output buffer length, but added an insufficient amount (a fixed 128 bytes). This patch dynamically accounts for the additional space required by the formatting output (it can over-account in some cases, but this is unlikely to be a performance concern compared to the original implementation). Reviewers: franziskus Differential Revision: https://nss-review.dev.mozaws.net/D307
gtests/certhigh_gtest/Makefile
gtests/certhigh_gtest/certhigh_gtest.gyp
gtests/certhigh_gtest/certhigh_unittest.cc
gtests/certhigh_gtest/manifest.mn
gtests/manifest.mn
lib/certhigh/certhtml.c
nss.gyp
tests/gtests/gtests.sh
copy from gtests/der_gtest/Makefile
copy to gtests/certhigh_gtest/Makefile
copy from gtests/der_gtest/der_gtest.gyp
copy to gtests/certhigh_gtest/certhigh_gtest.gyp
--- a/gtests/der_gtest/der_gtest.gyp
+++ b/gtests/certhigh_gtest/certhigh_gtest.gyp
@@ -3,22 +3,20 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 {
   'includes': [
     '../../coreconf/config.gypi',
     '../common/gtest.gypi',
   ],
   'targets': [
     {
-      'target_name': 'der_gtest',
+      'target_name': 'certhigh_gtest',
       'type': 'executable',
       'sources': [
-        'der_getint_unittest.cc',
-        'der_private_key_import_unittest.cc',
-        'der_quickder_unittest.cc',
+        'certhigh_unittest.cc',
         '<(DEPTH)/gtests/common/gtests.cc'
       ],
       'dependencies': [
         '<(DEPTH)/exports.gyp:nss_exports',
         '<(DEPTH)/gtests/google_test/google_test.gyp:gtest',
         '<(DEPTH)/lib/util/util.gyp:nssutil3',
         '<(DEPTH)/lib/ssl/ssl.gyp:ssl3',
         '<(DEPTH)/lib/nss/nss.gyp:nss3',
new file mode 100644
--- /dev/null
+++ b/gtests/certhigh_gtest/certhigh_unittest.cc
@@ -0,0 +1,59 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include <string>
+
+#include "gtest/gtest.h"
+
+#include "cert.h"
+#include "certt.h"
+#include "secitem.h"
+
+namespace nss_test {
+
+class CERT_FormatNameUnitTest : public ::testing::Test {};
+
+TEST_F(CERT_FormatNameUnitTest, Overflow) {
+  // Construct a CERTName consisting of a single RDN with 20 organizational unit
+  // AVAs and 20 domain component AVAs. The actual contents don't matter, just
+  // the types.
+
+  uint8_t oidValueBytes[] = {0x0c, 0x02, 0x58, 0x58};  // utf8String "XX"
+  SECItem oidValue = {siBuffer, oidValueBytes, sizeof(oidValueBytes)};
+  uint8_t oidTypeOUBytes[] = {0x55, 0x04, 0x0b};  // organizationalUnit
+  SECItem oidTypeOU = {siBuffer, oidTypeOUBytes, sizeof(oidTypeOUBytes)};
+  CERTAVA ouAVA = {oidTypeOU, oidValue};
+  uint8_t oidTypeDCBytes[] = {0x09, 0x92, 0x26, 0x89, 0x93,
+                              0xf2, 0x2c, 0x64, 0x1,  0x19};  // domainComponent
+  SECItem oidTypeDC = {siBuffer, oidTypeDCBytes, sizeof(oidTypeDCBytes)};
+  CERTAVA dcAVA = {oidTypeDC, oidValue};
+
+  const int kNumEachAVA = 20;
+  CERTAVA* avas[(2 * kNumEachAVA) + 1];
+  for (int i = 0; i < kNumEachAVA; i++) {
+    avas[2 * i] = &ouAVA;
+    avas[(2 * i) + 1] = &dcAVA;
+  }
+  avas[2 * kNumEachAVA] = nullptr;
+
+  CERTRDN rdn = {avas};
+  CERTRDN* rdns[2];
+  rdns[0] = &rdn;
+  rdns[1] = nullptr;
+
+  std::string expectedResult =
+      "XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>"
+      "XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>"
+      "XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>XX<br>"
+      "XX<br>XX<br>XX<br>XX<br>";
+
+  CERTName name = {nullptr, rdns};
+  char* result = CERT_FormatName(&name);
+  EXPECT_EQ(expectedResult, result);
+  PORT_Free(result);
+}
+
+}  // namespace nss_test
copy from gtests/der_gtest/manifest.mn
copy to gtests/certhigh_gtest/manifest.mn
--- a/gtests/der_gtest/manifest.mn
+++ b/gtests/certhigh_gtest/manifest.mn
@@ -2,23 +2,21 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 CORE_DEPTH = ../..
 DEPTH      = ../..
 MODULE = nss
 
 CPPSRCS = \
-      der_getint_unittest.cc \
-      der_private_key_import_unittest.cc \
-      der_quickder_unittest.cc \
+      certhigh_unittest.cc \
       $(NULL)
 
 INCLUDES += -I$(CORE_DEPTH)/gtests/google_test/gtest/include \
             -I$(CORE_DEPTH)/gtests/common \
             -I$(CORE_DEPTH)/cpputil
 
 REQUIRES = nspr nss libdbm gtest
 
-PROGRAM = der_gtest
+PROGRAM = certhigh_gtest
 
 EXTRA_LIBS = $(DIST)/lib/$(LIB_PREFIX)gtest.$(LIB_SUFFIX) $(EXTRA_OBJS) \
              ../common/$(OBJDIR)/gtests$(OBJ_SUFFIX)
--- a/gtests/manifest.mn
+++ b/gtests/manifest.mn
@@ -3,14 +3,15 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 CORE_DEPTH = ..
 DEPTH      = ..
 
 DIRS = \
 	google_test \
 	common \
+	certhigh_gtest \
 	der_gtest \
 	util_gtest \
 	pk11_gtest \
 	ssl_gtest \
         nss_bogo_shim \
 	$(NULL)
--- a/lib/certhigh/certhtml.c
+++ b/lib/certhigh/certhtml.c
@@ -97,104 +97,125 @@ CERT_FormatName(CERTName *name)
                     if (cn) {
                         break;
                     }
                     cn = CERT_DecodeAVAValue(&ava->value);
                     if (!cn) {
                         goto loser;
                     }
                     len += cn->len;
+                    // cn will always have BREAK after it
+                    len += BREAKLEN;
                     break;
                 case SEC_OID_AVA_COUNTRY_NAME:
                     if (country) {
                         break;
                     }
                     country = CERT_DecodeAVAValue(&ava->value);
                     if (!country) {
                         goto loser;
                     }
                     len += country->len;
+                    // country may have COMMA after it (if we over-count len,
+                    // that's fine - we'll just allocate a buffer larger than we
+                    // need)
+                    len += COMMALEN;
                     break;
                 case SEC_OID_AVA_LOCALITY:
                     if (loc) {
                         break;
                     }
                     loc = CERT_DecodeAVAValue(&ava->value);
                     if (!loc) {
                         goto loser;
                     }
                     len += loc->len;
+                    // loc may have COMMA after it
+                    len += COMMALEN;
                     break;
                 case SEC_OID_AVA_STATE_OR_PROVINCE:
                     if (state) {
                         break;
                     }
                     state = CERT_DecodeAVAValue(&ava->value);
                     if (!state) {
                         goto loser;
                     }
                     len += state->len;
+                    // state currently won't have COMMA after it, but this is a
+                    // (probably vain) attempt to future-proof this code
+                    len += COMMALEN;
                     break;
                 case SEC_OID_AVA_ORGANIZATION_NAME:
                     if (org) {
                         break;
                     }
                     org = CERT_DecodeAVAValue(&ava->value);
                     if (!org) {
                         goto loser;
                     }
                     len += org->len;
+                    // org will have BREAK after it
+                    len += BREAKLEN;
                     break;
                 case SEC_OID_AVA_DN_QUALIFIER:
                     if (dq) {
                         break;
                     }
                     dq = CERT_DecodeAVAValue(&ava->value);
                     if (!dq) {
                         goto loser;
                     }
                     len += dq->len;
+                    // dq will have BREAK after it
+                    len += BREAKLEN;
                     break;
                 case SEC_OID_AVA_ORGANIZATIONAL_UNIT_NAME:
                     if (ou_count < MAX_OUS) {
                         orgunit[ou_count] = CERT_DecodeAVAValue(&ava->value);
                         if (!orgunit[ou_count]) {
                             goto loser;
                         }
                         len += orgunit[ou_count++]->len;
+                        // each ou will have BREAK after it
+                        len += BREAKLEN;
                     }
                     break;
                 case SEC_OID_AVA_DC:
                     if (dc_count < MAX_DC) {
                         dc[dc_count] = CERT_DecodeAVAValue(&ava->value);
                         if (!dc[dc_count]) {
                             goto loser;
                         }
                         len += dc[dc_count++]->len;
+                        // each dc will have BREAK after it
+                        len += BREAKLEN;
                     }
                     break;
                 case SEC_OID_PKCS9_EMAIL_ADDRESS:
                 case SEC_OID_RFC1274_MAIL:
                     if (email) {
                         break;
                     }
                     email = CERT_DecodeAVAValue(&ava->value);
                     if (!email) {
                         goto loser;
                     }
                     len += email->len;
+                    // email will have BREAK after it
+                    len += BREAKLEN;
                     break;
                 default:
                     break;
             }
         }
     }
 
-    /* XXX - add some for formatting */
-    len += 128;
+    // there may be a final BREAK
+    len += BREAKLEN;
 
     /* allocate buffer */
     buf = (char *)PORT_Alloc(len);
     if (!buf) {
         goto loser;
     }
 
     tmpbuf = buf;
--- a/nss.gyp
+++ b/nss.gyp
@@ -172,16 +172,17 @@
             'cmd/tests/tests.gyp:dertimetest',
             'cmd/tests/tests.gyp:encodeinttest',
             'cmd/tests/tests.gyp:nonspr10',
             'cmd/tests/tests.gyp:remtest',
             'cmd/tests/tests.gyp:secmodtest',
             'cmd/tstclnt/tstclnt.gyp:tstclnt',
             'cmd/vfychain/vfychain.gyp:vfychain',
             'cmd/vfyserv/vfyserv.gyp:vfyserv',
+            'gtests/certhigh_gtest/certhigh_gtest.gyp:certhigh_gtest',
             'gtests/der_gtest/der_gtest.gyp:der_gtest',
             'gtests/freebl_gtest/freebl_gtest.gyp:prng_gtest',
             'gtests/pk11_gtest/pk11_gtest.gyp:pk11_gtest',
             'gtests/ssl_gtest/ssl_gtest.gyp:ssl_gtest',
             'gtests/util_gtest/util_gtest.gyp:util_gtest',
             'gtests/nss_bogo_shim/nss_bogo_shim.gyp:nss_bogo_shim',
           ],
           'conditions': [
--- a/tests/gtests/gtests.sh
+++ b/tests/gtests/gtests.sh
@@ -78,13 +78,13 @@ gtest_start()
 gtest_cleanup()
 {
   html "</TABLE><BR>"
   cd ${QADIR}
   . common/cleanup.sh
 }
 
 ################## main #################################################
-GTESTS="prng_gtest der_gtest pk11_gtest util_gtest freebl_gtest"
+GTESTS="prng_gtest certhigh_gtest der_gtest pk11_gtest util_gtest freebl_gtest"
 SOURCE_DIR="$PWD"/../..
 gtest_init $0
 gtest_start
 gtest_cleanup