Engine cleanup and hardening

src/we_aes_block.c

@@ -248,7 +248,7 @@ static int we_aes_cbc_ctrl(EVP_CIPHER_CTX *ctx, int type, int arg, void *ptr)
 
     /* Get the AES-CBC data to work with. */
     aes = (we_AesBlock *)EVP_CIPHER_CTX_get_cipher_data(ctx);
-    if (aes != NULL) {
+    if (aes == NULL) {
         WOLFENGINE_ERROR_FUNC_NULL(WE_LOG_CIPHER,
                                    "EVP_CIPHER_CTX_get_cipher_data", aes);
         ret = 0;
@@ -428,14 +428,24 @@ static int we_aes_ecb_init(EVP_CIPHER_CTX *ctx, const unsigned char *key,
         ret = 0;
     }
 
-    if ((ret == 1) && (key == NULL)) {
+    if ((ret == 1) && (key != NULL)) {
         WOLFENGINE_MSG(WE_LOG_CIPHER,
                        "Initializing wolfCrypt Aes structure: %p", &aes->aes);
         rc = wc_AesInit(&aes->aes, NULL, INVALID_DEVID);
         if (rc != 0) {
             WOLFENGINE_ERROR_FUNC(WE_LOG_CIPHER, "wc_AesInit", rc);
             ret = 0;
         }
+        if (ret == 1) {
+            WOLFENGINE_MSG(WE_LOG_CIPHER, "Setting AES key (%d bytes)",
+                           EVP_CIPHER_CTX_key_length(ctx));
+            rc = wc_AesSetKey(&aes->aes, key, EVP_CIPHER_CTX_key_length(ctx),
+                              NULL, enc ? AES_ENCRYPTION : AES_DECRYPTION);
+            if (rc != 0) {
+                WOLFENGINE_ERROR_FUNC(WE_LOG_CIPHER, "wc_AesSetKey", rc);
+                ret = 0;
+            }
+        }
     }
 
     if (ret == 1) {
@@ -444,17 +454,6 @@ static int we_aes_ecb_init(EVP_CIPHER_CTX *ctx, const unsigned char *key,
         aes->enc = enc;
     }
 
-    if ((ret == 1) && (key != NULL)) {
-        WOLFENGINE_MSG(WE_LOG_CIPHER, "Setting AES key (%d bytes)",
-                       EVP_CIPHER_CTX_key_length(ctx));
-        rc = wc_AesSetKey(&aes->aes, key, EVP_CIPHER_CTX_key_length(ctx),
-                          NULL, enc ? AES_ENCRYPTION : AES_DECRYPTION);
-        if (rc != 0) {
-            WOLFENGINE_ERROR_FUNC(WE_LOG_CIPHER, "wc_AesSetKey", rc);
-            ret = 0;
-        }
-    }
-
     WOLFENGINE_LEAVE(WE_LOG_CIPHER, "we_aes_ecb_init", ret);
 
     return ret;
@@ -732,7 +731,7 @@ static int we_aes_ecb_ctrl(EVP_CIPHER_CTX *ctx, int type, int arg, void *ptr)
 
     /* Get the AES-ECB data to work with. */
     aes = (we_AesBlock *)EVP_CIPHER_CTX_get_cipher_data(ctx);
-    if (aes != NULL) {
+    if (aes == NULL) {
         WOLFENGINE_ERROR_FUNC_NULL(WE_LOG_CIPHER,
                                    "EVP_CIPHER_CTX_get_cipher_data", aes);
         ret = 0;

src/we_aes_cbc_hmac.c

@@ -297,8 +297,14 @@ static int we_aes_cbc_hmac_dec(we_AesCbcHmac* aes, unsigned char *out,
                 ret = -1;
         }
         if (ret != -1) {
-            /* Remove padding and MAC length. */
-            ret -= pb + 1 + SHA256_DIGEST_LENGTH;
+            /* Record must hold the padding and MAC or the length underflows. */
+            if (ret < pb + 1 + SHA256_DIGEST_LENGTH) {
+                ret = -1;
+            }
+            else {
+                /* Remove padding and MAC length. */
+                ret -= pb + 1 + SHA256_DIGEST_LENGTH;
+            }
         }
 
         /* Update record header to have correct message length. */
@@ -577,10 +583,10 @@ int we_init_aescbc_hmac_meths(void)
     /* AES128-CBC HMAC-SHA256 */
     we_aes128_cbc_hmac_ciph = EVP_CIPHER_meth_new(NID_aes_128_cbc_hmac_sha256,
         WC_AES_BLOCK_SIZE, AES_128_KEY_SIZE);
-    if (we_aes128_cbc_ciph == NULL) {
+    if (we_aes128_cbc_hmac_ciph == NULL) {
         WOLFENGINE_ERROR_FUNC_NULL(WE_LOG_CIPHER,
                                    "EVP_CIPHER_meth_new - AES-128-CBC "
-                                   "HMAC SHA256", we_aes128_cbc_ciph);
+                                   "HMAC SHA256", we_aes128_cbc_hmac_ciph);
         ret = 0;
     }
     if (ret == 1) {
@@ -591,10 +597,10 @@ int we_init_aescbc_hmac_meths(void)
     if (ret == 1) {
         we_aes256_cbc_hmac_ciph = EVP_CIPHER_meth_new(
             NID_aes_256_cbc_hmac_sha256, WC_AES_BLOCK_SIZE, AES_256_KEY_SIZE);
-        if (we_aes256_cbc_ciph == NULL) {
+        if (we_aes256_cbc_hmac_ciph == NULL) {
             WOLFENGINE_ERROR_FUNC_NULL(WE_LOG_CIPHER,
                                        "EVP_CIPHER_meth_new - AES-256-CBC "
-                                       "HMAC SHA256", we_aes256_cbc_ciph);
+                                       "HMAC SHA256", we_aes256_cbc_hmac_ciph);
             ret = 0;
         }
     }

src/we_aes_ctr.c

@@ -94,7 +94,7 @@ static int we_aes_ctr_init(EVP_CIPHER_CTX *ctx, const unsigned char *key,
         rc = wc_AesSetIV(&aes->aes, iv);
         if (rc != 0) {
             WOLFENGINE_ERROR_FUNC(WE_LOG_CIPHER, "wc_AesSetIV", rc);
-            ret = -1;
+            ret = 0;
         }
         else {
             /*
@@ -150,7 +150,7 @@ static int we_aes_ctr_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out,
             }
             else {
                 unsigned int num = EVP_CIPHER_CTX_num(ctx);
-                num = (num + len) % AES_128_KEY_SIZE;
+                num = (num + len) % WC_AES_BLOCK_SIZE;
                 EVP_CIPHER_CTX_set_num(ctx, num);
 
                 XMEMCPY(EVP_CIPHER_CTX_iv_noconst(ctx), aes->aes.reg,

src/we_aes_gcm.c

@@ -168,7 +168,7 @@ static int we_aes_gcm_cleanup(EVP_CIPHER_CTX *ctx)
             OPENSSL_free(aes->aad);
         }
         if (aes->tmp != NULL) {
-            OPENSSL_free(aes->tmp);
+            OPENSSL_clear_free(aes->tmp, aes->tmpLen);
             aes->tmp = NULL;
         }
         aes->tmpLen = 0;
@@ -328,19 +328,19 @@ static int we_aes_gcm_update(we_AesGcm* aes, const unsigned char* in,
                            "out = %p]", aes, in, len, out);
 
     if (len != 0 && in != NULL) {
-        aes->tmp = (unsigned char*)OPENSSL_malloc(len);
-        if (aes->tmp == NULL) {
-            WOLFENGINE_ERROR_FUNC_NULL(WE_LOG_CIPHER, "OPENSSL_malloc",
-                                       aes->tmp);
+        unsigned char *newTmp = (unsigned char*)OPENSSL_malloc(len);
+        if (newTmp == NULL) {
+            WOLFENGINE_ERROR_FUNC_NULL(WE_LOG_CIPHER, "OPENSSL_malloc", newTmp);
             ret = -1;
         }
         else {
-            /*
-             * All encryption/decryption is deferred to we_aes_gcm_final. We
-             * just save the input data and the address of the output buffer
-             * here.
-             */
-            XMEMCPY(aes->tmp, in, len);
+            /* Swap in the new buffer only on success so a failed allocation
+             * leaves the previously buffered state unchanged. */
+            if (aes->tmp != NULL) {
+                OPENSSL_clear_free(aes->tmp, aes->tmpLen);
+            }
+            XMEMCPY(newTmp, in, len);
+            aes->tmp = newTmp;
             aes->tmpLen = (int)len;
             aes->outputBuf = out;
             /* Return length of buffered input data. */
@@ -430,7 +430,7 @@ static int we_aes_gcm_final(we_AesGcm* aes)
     }
 
     if (aes->tmp != NULL) {
-        OPENSSL_free(aes->tmp);
+        OPENSSL_clear_free(aes->tmp, aes->tmpLen);
         aes->tmp = NULL;
     }
     aes->tmpLen = 0;
@@ -443,7 +443,7 @@ static int we_aes_gcm_final(we_AesGcm* aes)
 
     if (aes->ivInc) {
         int i;
-        for (i = aes->ivLen - 1; i >= aes->ivLen - 8; i--) {
+        for (i = aes->ivLen - 1; (i >= 0) && (i >= aes->ivLen - 8); i--) {
             if ((++aes->iv[i]) != 0) {
                 break;
             }
@@ -649,7 +649,8 @@ static int we_aes_gcm_ctrl(EVP_CIPHER_CTX *ctx, int type, int arg, void *ptr)
                  *   arg [in] size of generated IV/nonce
                  *   ptr [in] generated IV/nonce data
                  */
-                if ((arg <= 0) || (arg > GCM_NONCE_MAX_SZ)) {
+                if ((arg <= 0) || (arg > GCM_NONCE_MAX_SZ) || (ptr == NULL) ||
+                        (arg > aes->ivLen)) {
                     XSNPRINTF(errBuff, sizeof(errBuff), "Invalid nonce length "
                               "%d", arg);
                     WOLFENGINE_ERROR_MSG(WE_LOG_CIPHER, errBuff);

src/we_des3_cbc.c

@@ -280,7 +280,7 @@ static int we_des3_cbc_ctrl(EVP_CIPHER_CTX *ctx, int type, int arg, void *ptr)
 
     /* Get the DES3-CBC data to work with. */
     des3 = (we_Des3Cbc *)EVP_CIPHER_CTX_get_cipher_data(ctx);
-    if (des3 != NULL) {
+    if (des3 == NULL) {
         WOLFENGINE_ERROR_FUNC_NULL(WE_LOG_CIPHER,
                                    "EVP_CIPHER_CTX_get_cipher_data", des3);
         ret = 0;

src/we_dh.c

@@ -349,6 +349,8 @@ static int we_dh_generate_key_int(DH *dh, we_Dh *engineDh)
     unsigned int pubLen = 0;
     BIGNUM *privBn = NULL;
     BIGNUM *pubBn = NULL;
+    unsigned char *gBuf = NULL;
+    int gBufLen = 0;
 #ifndef WE_DH_USE_GLOBAL_RNG
     WC_RNG *pRng = &engineDh->rng;
 #else
@@ -397,9 +399,6 @@ static int we_dh_generate_key_int(DH *dh, we_Dh *engineDh)
     if (ret == 1) {
         /* Check if private key already set. */
         if ((privBn = (BIGNUM *)DH_get0_priv_key(dh)) != NULL) {
-            unsigned char *gBuf;
-            int gBufLen;
-
             /* Get private key into buffer. */
             privLen = BN_bn2bin(privBn, priv);
             /* Get generator into buffer. */
@@ -494,6 +493,9 @@ static int we_dh_generate_key_int(DH *dh, we_Dh *engineDh)
     if (pub != NULL) {
         OPENSSL_free(pub);
     }
+    if (gBuf != NULL) {
+        OPENSSL_free(gBuf);
+    }
 
     WOLFENGINE_LEAVE(WE_LOG_KE, "we_dh_generate_key_int", ret);
 
@@ -1258,7 +1260,11 @@ static int we_dh_pkey_paramgen(EVP_PKEY_CTX *ctx, EVP_PKEY *pkey)
 
     if (ret == 1) {
         /* Assign OpenSSL DH object to PKEY. */
-        EVP_PKEY_assign_DH(pkey, dh);
+        if (EVP_PKEY_assign_DH(pkey, dh) != 1) {
+            WOLFENGINE_ERROR_FUNC(WE_LOG_KE, "EVP_PKEY_assign_DH", 0);
+            DH_free(dh);
+            ret = 0;
+        }
     }
 
     if (ret == 1) {
@@ -1313,6 +1319,7 @@ static int we_dh_pkey_keygen(EVP_PKEY_CTX *ctx, EVP_PKEY *pkey)
         ret = EVP_PKEY_assign_DH(pkey, dh);
         if (ret != 1) {
             WOLFENGINE_ERROR_FUNC(WE_LOG_KE, "EVP_PKEY_assign_DH", ret);
+            DH_free(dh);
         }
     }
 

src/we_digest.c

@@ -981,9 +981,15 @@ static int we_init_digest_meth(EVP_MD *method)
     if (ret == 1) {
         const int *nids;
         int cnt;
+        int maxCnt;
 
+        maxCnt = (int)(sizeof(method->required_pkey_type) /
+                       sizeof(method->required_pkey_type[0]));
         cnt = we_pkey_get_nids(&nids);
-        XMEMCPY(method->required_pkey_type, nids, cnt);
+        if (cnt > maxCnt) {
+            cnt = maxCnt;
+        }
+        XMEMCPY(method->required_pkey_type, nids, cnt * sizeof(*nids));
         method->flags |= EVP_MD_FLAG_PKEY_METHOD_SIGNATURE;
     }
 #endif

src/we_ecc.c

@@ -834,8 +834,12 @@ static int we_pkey_ecdsa_verify(EVP_PKEY_CTX *ctx, const unsigned char *sig,
         }
     }
     /* wolfSSL FIPS is not checking SEQUENCE length. */
-    if ((ret == 1) && (sig[0] == 0x30)) {
-        size_t len;
+    if ((ret == 1) && ((sig == NULL) || (sigLen < 2))) {
+        WOLFENGINE_ERROR_MSG(WE_LOG_PK, "Signature too short");
+        ret = -1;
+    }
+    else if ((ret == 1) && (sig[0] == 0x30)) {
+        size_t len = 0;
         int o = 1;
 
         /* Check for indefinite length - length not specified. */
@@ -846,10 +850,17 @@ static int we_pkey_ecdsa_verify(EVP_PKEY_CTX *ctx, const unsigned char *sig,
         /* Check for multi-byte length. */
         else if (sig[o] > 0x80) {
             byte cnt = (sig[o++]) & 0x7f;
-            len = 0;
-            while ((cnt--) > 0) {
-                len <<= 8;
-                len += sig[o++];
+            /* Length bytes must be present and fit in the length variable. */
+            if ((cnt == 0) || (cnt > sizeof(len)) ||
+                    ((size_t)o + cnt > sigLen)) {
+                WOLFENGINE_ERROR_MSG(WE_LOG_PK, "Signature length invalid");
+                ret = -1;
+            }
+            else {
+                while ((cnt--) > 0) {
+                    len <<= 8;
+                    len += sig[o++];
+                }
             }
         }
         /* Length in byte. */
@@ -858,7 +869,7 @@ static int we_pkey_ecdsa_verify(EVP_PKEY_CTX *ctx, const unsigned char *sig,
         }
         /* Check signature length is:
          *     SEQUENCE header length + SQUENCE data length */
-        if ((ret == 1) && (o + len != sigLen)) {
+        if ((ret == 1) && ((size_t)o + len != sigLen)) {
             WOLFENGINE_ERROR_MSG(WE_LOG_PK, "Signature length invalid");
             ret = -1;
         }
@@ -2106,14 +2117,6 @@ static ECDSA_SIG* we_ecdsa_do_sign_ex(const unsigned char *d, int dlen,
         }
     }
 
-    if (err == 0) {
-        rc = mp_init_multi(&sig_r, &sig_s, NULL, NULL, NULL, NULL);
-        if (rc != MP_OKAY) {
-            WOLFENGINE_ERROR_FUNC(WE_LOG_PK, "mp_init_multi", rc);
-            err = 1;
-        }
-    }
-
     /* Sign hash with ECDSA */
     if (err == 0) {
 #if defined(WE_ECC_USE_GLOBAL_RNG) && !defined(WE_SINGLE_THREADED)

src/we_hkdf.c

@@ -101,7 +101,7 @@ static void we_hkdf_cleanup(EVP_PKEY_CTX *ctx)
         }
         /* Clear and free salt. */
         if (hkdf->salt != NULL) {
-            OPENSSL_free(hkdf->salt);
+            OPENSSL_clear_free(hkdf->salt, hkdf->saltSz);
         }
         /* Clear info - sensitive data. */
         OPENSSL_cleanse(hkdf->info, hkdf->infoSz);
@@ -298,7 +298,7 @@ static int we_hkdf_ctrl(EVP_PKEY_CTX *ctx, int type, int num, void *ptr)
                     hkdf->salt = OPENSSL_memdup(ptr, num);
                     if (hkdf->salt == NULL) {
                         WOLFENGINE_ERROR_FUNC_NULL(WE_LOG_PK,
-                            "OPENSSL_memdup(salt)", hkdf->key);
+                            "OPENSSL_memdup(salt)", hkdf->salt);
                         ret = 0;
                     }
                 }
@@ -399,10 +399,16 @@ static int we_hkdf_ctrl_str(EVP_PKEY_CTX *ctx, const char *type,
     }
 #endif
     else if (XSTRNCMP(type, "md", 3) == 0) {
+        const EVP_MD *md = EVP_get_digestbyname(value);
         hkdf = (we_Hkdf *)EVP_PKEY_CTX_get_data(ctx);
         /* Cannot get here without initialization succeeding. */
-        hkdf->mdType =
-            we_nid_to_wc_hash_type(EVP_MD_type(EVP_get_digestbyname(value)));
+        if (md == NULL) {
+            WOLFENGINE_ERROR_MSG(WE_LOG_PK, "Unsupported digest name");
+            ret = 0;
+        }
+        else {
+            hkdf->mdType = we_nid_to_wc_hash_type(EVP_MD_type(md));
+        }
     }
     else if (XSTRNCMP(type, "key", 4) == 0) {
         ret = EVP_PKEY_CTX_str2ctrl(ctx, EVP_PKEY_CTRL_HKDF_KEY, value);

src/we_internal.c

@@ -1148,7 +1148,8 @@ static int wolfengine_destroy(ENGINE *e)
     EVP_MD_meth_free(we_ecdsa_sha1_md);
     we_ecdsa_sha1_md = NULL;
 #endif
-#if defined(WE_HAVE_ECC) || defined(WE_HAVE_AESGCM) || defined(WE_HAVE_RSA)
+#if defined(WE_HAVE_ECC) || defined(WE_HAVE_AESGCM) || defined(WE_HAVE_RSA) || \
+    defined(WE_HAVE_DH) || defined(WE_HAVE_RANDOM)
     we_final_random();
 #endif
 

src/we_logging.c

@@ -358,7 +358,7 @@ void WOLFENGINE_BUFFER(int component, const unsigned char* buffer,
                        unsigned int length)
 {
     int i, buflen = (int)length, bufidx;
-    char line[(WOLFENGINE_LINE_LEN * 4) + 3]; /* \t00..0F | chars...chars\0 */
+    char line[(WOLFENGINE_LINE_LEN * 4) + 4]; /* \t00..0F | chars...chars\0 */
 
     if (!loggingEnabled) {
         return;
@@ -386,7 +386,7 @@ void WOLFENGINE_BUFFER(int component, const unsigned char* buffer,
         }
 
         XSNPRINTF(&line[bufidx], sizeof(line)-bufidx, "| ");
-        bufidx++;
+        bufidx += 2;
 
         for (i = 0; i < WOLFENGINE_LINE_LEN; i++) {
             if (i < buflen) {