Fix coverity issues

471258 Unintentional integer overflow 471276, 471277, 471297, 471193 Unchecked return value 516455 Resource leak 471300,471167 Bad bit shift operation 497425, 497430 Overflowed constant 471249, 471299, 471250, 471328, 471335, 471259, 471239, 471182, 471163 Logically dead code 471319 Improper use of negative value 471286 Dereference after null check 471285 Structurally dead code 471272 Double unlock 471269 Unintentional integer overflow 471262 Array compared against 0 471235 Explicit null dereferenced 471226 Data race condition 471175 Argument cannot be negative
dongbeiouba · Dec 26, 2024 · 3d53047 · 3d53047
1 parent bf98e82
commit 3d53047
Show file tree

Hide file tree

Showing 18 changed files with 84 additions and 68 deletions.
diff --git a/apps/delecred.c b/apps/delecred.c
@@ -105,7 +105,8 @@ int delecred_main(int argc, char **argv)
                 ee_key_file = opt_arg();
                 break;
             case OPT_SEC:
-                opt_int(opt_arg(), &valid_time);
+                if (!opt_int(opt_arg(), &valid_time))
+                    goto opthelp;
                 break;
             case OPT_EXPECT_VERIFY_MD:
                 expect_verify_hash = opt_arg();

diff --git a/apps/ec_elgamal.c b/apps/ec_elgamal.c
@@ -360,17 +360,17 @@ int ec_elgamal_main(int argc, char **argv)
     }
 
     action_sum = encrypt + decrypt + add + sub + mul;
