Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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 2269169
Show file tree
Hide file tree
Showing 8 changed files with 179 additions and 3 deletions.
43 changes: 43 additions & 0 deletions gtests/certhigh_gtest/Makefile
@@ -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). #
#######################################################################
29 changes: 29 additions & 0 deletions gtests/certhigh_gtest/certhigh_gtest.gyp
@@ -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'
}
}
59 changes: 59 additions & 0 deletions 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
22 changes: 22 additions & 0 deletions gtests/certhigh_gtest/manifest.mn
@@ -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)
1 change: 1 addition & 0 deletions gtests/manifest.mn
Expand Up @@ -8,6 +8,7 @@ DEPTH = ..
DIRS = \
google_test \
common \
certhigh_gtest \
der_gtest \
util_gtest \
pk11_gtest \
Expand Down
25 changes: 23 additions & 2 deletions lib/certhigh/certhtml.c
Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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:
Expand All @@ -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:
Expand All @@ -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);
Expand Down
1 change: 1 addition & 0 deletions nss.gyp
Expand Up @@ -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',
Expand Down
2 changes: 1 addition & 1 deletion tests/gtests/gtests.sh
Expand Up @@ -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
Expand Down

0 comments on commit 2269169

Please sign in to comment.