Bug 1533216 - check we actually got some certs in collect_certs r=jcj

Test Plan: https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=1b44b7588cb9a6806701394c08bf64d13f84b982 Reviewers: jcj Reviewed By: jcj Bug #: 1533216 Differential Revision: https://phabricator.services.mozilla.com/D23752 --HG-- extra : rebase_source : 85b6afa770ee9085482322add6b6747aa5fc9016 extra : histedit_source : 3a11df58e7238bfd0d75a82b807cdf7e351cefb6
sailfishos-mirror · Mar 20, 2019 · 4201b8a · 4201b8a
1 parent efdfa39
commit 4201b8a
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 8 deletions.
diff --git a/gtests/certdb_gtest/certdb_gtest.gyp b/gtests/certdb_gtest/certdb_gtest.gyp
@@ -12,6 +12,7 @@
       'type': 'executable',
       'sources': [
         'alg1485_unittest.cc',
+        'decode_certs_unittest.cc',
         '<(DEPTH)/gtests/common/gtests.cc'
       ],
       'dependencies': [
@@ -20,6 +21,7 @@
         '<(DEPTH)/lib/util/util.gyp:nssutil3',
         '<(DEPTH)/lib/ssl/ssl.gyp:ssl3',
         '<(DEPTH)/lib/nss/nss.gyp:nss3',
+        '<(DEPTH)/lib/smime/smime.gyp:smime3',
       ]
     }
   ],

diff --git a/gtests/certdb_gtest/decode_certs_unittest.cc b/gtests/certdb_gtest/decode_certs_unittest.cc
@@ -0,0 +1,28 @@
+/* -*- 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 "gtest/gtest.h"
+
+#include "cert.h"
+#include "prerror.h"
+#include "secerr.h"
+
+class DecodeCertsTest : public ::testing::Test {};
+
+TEST_F(DecodeCertsTest, EmptyCertPackage) {
+  // This represents a PKCS#7 ContentInfo with a contentType of
+  // '2.16.840.1.113730.2.5' (Netscape data-type cert-sequence) and a content
+  // consisting of an empty SEQUENCE. This is valid ASN.1, but it contains no
+  // certificates, so CERT_DecodeCertFromPackage should just return a null
+  // pointer.
+  unsigned char emptyCertPackage[] = {0x30, 0x0f, 0x06, 0x09, 0x60, 0x86,
+                                      0x48, 0x01, 0x86, 0xf8, 0x42, 0x02,
+                                      0x05, 0xa0, 0x02, 0x30, 0x00};
+  EXPECT_EQ(nullptr, CERT_DecodeCertFromPackage(
+                         reinterpret_cast<char*>(emptyCertPackage),
+                         sizeof(emptyCertPackage)));
+  EXPECT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
+}
diff --git a/gtests/certdb_gtest/manifest.mn b/gtests/certdb_gtest/manifest.mn
@@ -8,6 +8,7 @@ MODULE = nss
 
 CPPSRCS = \
       alg1485_unittest.cc \
+      decode_certs_unittest.cc \
       $(NULL)
 
 INCLUDES += -I$(CORE_DEPTH)/gtests/google_test/gtest/include \

diff --git a/lib/pkcs7/certread.c b/lib/pkcs7/certread.c
@@ -492,14 +492,16 @@ typedef struct {
 static SECStatus
 collect_certs(void *arg, SECItem **certs, int numcerts)
 {
-    SECStatus rv;
-    collect_args *collectArgs;
-
-    collectArgs = (collect_args *)arg;
-
-    rv = SECITEM_CopyItem(collectArgs->arena, &collectArgs->cert, *certs);
-
-    return (rv);
+    collect_args *collectArgs = (collect_args *)arg;
+    if (!collectArgs || !collectArgs->arena) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return SECFailure;
+    }
+    if (numcerts < 1 || !certs || !*certs) {
+        PORT_SetError(SEC_ERROR_BAD_DER);
+        return SECFailure;
+    }
+    return SECITEM_CopyItem(collectArgs->arena, &collectArgs->cert, *certs);
 }
 
 /*