Skip to content
Permalink
Browse files
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

--HG--
rename : gtests/der_gtest/Makefile => gtests/certhigh_gtest/Makefile
rename : gtests/der_gtest/der_gtest.gyp => gtests/certhigh_gtest/certhigh_gtest.gyp
rename : gtests/der_gtest/manifest.mn => gtests/certhigh_gtest/manifest.mn
extra : rebase_source : 1fb5cbf1c77018e6d7f9f9aed0f3d9a3b33f4909
  • Loading branch information
mozkeeler committed May 10, 2017
1 parent d2ec59c commit 226916905b3c0fcbdb4fb22b11190d943085ef0c
@@ -0,0 +1,43 @@
#! gmake
#
# 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/.

#######################################################################
# (1) Include initial platform-independent assignments (MANDATORY). #
#######################################################################

include manifest.mn

#######################################################################
# (2) Include "global" configuration information. (OPTIONAL) #
#######################################################################

include $(CORE_DEPTH)/coreconf/config.mk

#######################################################################
# (3) Include "component" configuration information. (OPTIONAL) #
#######################################################################


#######################################################################
# (4) Include "local" platform-dependent assignments (OPTIONAL). #
#######################################################################

include ../common/gtest.mk

#######################################################################
# (5) Execute "global" rules. (OPTIONAL) #
#######################################################################

include $(CORE_DEPTH)/coreconf/rules.mk

#######################################################################
# (6) Execute "component" rules. (OPTIONAL) #
#######################################################################


#######################################################################
# (7) Execute "local" rules. (OPTIONAL). #
#######################################################################
@@ -0,0 +1,29 @@
# 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/.
{
'includes': [
'../../coreconf/config.gypi',
'../common/gtest.gypi',
],
'targets': [
{
'target_name': 'certhigh_gtest',
'type': 'executable',
'sources': [
'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',
]
}
],
'variables': {
'module': 'nss'
}
}
@@ -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
@@ -0,0 +1,22 @@
#
# 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 = \
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 = certhigh_gtest

EXTRA_LIBS = $(DIST)/lib/$(LIB_PREFIX)gtest.$(LIB_SUFFIX) $(EXTRA_OBJS) \
../common/$(OBJDIR)/gtests$(OBJ_SUFFIX)
@@ -8,6 +8,7 @@ DEPTH = ..
DIRS = \
google_test \
common \
certhigh_gtest \
der_gtest \
util_gtest \
pk11_gtest \
@@ -102,6 +102,8 @@ CERT_FormatName(CERTName *name)
goto loser;
}
len += cn->len;
// cn will always have BREAK after it
len += BREAKLEN;
break;
case SEC_OID_AVA_COUNTRY_NAME:
if (country) {
@@ -112,6 +114,10 @@ CERT_FormatName(CERTName *name)
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) {
@@ -122,6 +128,8 @@ CERT_FormatName(CERTName *name)
goto loser;
}
len += loc->len;
// loc may have COMMA after it
len += COMMALEN;
break;
case SEC_OID_AVA_STATE_OR_PROVINCE:
if (state) {
@@ -132,6 +140,9 @@ CERT_FormatName(CERTName *name)
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) {
@@ -142,6 +153,8 @@ CERT_FormatName(CERTName *name)
goto loser;
}
len += org->len;
// org will have BREAK after it
len += BREAKLEN;
break;
case SEC_OID_AVA_DN_QUALIFIER:
if (dq) {
@@ -152,6 +165,8 @@ CERT_FormatName(CERTName *name)
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) {
@@ -160,6 +175,8 @@ CERT_FormatName(CERTName *name)
goto loser;
}
len += orgunit[ou_count++]->len;
// each ou will have BREAK after it
len += BREAKLEN;
}
break;
case SEC_OID_AVA_DC:
@@ -169,6 +186,8 @@ CERT_FormatName(CERTName *name)
goto loser;
}
len += dc[dc_count++]->len;
// each dc will have BREAK after it
len += BREAKLEN;
}
break;
case SEC_OID_PKCS9_EMAIL_ADDRESS:
@@ -181,15 +200,17 @@ CERT_FormatName(CERTName *name)
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);
@@ -177,6 +177,7 @@
'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',
@@ -83,7 +83,7 @@ gtest_cleanup()
}

################## 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

0 comments on commit 2269169

Please sign in to comment.