Bug 1515342 - More thorough input checking, r=jcj
authorMartin Thomson <mt@lowentropy.net>
Fri, 14 Jun 2019 07:55:51 +0000
changeset 15174 dabfe1160c682b4d1d19c5a7a13ab3828bb9d37f
parent 15173 6cfb54d262d030783137aa6478b45ecb3cbfc624
child 15175 efd44c8a6c6330fd5752f57d8d8b627aa3b36b43
push id3407
push userjjones@mozilla.com
push dateFri, 21 Jun 2019 18:15:06 +0000
reviewersjcj
bugs1515342
Bug 1515342 - More thorough input checking, r=jcj All part of applying better discipline throughout. Differential Revision: https://phabricator.services.mozilla.com/D33736
lib/cryptohi/seckey.c
lib/freebl/dh.c
lib/freebl/ec.c
lib/util/quickder.c
--- a/lib/cryptohi/seckey.c
+++ b/lib/cryptohi/seckey.c
@@ -634,16 +634,21 @@ seckey_ExtractPublicKey(const CERTSubjec
 
                 rv = SEC_QuickDERDecodeItem(arena, pubk, SECKEY_DHParamKeyTemplate,
                                             &newParms);
 
                 if (rv == SECSuccess)
                     return pubk;
                 break;
             case SEC_OID_ANSIX962_EC_PUBLIC_KEY:
+                /* A basic sanity check on inputs. */
+                if (spki->algorithm.parameters.len == 0 || newOs.len == 0) {
+                    PORT_SetError(SEC_ERROR_INPUT_LEN);
+                    break;
+                }
                 pubk->keyType = ecKey;
                 pubk->u.ec.size = 0;
 
                 /* Since PKCS#11 directly takes the DER encoding of EC params
                  * and public value, we don't need any decoding here.
                  */
                 rv = SECITEM_CopyItem(arena, &pubk->u.ec.DEREncodedParams,
                                       &spki->algorithm.parameters);
--- a/lib/freebl/dh.c
+++ b/lib/freebl/dh.c
@@ -205,17 +205,18 @@ DH_Derive(SECItem *publicValue,
           SECItem *derivedSecret,
           unsigned int outBytes)
 {
     mp_int p, Xa, Yb, ZZ, psub1;
     mp_err err = MP_OKAY;
     unsigned int len = 0;
     unsigned int nb;
     unsigned char *secret = NULL;
-    if (!publicValue || !prime || !privateValue || !derivedSecret) {
+    if (!publicValue || !publicValue->len || !prime || !prime->len ||
+        !privateValue || !privateValue->len || !derivedSecret) {
         PORT_SetError(SEC_ERROR_INVALID_ARGS);
         return SECFailure;
     }
     memset(derivedSecret, 0, sizeof *derivedSecret);
     MP_DIGITS(&p) = 0;
     MP_DIGITS(&Xa) = 0;
     MP_DIGITS(&Yb) = 0;
     MP_DIGITS(&ZZ) = 0;
--- a/lib/freebl/ec.c
+++ b/lib/freebl/ec.c
@@ -197,18 +197,18 @@ ec_NewKey(ECParams *ecParams, ECPrivateK
     mp_err err = MP_OKAY;
     int len;
 
 #if EC_DEBUG
     printf("ec_NewKey called\n");
 #endif
     MP_DIGITS(&k) = 0;
 
-    if (!ecParams || !privKey || !privKeyBytes || (privKeyLen < 0) ||
-        !ecParams->name) {
+    if (!ecParams || ecParams->name == ECCurve_noName ||
+        !privKey || !privKeyBytes || privKeyLen <= 0) {
         PORT_SetError(SEC_ERROR_INVALID_ARGS);
         return SECFailure;
     }
 
     /* Initialize an arena for the EC key. */
     if (!(arena = PORT_NewArena(NSS_FREEBL_DEFAULT_CHUNKSIZE)))
         return SECFailure;
 
@@ -386,17 +386,17 @@ cleanup:
  */
 SECStatus
 EC_NewKey(ECParams *ecParams, ECPrivateKey **privKey)
 {
     SECStatus rv = SECFailure;
     int len;
     unsigned char *privKeyBytes = NULL;
 
-    if (!ecParams) {
+    if (!ecParams || ecParams->name == ECCurve_noName || !privKey) {
         PORT_SetError(SEC_ERROR_INVALID_ARGS);
         return SECFailure;
     }
 
     len = ecParams->order.len;
     privKeyBytes = ec_GenerateRandomPrivateKey(ecParams->order.data, len);
     if (privKeyBytes == NULL)
         goto cleanup;
@@ -425,17 +425,18 @@ SECStatus
 EC_ValidatePublicKey(ECParams *ecParams, SECItem *publicValue)
 {
     mp_int Px, Py;
     ECGroup *group = NULL;
     SECStatus rv = SECFailure;
     mp_err err = MP_OKAY;
     int len;
 
-    if (!ecParams || !publicValue || !ecParams->name) {
+    if (!ecParams || ecParams->name == ECCurve_noName ||
+        !publicValue || !publicValue->len) {
         PORT_SetError(SEC_ERROR_INVALID_ARGS);
         return SECFailure;
     }
 
     /* Uses curve specific code for point validation. */
     if (ecParams->fieldID.type == ec_field_plain) {
         const ECMethod *method = ec_get_method_from_name(ecParams->name);
         if (method == NULL || method->validate == NULL) {
@@ -531,18 +532,19 @@ ECDH_Derive(SECItem *publicValue,
     SECItem pointQ = { siBuffer, NULL, 0 };
     mp_int k; /* to hold the private value */
     mp_int cofactor;
     mp_err err = MP_OKAY;
 #if EC_DEBUG
     int i;
 #endif
 
-    if (!publicValue || !ecParams || !privateValue || !derivedSecret ||
-        !ecParams->name) {
+    if (!publicValue || !publicValue->len ||
+        !ecParams || ecParams->name == ECCurve_noName ||
+        !privateValue || !privateValue->len || !derivedSecret) {
         PORT_SetError(SEC_ERROR_INVALID_ARGS);
         return SECFailure;
     }
 
     /*
      * Make sure the point is on the requested curve to avoid
      * certain small subgroup attacks.
      */
--- a/lib/util/quickder.c
+++ b/lib/util/quickder.c
@@ -752,16 +752,23 @@ DecodeItem(void* dest,
                                     temp.data++;
                                     temp.len--;
                                 }
                             }
                             break;
                         }
 
                         case SEC_ASN1_BIT_STRING: {
+                            /* Can't be 8 or more spare bits, or any spare bits
+			     * if there are no octets. */
+                            if (temp.data[0] >= 8 || (temp.data[0] > 0 && temp.len == 1)) {
+                                PORT_SetError(SEC_ERROR_BAD_DER);
+                                rv = SECFailure;
+                                break;
+                            }
                             /* change the length in the SECItem to be the number
                                of bits */
                             temp.len = (temp.len - 1) * 8 - (temp.data[0] & 0x7);
                             temp.data++;
                             break;
                         }
 
                         default: {