-    if (action_sum == 0) {
-        BIO_printf(bio_err, "No action parameter specified.\n");
-        goto opthelp1;
-    } else if (action_sum != 1) {
-        BIO_printf(bio_err, "Only one action parameter must be specified.\n");
+    if (action_sum != 1) {
+        if (action_sum == 0)
+            BIO_printf(bio_err, "No action parameter specified.\n");
+        else
+            BIO_printf(bio_err, "Only one action parameter must be specified.\n");
+
         goto opthelp1;
     }
 
     while ((o = opt_next()) != OPT_EOF) {
         switch (o) {
-        case OPT_EOF:
         case OPT_ERR:
 opthelp2:
             BIO_printf(bio_err, "%s: Use -help for summary.\n", prog);

diff --git a/apps/paillier.c b/apps/paillier.c
@@ -447,7 +447,6 @@ int paillier_main(int argc, char **argv)
     prog = opt_init(argc, argv, paillier_options);
     if ((o = opt_next()) != OPT_EOF) {
         switch (o) {
-        case OPT_EOF:
         case OPT_ERR:
 opthelp1:
             BIO_printf(bio_err, "%s: Use -help for summary.\n", prog);
@@ -492,11 +491,12 @@ int paillier_main(int argc, char **argv)
     }
 
     action_sum = keygen + pubgen + key + pub + encrypt + decrypt + add + add_plain + sub + mul;
-    if (action_sum == 0) {
-        BIO_printf(bio_err, "No action parameter specified.\n");
-        goto opthelp1;
-    } else if (action_sum != 1) {
-        BIO_printf(bio_err, "Only one action parameter must be specified.\n");
+    if (action_sum != 1) {
+        if (action_sum == 0)
+            BIO_printf(bio_err, "No action parameter specified.\n");
+        else
+            BIO_printf(bio_err, "Only one action parameter must be specified.\n");
+
         goto opthelp1;
     }
 

diff --git a/apps/speed.c b/apps/speed.c
@@ -641,7 +641,7 @@ static OPT_PAIR bulletproofs_choices[] = {
 # endif
 };
 
-static int bulletproofs_bits[] = {16, 32, 64};
+static int bulletproofs_bits[] = {16, 32, 63};
 static int bulletproofs_agg_max[] = {1, 16, 32};
 
 # define BULLETPROOFS_NUM                   OSSL_NELEM(bulletproofs_choices)
@@ -1008,22 +1008,26 @@ static int EVP_Update_loop(void *args)
             rc = EVP_DecryptUpdate(ctx, buf, &outl, buf, lengths[testnum]);
             if (rc != 1) {
                 /* reset iv in case of counter overflow */
-                EVP_CipherInit_ex(ctx, NULL, NULL, NULL, iv, -1);
+                rc = EVP_CipherInit_ex(ctx, NULL, NULL, NULL, iv, -1);
             }
         }
     } else {
         for (count = 0; COND(c[D_EVP][testnum]); count++) {
             rc = EVP_EncryptUpdate(ctx, buf, &outl, buf, lengths[testnum]);
             if (rc != 1) {
                 /* reset iv in case of counter overflow */
-                EVP_CipherInit_ex(ctx, NULL, NULL, NULL, iv, -1);
+                rc = EVP_CipherInit_ex(ctx, NULL, NULL, NULL, iv, -1);
             }
         }
     }
     if (decrypt)
-        EVP_DecryptFinal_ex(ctx, buf, &outl);
+        rc = EVP_DecryptFinal_ex(ctx, buf, &outl);
     else
-        EVP_EncryptFinal_ex(ctx, buf, &outl);
+        rc = EVP_EncryptFinal_ex(ctx, buf, &outl);
+
+    if (rc == 0)
+        BIO_printf(bio_err, "Error finalizing cipher loop\n");
+
     return count;
 }
 
@@ -4769,7 +4773,7 @@ int speed_main(int argc, char **argv)
 
 #ifndef OPENSSL_NO_BULLETPROOFS
     for (i = 1; i < sizeof(bp_secrets)/sizeof(bp_secrets[0]); i++) {
-        bp_secrets[i] = (1U << i) - 1;
+        bp_secrets[i] = (1ULL << i) - 1;
     }
 
     if (!(v = BN_new()))
@@ -4783,7 +4787,7 @@ int speed_main(int argc, char **argv)
             continue;           /* Ignore Curve */
 
         for (m = 0; m < BULLETPROOFS_BITS_NUM; m++) {
-            bp_secrets[0] = (1U << bulletproofs_bits[m]) - 1;
+            bp_secrets[0] = (1ULL << bulletproofs_bits[m]) - 1;
 
             for (n = 0; n < BULLETPROOFS_AGG_MAX_NUM; n++) {
                 bp_pp[testnum][m][n] = BP_PUB_PARAM_new_by_curve_id(test_bulletproofs_curves[testnum].nid,
@@ -4823,7 +4827,7 @@ int speed_main(int argc, char **argv)
                     }
 
                     bp_ctx[testnum][m][n][j] = BP_RANGE_CTX_new(bp_pp[testnum][m][n], bp_witness[testnum][m][n][j], bp_transcript[testnum][m][n]);
-                    if (bp_ctx[testnum][m][n] == NULL)
+                    if (bp_ctx[testnum][m][n][j] == NULL)
                         goto end;
 
                     if (!BP_RANGE_PROOF_prove(bp_ctx[testnum][m][n][j], bp_proof[testnum][m][n])) {

diff --git a/crypto/ec/ec_elgamal_crypt.c b/crypto/ec/ec_elgamal_crypt.c
@@ -92,7 +92,8 @@ EC_ELGAMAL_CTX *EC_ELGAMAL_CTX_new(EC_KEY *key, const EC_POINT *h, int32_t flag)
     }
 #endif
 
-    EC_KEY_up_ref(key);
+    if (!EC_KEY_up_ref(key))
+        goto err;
     ctx->key = key;
     ctx->flag = flag;
 

diff --git a/crypto/mem_sec.c b/crypto/mem_sec.c
@@ -223,11 +223,17 @@ int CRYPTO_secure_allocated(const void *ptr)
 
 size_t CRYPTO_secure_used(void)
 {
+    size_t ret = 0;
+
 #ifndef OPENSSL_NO_SECURE_MEMORY
-    return secure_mem_used;
-#else
-    return 0;
+    if (!CRYPTO_THREAD_read_lock(sec_malloc_lock))
+        return 0;
+
+    ret = secure_mem_used;
+
+    CRYPTO_THREAD_unlock(sec_malloc_lock);
 #endif /* OPENSSL_NO_SECURE_MEMORY */
+    return ret;
 }
 
 size_t CRYPTO_secure_actual_size(void *ptr)

diff --git a/crypto/provider_core.c b/crypto/provider_core.c
@@ -639,7 +639,7 @@ int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov,
         if (!ossl_provider_up_ref(actualtmp)) {
             ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE);
             actualtmp = NULL;
-            goto err;
+            return 0;
         }
         *actualprov = actualtmp;
     }
@@ -663,8 +663,6 @@ int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov,
 
  err:
     CRYPTO_THREAD_unlock(store->lock);
-    if (actualprov != NULL)
-        ossl_provider_free(*actualprov);
     return 0;
 }
 

diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c
@@ -284,11 +284,13 @@ int X509_STORE_copy(X509_STORE *dest, const X509_STORE *src)
         for (i = 0; i < num; i++) {
             obj = sk_X509_OBJECT_value(src->objs, i);
             if (obj->type == X509_LU_X509) {
-                X509_STORE_add_cert(dest, obj->data.x509);
+                if (!X509_STORE_add_cert(dest, obj->data.x509))
+                    return 0;
             } else if (obj->type == X509_LU_CRL) {
-                X509_STORE_add_crl(dest, obj->data.crl);
+                if (!X509_STORE_add_crl(dest, obj->data.crl))
+                    return 0;
             } else {
-                /* abort(); */
+                return 0;
             }
         }
     }

