From 38d6d75dbc731e2717c5bf38a729c5a6b0a4f44f Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sun, 7 Feb 2021 16:46:31 -0500 Subject: [PATCH 1/2] Fix return value checks in OpenSSL code According to `man 3ssl` the only successful return value for EVP_PKEY_verify_init() is 1, and EVP_PKEY_CTX_set_rsa_padding() and EVP_PKEY_CTX_set_signature_md() can both return 0 or a negative number on failure or any positive number on success. BN_bn2binpad() returns -1 on error, but 0 (an empty key or signature) is also not valid. Therefore use != 1 to check the return value of EVP_PKEY_verify_init(), <= 0 to check the return values of the other three functions mentioned above. Also delete a bunch of cruft. --- rpmio/digest_openssl.c | 55 +++++++++--------------------------------- 1 file changed, 12 insertions(+), 43 deletions(-) diff --git a/rpmio/digest_openssl.c b/rpmio/digest_openssl.c index 0cb781e57e..20c272df80 100644 --- a/rpmio/digest_openssl.c +++ b/rpmio/digest_openssl.c @@ -450,7 +450,7 @@ static void pgpFreeSigRSA(pgpDigAlg pgpsig) static int pgpVerifySigRSA(pgpDigAlg pgpkey, pgpDigAlg pgpsig, uint8_t *hash, size_t hashlen, int hash_algo) { - int rc, ret; + int rc = 1; /* assume failure */ EVP_PKEY_CTX *pkey_ctx = NULL; struct pgpDigSigRSA_s *sig = pgpsig->data; @@ -458,53 +458,32 @@ static int pgpVerifySigRSA(pgpDigAlg pgpkey, pgpDigAlg pgpsig, struct pgpDigKeyRSA_s *key = pgpkey->data; - if (!constructRSASigningKey(key)) { - rc = 1; + if (!constructRSASigningKey(key)) goto done; - } pkey_ctx = EVP_PKEY_CTX_new(key->evp_pkey, NULL); - if (!pkey_ctx) { - rc = 1; + if (!pkey_ctx) goto done; - } - ret = EVP_PKEY_verify_init(pkey_ctx); - if (ret < 0) { - rc = 1; + if (EVP_PKEY_verify_init(pkey_ctx) != 1) goto done; - } - ret = EVP_PKEY_CTX_set_rsa_padding(pkey_ctx, RSA_PKCS1_PADDING); - if (ret < 0) { - rc = 1; + if (EVP_PKEY_CTX_set_rsa_padding(pkey_ctx, RSA_PKCS1_PADDING) <= 0) goto done; - } - ret = EVP_PKEY_CTX_set_signature_md(pkey_ctx, getEVPMD(hash_algo)); - if (ret < 0) { - rc = 1; + if (EVP_PKEY_CTX_set_signature_md(pkey_ctx, getEVPMD(hash_algo)) <= 0) goto done; - } int pkey_len = EVP_PKEY_size(key->evp_pkey); padded_sig = xcalloc(1, pkey_len); - if (!BN_bn2binpad(sig->bn, padded_sig, pkey_len)) { - rc = 1; + if (BN_bn2binpad(sig->bn, padded_sig, pkey_len) <= 0) goto done; - } - ret = EVP_PKEY_verify(pkey_ctx, padded_sig, pkey_len, hash, hashlen); - if (ret == 1) + if (EVP_PKEY_verify(pkey_ctx, padded_sig, pkey_len, hash, hashlen) == 1) { /* Success */ rc = 0; } - else - { - /* Failure */ - rc = 1; - } done: EVP_PKEY_CTX_free(pkey_ctx); @@ -735,32 +714,22 @@ static void pgpFreeSigDSA(pgpDigAlg pgpsig) static int pgpVerifySigDSA(pgpDigAlg pgpkey, pgpDigAlg pgpsig, uint8_t *hash, size_t hashlen, int hash_algo) { - int rc, ret; + int rc = 1; /* assume failure */ struct pgpDigSigDSA_s *sig = pgpsig->data; struct pgpDigKeyDSA_s *key = pgpkey->data; - if (!constructDSASigningKey(key)) { - rc = 1; + if (!constructDSASigningKey(key)) goto done; - } - if (!constructDSASignature(sig)) { - rc = 1; + if (!constructDSASignature(sig)) goto done; - } - ret = DSA_do_verify(hash, hashlen, sig->dsa_sig, key->dsa_key); - if (ret == 1) + if (DSA_do_verify(hash, hashlen, sig->dsa_sig, key->dsa_key) == 1) { /* Success */ rc = 0; } - else - { - /* Failure */ - rc = 1; - } done: return rc; From fd51921147203f7351fd9de1fcf56f95cac83e54 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 9 Apr 2021 13:34:12 -0400 Subject: [PATCH 2/2] Avoid double frees if EVP_PKEY_assign_RSA fails Previously, the bignums would be left as dangling and double-freed. --- rpmio/digest_openssl.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/rpmio/digest_openssl.c b/rpmio/digest_openssl.c index 20c272df80..02f34a90f3 100644 --- a/rpmio/digest_openssl.c +++ b/rpmio/digest_openssl.c @@ -292,8 +292,8 @@ struct pgpDigKeyRSA_s { BIGNUM *n; /* Common Modulus */ BIGNUM *e; /* Public Exponent */ - EVP_PKEY *evp_pkey; /* Fully constructed key */ + unsigned char immutable; /* if set, this key cannot be mutated */ }; static int constructRSASigningKey(struct pgpDigKeyRSA_s *key) @@ -301,33 +301,34 @@ static int constructRSASigningKey(struct pgpDigKeyRSA_s *key) if (key->evp_pkey) { /* We've already constructed it, so just reuse it */ return 1; - } + } else if (key->immutable) + return 0; + key->immutable = 1; /* Create the RSA key */ RSA *rsa = RSA_new(); if (!rsa) return 0; - if (!RSA_set0_key(rsa, key->n, key->e, NULL)) { - RSA_free(rsa); - return 0; - } + if (RSA_set0_key(rsa, key->n, key->e, NULL) <= 0) + goto exit; + key->n = key->e = NULL; /* Create an EVP_PKEY container to abstract the key-type. */ - key->evp_pkey = EVP_PKEY_new(); - if (!key->evp_pkey) { - RSA_free(rsa); - return 0; - } + if (!(key->evp_pkey = EVP_PKEY_new())) + goto exit; /* Assign the RSA key to the EVP_PKEY structure. This will take over memory management of the RSA key */ if (!EVP_PKEY_assign_RSA(key->evp_pkey, rsa)) { EVP_PKEY_free(key->evp_pkey); key->evp_pkey = NULL; - RSA_free(rsa); + goto exit; } return 1; +exit: + RSA_free(rsa); + return 0; } static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num, const uint8_t *p) @@ -335,9 +336,10 @@ static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num, const uint8_t *p) size_t mlen = pgpMpiLen(p) - 2; struct pgpDigKeyRSA_s *key = pgpkey->data; - if (!key) { + if (!key) key = pgpkey->data = xcalloc(1, sizeof(*key)); - } + else if (key->immutable) + return 1; switch (num) { case 0: @@ -347,7 +349,7 @@ static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num, const uint8_t *p) return 1; } - key->nbytes = mlen; + key->nbytes = mlen; /* Create a BIGNUM from the pointer. Note: this assumes big-endian data as required by PGP */ key->n = BN_bin2bn(p+2, mlen, NULL);