Skip to content

Commit

Permalink
Fix various memory leaks.
Browse files Browse the repository at this point in the history 
This commit fixes various memory leaks that have been found using ASan.

Note: I will soon add a CI job for testing oqs-provider against memory leaks
with ASan.


1. Memory leak in `oqs_test_signatures`.
===

In `oqs_test_signatures`, [`OPENSSL_free`] was used with `EVP_PKEY_CTX`. However,
the right destructor function for `EVP_PKEY_CTX` is [`EVP_PKEY_CTX_free`].

This commmit fixes this minor bug.

[`OPENSSL_free`](https://www.openssl.org/docs/man3.0/man3/OPENSSL_free.html)
[`EVP_PKEY_CTX_free`](https://www.openssl.org/docs/man3.0/man3/EVP_PKEY_CTX_free.html)


2. a selftest key that wasn't freed.
===

In `oqsprov/oqsprov_keys.c`, `d2i_PrivateKey` is used to verify that we the
key we've just generated is sound.
However, the returned object wasn't freed.
ASan trace that helped me finding this one:

```
Indirect leak of 96 byte(s) in 4 object(s) allocated from:
    #0 0x5651eef1428e in malloc (/home/pawn/work/oqs-provider/build/test/oqs_test_signatures+0xb928e) (BuildId: 6cc7ebd4bc73a0c78b2ab8d7b9dcb8b165d88e74)
    open-quantum-safe#1 0x7fb7a23a946b in CRYPTO_malloc /home/pawn/work/openssl/crypto/mem.c:190:12
    open-quantum-safe#2 0x7fb7a23a94a2 in CRYPTO_zalloc /home/pawn/work/openssl/crypto/mem.c:197:11
    open-quantum-safe#3 0x7fb7a209cdcd in BN_new /home/pawn/work/openssl/crypto/bn/bn_lib.c:247:16
    open-quantum-safe#4 0x7fb7a2224a00 in ossl_ec_GFp_mont_group_set_curve /home/pawn/work/openssl/crypto/ec/ecp_mont.c:169:11
    open-quantum-safe#5 0x7fb7a220f5fa in EC_GROUP_set_curve /home/pawn/work/openssl/crypto/ec/ec_lib.c:562:12
    open-quantum-safe#6 0x7fb7a2202d75 in EC_GROUP_new_curve_GFp /home/pawn/work/openssl/crypto/ec/ec_cvt.c:61:10
    open-quantum-safe#7 0x7fb7a220184d in ec_group_new_from_data /home/pawn/work/openssl/crypto/ec/ec_curve.c:3179:22
    open-quantum-safe#8 0x7fb7a2200ff9 in EC_GROUP_new_by_curve_name_ex /home/pawn/work/openssl/crypto/ec/ec_curve.c:3287:19
    open-quantum-safe#9 0x7fb7a2201ed9 in EC_GROUP_new_by_curve_name /home/pawn/work/openssl/crypto/ec/ec_curve.c:3303:12
    open-quantum-safe#10 0x7fb7a21f84e2 in EC_GROUP_new_from_ecpkparameters /home/pawn/work/openssl/crypto/ec/ec_asn1.c:863:20
    open-quantum-safe#11 0x7fb7a21f8f28 in d2i_ECPrivateKey /home/pawn/work/openssl/crypto/ec/ec_asn1.c:956:22
    open-quantum-safe#12 0x7fb7a270b4b5 in der2key_decode /home/pawn/work/openssl/providers/implementations/encode_decode/decode_der2key.c:220:19
    open-quantum-safe#13 0x7fb7a226eae8 in decoder_process /home/pawn/work/openssl/crypto/encode_decode/decoder_lib.c:971:14
    open-quantum-safe#14 0x7fb7a226d106 in OSSL_DECODER_from_bio /home/pawn/work/openssl/crypto/encode_decode/decoder_lib.c:82:10
    open-quantum-safe#15 0x7fb7a226f007 in OSSL_DECODER_from_data /home/pawn/work/openssl/crypto/encode_decode/decoder_lib.c:157:9
    open-quantum-safe#16 0x7fb7a200e123 in d2i_PrivateKey_decoder /home/pawn/work/openssl/crypto/asn1/d2i_pr.c:74:11
    open-quantum-safe#17 0x7fb7a200dbba in d2i_PrivateKey_ex /home/pawn/work/openssl/crypto/asn1/d2i_pr.c:162:11
    open-quantum-safe#18 0x7fb7a200e3d2 in d2i_PrivateKey /home/pawn/work/openssl/crypto/asn1/d2i_pr.c:172:12
    open-quantum-safe#19 0x7fb79dc6a22e in oqsx_key_gen_evp_key /home/pawn/work/oqs-provider/oqsprov/oqsprov_keys.c:1036:25
    open-quantum-safe#20 0x7fb79dc68f3f in oqsx_key_gen /home/pawn/work/oqs-provider/oqsprov/oqsprov_keys.c:1077:16
    open-quantum-safe#21 0x7fb79dc7164a in oqsx_genkey /home/pawn/work/oqs-provider/oqsprov/oqs_kmgmt.c:496:9
    open-quantum-safe#22 0x7fb79dc6d9f8 in oqsx_gen /home/pawn/work/oqs-provider/oqsprov/oqs_kmgmt.c:509:12
    open-quantum-safe#23 0x7fb7a23373c7 in evp_keymgmt_gen /home/pawn/work/openssl/crypto/evp/keymgmt_meth.c:366:12
    open-quantum-safe#24 0x7fb7a2334b58 in evp_keymgmt_util_gen /home/pawn/work/openssl/crypto/evp/keymgmt_lib.c:517:20
    open-quantum-safe#25 0x7fb7a235a775 in EVP_PKEY_generate /home/pawn/work/openssl/crypto/evp/pmeth_gn.c:189:13
    open-quantum-safe#26 0x5651eef4f2bd in test_oqs_signatures /home/pawn/work/oqs-provider/test/oqs_test_signatures.c:39:48
    open-quantum-safe#27 0x5651eef4ee3d in main /home/pawn/work/oqs-provider/test/oqs_test_signatures.c:114:17
    open-quantum-safe#28 0x7fb7a1b671c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
```

3. a `EVP_PKEY_CTX` that wasn't freed.
===

In `oqsx_key_free`, `key->oqsx_provider_ctx.oqsx_evp_ctx->ctx` is freed if
the keytype is `ECP_HYB_KEM` or `ECX_HYB_KEM`. However,
`key->oqsx_provider_ctx.oqsx_evp_ctx->ctx` is also used for signatures.
In that case, a call to `EVP_PKEY_CTX_free` was missing.
ASan trace that helped me finding this one:

```
Indirect leak of 7 byte(s) in 2 object(s) allocated from:
    #0 0x55b6fc8d427e in malloc (/home/pawn/work/oqs-provider/build/test/oqs_test_signatures+0xb927e) (BuildId: 3a8bff15d3315ba7ac1746fdc7b67eb464c607a1)
    open-quantum-safe#1 0x7ff34dfa946b in CRYPTO_malloc /home/pawn/work/openssl/crypto/mem.c:190:12
    open-quantum-safe#2 0x7ff34dfac50a in CRYPTO_strndup /home/pawn/work/openssl/crypto/o_str.c:43:11
    open-quantum-safe#3 0x7ff34df9d152 in ossl_algorithm_get1_first_name /home/pawn/work/openssl/crypto/core_algorithm.c:195:11
    open-quantum-safe#4 0x7ff34df353a1 in keymgmt_from_algorithm /home/pawn/work/openssl/crypto/evp/keymgmt_meth.c:50:31
    open-quantum-safe#5 0x7ff34df15e44 in construct_evp_method /home/pawn/work/openssl/crypto/evp/evp_fetch.c:219:14
    open-quantum-safe#6 0x7ff34df9e3ce in ossl_method_construct_this /home/pawn/work/openssl/crypto/core_fetch.c:109:19
    open-quantum-safe#7 0x7ff34df9d740 in algorithm_do_map /home/pawn/work/openssl/crypto/core_algorithm.c:77:13
    open-quantum-safe#8 0x7ff34df9ce88 in algorithm_do_this /home/pawn/work/openssl/crypto/core_algorithm.c:122:15
    open-quantum-safe#9 0x7ff34dfcce3e in ossl_provider_doall_activated /home/pawn/work/openssl/crypto/provider_core.c:1423:14
    open-quantum-safe#10 0x7ff34df9ca64 in ossl_algorithm_do_all /home/pawn/work/openssl/crypto/core_algorithm.c:162:9
    open-quantum-safe#11 0x7ff34df9dd64 in ossl_method_construct /home/pawn/work/openssl/crypto/core_fetch.c:153:5
    open-quantum-safe#12 0x7ff34df13b6f in inner_evp_generic_fetch /home/pawn/work/openssl/crypto/evp/evp_fetch.c:312:23
    open-quantum-safe#13 0x7ff34df13226 in evp_generic_fetch /home/pawn/work/openssl/crypto/evp/evp_fetch.c:364:14
    open-quantum-safe#14 0x7ff34df36a42 in EVP_KEYMGMT_fetch /home/pawn/work/openssl/crypto/evp/keymgmt_meth.c:220:12
    open-quantum-safe#15 0x7ff34df5d6e4 in int_ctx_new /home/pawn/work/openssl/crypto/evp/pmeth_lib.c:280:23
    open-quantum-safe#16 0x7ff34df5f0a9 in EVP_PKEY_CTX_new_id /home/pawn/work/openssl/crypto/evp/pmeth_lib.c:469:12
    open-quantum-safe#17 0x7ff349865bcb in oqsx_hybsig_init /home/pawn/work/oqs-provider/oqsprov/oqsprov_keys.c:578:20
    open-quantum-safe#18 0x7ff349865157 in oqsx_key_new /home/pawn/work/oqs-provider/oqsprov/oqsprov_keys.c:791:16
    open-quantum-safe#19 0x7ff3498715a3 in oqsx_genkey /home/pawn/work/oqs-provider/oqsprov/oqs_kmgmt.c:487:16
    open-quantum-safe#20 0x7ff34986da08 in oqsx_gen /home/pawn/work/oqs-provider/oqsprov/oqs_kmgmt.c:509:12
    open-quantum-safe#21 0x7ff34df373c7 in evp_keymgmt_gen /home/pawn/work/openssl/crypto/evp/keymgmt_meth.c:366:12
    open-quantum-safe#22 0x7ff34df34b58 in evp_keymgmt_util_gen /home/pawn/work/openssl/crypto/evp/keymgmt_lib.c:517:20
    open-quantum-safe#23 0x7ff34df5a775 in EVP_PKEY_generate /home/pawn/work/openssl/crypto/evp/pmeth_gn.c:189:13
    open-quantum-safe#24 0x55b6fc90f2ad in test_oqs_signatures /home/pawn/work/oqs-provider/test/oqs_test_signatures.c:39:48
    open-quantum-safe#25 0x55b6fc90ee2d in main /home/pawn/work/oqs-provider/test/oqs_test_signatures.c:114:17
    open-quantum-safe#26 0x7ff34d8461c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
```

4. Missing `EVP_PKEY_CTX_free` in case of error.
===

In `oqsx_hybsig_init`, a `EVP_PKEY_CTX` is initialized using `EVP_PKEY_CTX_new_id`.
However, later in the code, some extra work is done, and in case of error, the
`EVP_PKEY_CTX` wasn't freed.

(I couldn't recover the ASan message for this one…)


5. `OPENSSL_free` instead of `EVP_PKEY_free`.
===

In `oqsx_key_gen`, when a classical private key is generated, the field
`classical_pkey` of a `OQSX_KEY` is set with a value of type `EVP_PKEY`.

However, in `oqsx_key_free`, `OPENSSL_free` was used on `classical_pkey`
instead of `EVP_PKEY_free`.

ASan trace that helped me finding this one:

```
   #0 0x555a6b1b728e in malloc (/home/pawn/work/oqs-provider/build/test/oqs_test_signatures+0xb928e) (BuildId: 6cc7ebd4bc73a0c78b2ab8d7b9dcb8b165d88e74)
    open-quantum-safe#1 0x7f59a93a946b in CRYPTO_malloc /home/pawn/work/openssl/crypto/mem.c:190:12
    open-quantum-safe#2 0x7f59a93a94a2 in CRYPTO_zalloc /home/pawn/work/openssl/crypto/mem.c:197:11
    open-quantum-safe#3 0x7f59a909cdcd in BN_new /home/pawn/work/openssl/crypto/bn/bn_lib.c:247:16
    open-quantum-safe#4 0x7f59a920b955 in ossl_ec_group_new_ex /home/pawn/work/openssl/crypto/ec/ec_lib.c:61:25
    open-quantum-safe#5 0x7f59a920dd55 in EC_GROUP_dup /home/pawn/work/openssl/crypto/ec/ec_lib.c:273:14
    open-quantum-safe#6 0x7f59a9207774 in EC_KEY_set_group /home/pawn/work/openssl/crypto/ec/ec_key.c:733:18
    open-quantum-safe#7 0x7f59a975b3b5 in ec_gen_assign_group /home/pawn/work/openssl/providers/implementations/keymgmt/ec_kmgmt.c:1236:12
    open-quantum-safe#8 0x7f59a9758bdb in ec_gen /home/pawn/work/openssl/providers/implementations/keymgmt/ec_kmgmt.c:1274:11
    open-quantum-safe#9 0x7f59a93373c7 in evp_keymgmt_gen /home/pawn/work/openssl/crypto/evp/keymgmt_meth.c:366:12
    open-quantum-safe#10 0x7f59a9334b58 in evp_keymgmt_util_gen /home/pawn/work/openssl/crypto/evp/keymgmt_lib.c:517:20
    open-quantum-safe#11 0x7f59a935a775 in EVP_PKEY_generate /home/pawn/work/openssl/crypto/evp/pmeth_gn.c:189:13
    open-quantum-safe#12 0x7f59a935b404 in EVP_PKEY_keygen /home/pawn/work/openssl/crypto/evp/pmeth_gn.c:274:12
    open-quantum-safe#13 0x7f59a4c69a51 in oqsx_key_gen_evp_key /home/pawn/work/oqs-provider/oqsprov/oqsprov_keys.c:1012:12
    open-quantum-safe#14 0x7f59a4c68faf in oqsx_key_gen /home/pawn/work/oqs-provider/oqsprov/oqsprov_keys.c:1084:16
    open-quantum-safe#15 0x7f59a4c716ca in oqsx_genkey /home/pawn/work/oqs-provider/oqsprov/oqs_kmgmt.c:496:9
    open-quantum-safe#16 0x7f59a4c6da78 in oqsx_gen /home/pawn/work/oqs-provider/oqsprov/oqs_kmgmt.c:509:12
    open-quantum-safe#17 0x7f59a93373c7 in evp_keymgmt_gen /home/pawn/work/openssl/crypto/evp/keymgmt_meth.c:366:12
    open-quantum-safe#18 0x7f59a9334b58 in evp_keymgmt_util_gen /home/pawn/work/openssl/crypto/evp/keymgmt_lib.c:517:20
    open-quantum-safe#19 0x7f59a935a775 in EVP_PKEY_generate /home/pawn/work/openssl/crypto/evp/pmeth_gn.c:189:13
    open-quantum-safe#20 0x555a6b1f2890 in test_oqs_signatures /home/pawn/work/oqs-provider/test/oqs_test_signatures.c:68:44
    open-quantum-safe#21 0x555a6b1f1e3d in main /home/pawn/work/oqs-provider/test/oqs_test_signatures.c:114:17
    open-quantum-safe#22 0x7f59a8c461c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
```

6. Bonus point: fix a potential double-free.
===

In `oqsx_key_gen`, when the keytype isn't `KEY_TYPE_HYB_SIG`, the
generated `pkey` is freed, and the flow goes down to the `err` label:

https://github.com/open-quantum-safe/oqs-provider/blob/cc3895f208730abfe1efc15f7c3ff4358ac6e4c0/oqsprov/oqsprov_keys.c#L1089-L1104

There is a potential double-free bug here: if `oqsx_key_gen_oqs` fails, then
`EVP_PKEY_free` will be called twice on the same `pkey` pointer.

Note that it is very unlikely that `oqsx_key_gen_oqs` fails, since it only
calls `OQS_KEM_keypair`/`OQS_SIG_keypair`.
  • Loading branch information
@thb-sb
thb-sb committed Sep 5, 2023
1 parent cc3895f commit 3fc853a
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 9 deletions.
22 changes: 15 additions & 7 deletions oqsprov/oqsprov_keys.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -580,16 +580,20 @@ static int oqsx_hybsig_init(int bit_security, OQSX_EVP_CTX *evp_ctx,

if (idx < 3) { // EC
ret = EVP_PKEY_paramgen_init(evp_ctx->ctx);
ON_ERR_GOTO(ret <= 0, err);
ON_ERR_GOTO(ret <= 0, free_evp_ctx);

ret = EVP_PKEY_CTX_set_ec_paramgen_curve_nid(evp_ctx->ctx,
evp_ctx->evp_info->nid);
ON_ERR_GOTO(ret <= 0, err);
ON_ERR_GOTO(ret <= 0, free_evp_ctx);

ret = EVP_PKEY_paramgen(evp_ctx->ctx, &evp_ctx->keyParam);
ON_ERR_GOTO(ret <= 0 || !evp_ctx->keyParam, err);
ON_ERR_GOTO(ret <= 0 || !evp_ctx->keyParam, free_evp_ctx);
}
// RSA bit length set only during keygen
goto err;

free_evp_ctx:
EVP_PKEY_CTX_free(evp_ctx->ctx);

err:
return ret;
Expand Down Expand Up @@ -867,12 +871,14 @@ void oqsx_key_free(OQSX_KEY *key)
else if (key->keytype == KEY_TYPE_ECP_HYB_KEM
|| key->keytype == KEY_TYPE_ECX_HYB_KEM) {
OQS_KEM_free(key->oqsx_provider_ctx.oqsx_qs_ctx.kem);
EVP_PKEY_CTX_free(key->oqsx_provider_ctx.oqsx_evp_ctx->ctx);
EVP_PKEY_free(key->oqsx_provider_ctx.oqsx_evp_ctx->keyParam);
OPENSSL_free(key->oqsx_provider_ctx.oqsx_evp_ctx);
} else
OQS_SIG_free(key->oqsx_provider_ctx.oqsx_qs_ctx.sig);
OPENSSL_free(key->classical_pkey);
EVP_PKEY_free(key->classical_pkey);
if (key->oqsx_provider_ctx.oqsx_evp_ctx) {
EVP_PKEY_CTX_free(key->oqsx_provider_ctx.oqsx_evp_ctx->ctx);
EVP_PKEY_free(key->oqsx_provider_ctx.oqsx_evp_ctx->keyParam);
OPENSSL_free(key->oqsx_provider_ctx.oqsx_evp_ctx);
}
#ifdef OQS_PROVIDER_NOATOMIC
CRYPTO_THREAD_lock_free(key->lock);
#endif
Expand Down Expand Up @@ -1036,6 +1042,7 @@ static EVP_PKEY *oqsx_key_gen_evp_key(OQSX_EVP_CTX *ctx, unsigned char *pubkey,
EVP_PKEY *ck2 = d2i_PrivateKey(ctx->evp_info->keytype, NULL,
&privkey_enc2, privkeylen);
ON_ERR_SET_GOTO(!ck2, ret, -14, errhyb);
EVP_PKEY_free(ck2);
}
ENCODE_UINT32(pubkey, pubkeylen);
ENCODE_UINT32(privkey, privkeylen);
Expand Down Expand Up @@ -1087,6 +1094,7 @@ int oqsx_key_gen(OQSX_KEY *key)
ret = oqsx_key_gen_oqs(key, 0);
} else {
EVP_PKEY_free(pkey);
pkey = NULL;
ret = oqsx_key_gen_oqs(key, 1);
}
} else if (key->keytype == KEY_TYPE_SIG) {
Expand Down
4 changes: 2 additions & 2 deletions test/oqs_test_signatures.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ static int test_oqs_signatures(const char *sigalg_name)

EVP_MD_CTX_free(mdctx);
EVP_PKEY_free(key);
OPENSSL_free(ctx);
EVP_PKEY_CTX_free(ctx);
OPENSSL_free(sig);
mdctx = NULL;
key = NULL;
Expand All @@ -84,7 +84,7 @@ static int test_oqs_signatures(const char *sigalg_name)

EVP_MD_CTX_free(mdctx);
EVP_PKEY_free(key);
OPENSSL_free(ctx);
EVP_PKEY_CTX_free(ctx);
OPENSSL_free(sig);
return testresult;
}
Expand Down

0 comments on commit 3fc853a

Please sign in to comment.