diff --git a/crypto/zkp/bulletproofs/bulletproofs_encode.c b/crypto/zkp/bulletproofs/bulletproofs_encode.c
@@ -842,7 +842,7 @@ BP_RANGE_PROOF *BP_RANGE_PROOF_decode(const unsigned char *in, size_t size)
     proof->T2 = sk_EC_POINT_value(sk_point, 3);
 
     sk_bn = zkp_stack_of_bignum_decode(p, &len, bn_len);
-    if (sk_point == NULL)
+    if (sk_bn == NULL)
         goto err;
     p += len;
 
@@ -1114,7 +1114,7 @@ BP_R1CS_PROOF *BP_R1CS_PROOF_decode(const unsigned char *in, size_t size)
 #endif
 
     sk_bn = zkp_stack_of_bignum_decode(p, &len, bn_len);
-    if (sk_point == NULL)
+    if (sk_bn == NULL)
         goto err;
     p += len;
 

diff --git a/crypto/zkp/bulletproofs/inner_product.c b/crypto/zkp/bulletproofs/inner_product.c
@@ -172,6 +172,7 @@ bp_inner_product_proof_t *bp_inner_product_proof_new(bp_inner_product_ctx_t *ctx
 
     if (ctx == NULL || ctx->pp == NULL) {
         ERR_raise(ERR_LIB_ZKP_BP, ERR_R_PASSED_NULL_PARAMETER);
+        return NULL;
     }
 
     n = sk_EC_POINT_num(ctx->pp->sk_G);
@@ -531,7 +532,7 @@ int bp_inner_product_proof_verify(bp_inner_product_ctx_t *ctx,
     proof_num = sk_EC_POINT_num(proof->sk_L);
     n = 2 * proof_num  + 2 * pp_num + 1;
 
-    if (!(vec_x = OPENSSL_zalloc(proof_num * sizeof(*vec_x)))
+    if (proof_num < 0 || !(vec_x = OPENSSL_zalloc(proof_num * sizeof(*vec_x)))
         || !(vec_x_inv = OPENSSL_zalloc(proof_num * sizeof(*vec_x_inv)))) {
         ERR_raise(ERR_LIB_ZKP_BP, ERR_R_MALLOC_FAILURE);
         goto end;

diff --git a/crypto/zkp/bulletproofs/r1cs.c b/crypto/zkp/bulletproofs/r1cs.c
@@ -836,7 +836,8 @@ int BP_R1CS_PROOF_verify(BP_R1CS_CTX *ctx, BP_R1CS_PROOF *proof)
 
     v_n = sk_BP_VARIABLE_num(witness->sk_V);
     lg_n = sk_EC_POINT_num(ip_proof->sk_L);
-    if (padded_n != 1 << lg_n) {
+    if (lg_n < 0 || (lg_n >= (int)sizeof(int) * 8)
+        || ((unsigned int)padded_n != 1U << lg_n)) {
         ERR_raise(ERR_LIB_ZKP_BP, ERR_R_PASSED_INVALID_ARGUMENT);
         goto err;
     }

diff --git a/crypto/zkp/bulletproofs/r1cs_linear_combination.c b/crypto/zkp/bulletproofs/r1cs_linear_combination.c
@@ -210,7 +210,8 @@ BP_R1CS_LINEAR_COMBINATION *BP_R1CS_LINEAR_COMBINATION_dup(const BP_R1CS_LINEAR_
         if (item_dup == NULL)
             goto err;
 
-        sk_BP_R1CS_LINEAR_COMBINATION_ITEM_push(ret->items, item_dup);
+        if (sk_BP_R1CS_LINEAR_COMBINATION_ITEM_push(ret->items, item_dup) <= 0)
+            goto err;
     }
 
     ret->type = lc->type;
@@ -427,12 +428,12 @@ int BP_R1CS_LINEAR_COMBINATION_raw_mul(BP_R1CS_LINEAR_COMBINATION **output,
     BN_CTX_free(bn_ctx);
     return 1;
 err:
-    if (output == NULL)
-        output = NULL;
-    if (left == NULL)
-        left = NULL;
-    if (right == NULL)
-        right = NULL;
+    if (output != NULL)
+        *output = NULL;
+    if (left != NULL)
+        *left = NULL;
+    if (right != NULL)
+        *right = NULL;
 
     BP_R1CS_LINEAR_COMBINATION_free(llc);
     BP_R1CS_LINEAR_COMBINATION_free(rlc);

diff --git a/crypto/zkp/nizk/nizk_encode.c b/crypto/zkp/nizk/nizk_encode.c
@@ -892,7 +892,7 @@ NIZK_DLOG_KNOWLEDGE_PROOF *NIZK_DLOG_KNOWLEDGE_PROOF_decode(const unsigned char
     proof->A = sk_EC_POINT_value(sk_point, 0);
 
     sk_bn = zkp_stack_of_bignum_decode(p, &len, bn_len);
-    if (sk_point == NULL)
+    if (sk_bn == NULL)
         goto err;
     p += len;
 
@@ -1092,7 +1092,7 @@ NIZK_DLOG_EQUALITY_PROOF *NIZK_DLOG_EQUALITY_PROOF_decode(const unsigned char *i
     proof->A2 = sk_EC_POINT_value(sk_point, 1);
 
     sk_bn = zkp_stack_of_bignum_decode(p, &len, bn_len);
-    if (sk_point == NULL)
+    if (sk_bn == NULL)
         goto err;
     p += len;
 

diff --git a/providers/implementations/rands/drbg.c b/providers/implementations/rands/drbg.c
@@ -477,18 +477,18 @@ int ossl_prov_drbg_instantiate(PROV_DRBG *drbg, unsigned int strength,
 
     if (!drbg->instantiate(drbg, entropy, entropylen, nonce, noncelen,
                            pers, perslen)) {
-        cleanup_entropy(drbg, entropy, entropylen);
         ERR_raise(ERR_LIB_PROV, PROV_R_ERROR_INSTANTIATING_DRBG);
         goto end;
     }
-    cleanup_entropy(drbg, entropy, entropylen);
 
     drbg->state = EVP_RAND_STATE_READY;
     drbg->generate_counter = 1;
     drbg->reseed_time = time(NULL);
     tsan_store(&drbg->reseed_counter, drbg->reseed_next_counter);
 
  end:
+    if (entropy != NULL)
+        cleanup_entropy(drbg, entropy, entropylen);
     if (nonce != NULL)
         ossl_prov_cleanup_nonce(drbg->provctx, nonce, noncelen);
     if (drbg->state == EVP_RAND_STATE_READY)

diff --git a/ssl/ssl_dc.c b/ssl/ssl_dc.c
@@ -199,6 +199,10 @@ int SSL_verify_delegated_credential_signature(X509 *parent_cert,
 
     parent_cert_raw_index = parent_cert_raw;
     parent_cert_len = i2d_X509_AUX(parent_cert, &parent_cert_raw_index);
+    if (parent_cert_len <= 0) {
+        ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
+        goto err;
+    }
 
     if (!ssl_dc_tbs_data(parent_cert_raw, parent_cert_len,
                                            dc, is_server, &tbs, &tbs_len)) {
@@ -269,19 +273,25 @@ int DC_sign(DELEGATED_CREDENTIAL *dc, EVP_PKEY *dc_pkey,
     if (!DC_check_parent_cert_valid(ee_cert))
         goto end;
 
-    dc_pkey_raw_len = i2d_PUBKEY(dc_pkey, NULL);
-    if (dc_pkey_raw_len <= 0) {
+    ret = i2d_PUBKEY(dc_pkey, NULL);
+    if (ret <= 0) {
         ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
         goto end;
     }
 
-    if ((dc_pkey_raw = OPENSSL_malloc(dc_pkey_raw_len)) == NULL) {
+    if ((dc_pkey_raw = OPENSSL_malloc(ret)) == NULL) {
         ERR_raise(ERR_LIB_SSL, ERR_R_MALLOC_FAILURE);
         goto end;
     }
 
     dc_pkey_raw_index = dc_pkey_raw;
-    dc_pkey_raw_len = i2d_PUBKEY(dc_pkey, &dc_pkey_raw_index);
+    ret = i2d_PUBKEY(dc_pkey, &dc_pkey_raw_index);
+    if (ret <= 0) {
+        ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
+        goto end;
+    }
+
+    dc_pkey_raw_len = ret;
 
     dc_verify_lu = ssl_sigalg_lookup_by_pkey_and_hash(dc_pkey,
                                                       expect_verify_hash, 1);

diff --git a/ssl/statem_ntls/ntls_extensions.c b/ssl/statem_ntls/ntls_extensions.c
@@ -934,19 +934,9 @@ static int init_alpn(SSL *s, unsigned int context)
 static int final_alpn(SSL *s, unsigned int context, int sent)
 {
     if (!s->server && !sent && s->session->ext.alpn_selected != NULL)
-            s->ext.early_data_ok = 0;
-        return 1;
+        s->ext.early_data_ok = 0;
 
-    /*
-     * Call alpn_select callback if needed.  Has to be done after SNI and
-     * cipher negotiation (HTTP/2 restricts permitted ciphers). In TLSv1.3
-     * we also have to do this before we decide whether to accept early_data.
-     * In TLSv1.3 we've already negotiated our cipher so we do this call now.
-     * For < TLSv1.3 we defer it until after cipher negotiation.
-     *
-     * On failure SSLfatal_ntls() already called.
-     */
-    return tls_handle_alpn_ntls(s);
+    return 1;
 }
 
 static int init_sig_algs(SSL *s, unsigned int context)

diff --git a/ssl/statem_ntls/ntls_statem_clnt.c b/ssl/statem_ntls/ntls_statem_clnt.c
@@ -1414,9 +1414,11 @@ WORK_STATE tls_post_process_server_certificate_ntls(SSL *s, WORK_STATE wst)
         }
     }
 
-    X509_free(s->session->peer);
-    X509_up_ref(x);
-    s->session->peer = x;
+    if (x) {
+        X509_free(s->session->peer);
+        s->session->peer = x;
+        X509_up_ref(x);
+    }
     s->session->verify_result = s->verify_result;
 
     return WORK_FINISHED_CONTINUE;