diff options
| author | Daniel Bevenius <daniel.bevenius@gmail.com> | 2021-04-28 10:30:13 +0200 |
|---|---|---|
| committer | Benjamin Kaduk <kaduk@mit.edu> | 2021-05-09 11:31:52 -0700 |
| commit | 8be513ae46765ab4c4c3e244640652c24633288d (patch) | |
| tree | a0460643401c3cb4b30815fd7ee913c378a94d8c | |
| parent | f7f0632b01cf16efccb133e395cf115c194bd003 (diff) | |
Mark pop/clear error stack in der2key_decode_p8
This commit sets the error mark before calling d2i_X509_SIG
and clear it if that function call is successful.
The motivation for this is that if d2i_X509_SIG returns NULL then the
else clause will be entered and d2i_PKCS8_PRIV_KEY_INFO will be
called. If d2i_X509_SIG raised any errors those error will be on the
error stack when d2i_PKCS8_PRIV_KEY_INFO gets called, and even if it
returns successfully those errors will still be on the error stack.
We ran into this issue when upgrading Node.js to 3.0.0-alpha15.
More details can be found in the ref links below.
Refs: https://github.com/nodejs/node/issues/38373
Refs: https://github.com/danbev/learning-libcrypto/blob/master/notes/wrong-tag-issue2.md
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/15067)
| -rw-r--r-- | providers/implementations/encode_decode/decode_der2key.c | 5 | ||||
| -rw-r--r-- | test/evp_extra_test.c | 35 |
2 files changed, 40 insertions, 0 deletions
diff --git a/providers/implementations/encode_decode/decode_der2key.c b/providers/implementations/encode_decode/decode_der2key.c index 73acf527c1..01c050ccb0 100644 --- a/providers/implementations/encode_decode/decode_der2key.c +++ b/providers/implementations/encode_decode/decode_der2key.c @@ -124,10 +124,13 @@ static void *der2key_decode_p8(const unsigned char **input_der, ctx->flag_fatal = 0; + ERR_set_mark(); if ((p8 = d2i_X509_SIG(NULL, input_der, input_der_len)) != NULL) { char pbuf[PEM_BUFSIZE]; size_t plen = 0; + ERR_clear_last_mark(); + if (!pw_cb(pbuf, sizeof(pbuf), &plen, NULL, pw_cbarg)) ERR_raise(ERR_LIB_PROV, PROV_R_UNABLE_TO_GET_PASSPHRASE); else @@ -136,6 +139,8 @@ static void *der2key_decode_p8(const unsigned char **input_der, ctx->flag_fatal = 1; X509_SIG_free(p8); } else { + /* Pop any errors that might have been raised by d2i_X509_SIG. */ + ERR_pop_to_mark(); p8inf = d2i_PKCS8_PRIV_KEY_INFO(NULL, input_der, input_der_len); } if (p8inf != NULL diff --git a/test/evp_extra_test.c b/test/evp_extra_test.c index 7fd45bc316..56522e4af9 100644 --- a/test/evp_extra_test.c +++ b/test/evp_extra_test.c @@ -1172,7 +1172,41 @@ static int test_EVP_PKCS82PKEY(void) return ret; } + #endif +static int test_EVP_PKCS82PKEY_wrong_tag(void) +{ + EVP_PKEY *pkey = NULL; + EVP_PKEY *pkey2 = NULL; + BIO *membio = NULL; + char *membuf = NULL; + PKCS8_PRIV_KEY_INFO *p8inf = NULL; + int ok = 0; + + if (testctx != NULL) + /* test not supported with non-default context */ + return 1; + + if (!TEST_ptr(membio = BIO_new(BIO_s_mem())) + || !TEST_ptr(pkey = load_example_rsa_key()) + || !TEST_int_gt(i2d_PKCS8PrivateKey_bio(membio, pkey, NULL, + NULL, 0, NULL, NULL), + 0) + || !TEST_int_gt(BIO_get_mem_data(membio, &membuf), 0) + || !TEST_ptr(p8inf = d2i_PKCS8_PRIV_KEY_INFO_bio(membio, NULL)) + || !TEST_ptr(pkey2 = EVP_PKCS82PKEY(p8inf)) + || !TEST_int_eq(ERR_get_error(), 0)) { + goto done; + } + + ok = 1; + done: + EVP_PKEY_free(pkey); + EVP_PKEY_free(pkey2); + PKCS8_PRIV_KEY_INFO_free(p8inf); + BIO_free_all(membio); + return ok; +} /* This uses kExampleRSAKeyDER and kExampleRSAKeyPKCS8 to verify encoding */ static int test_privatekey_to_pkcs8(void) @@ -2894,6 +2928,7 @@ int setup_tests(void) ADD_TEST(test_EVP_Enveloped); ADD_ALL_TESTS(test_d2i_AutoPrivateKey, OSSL_NELEM(keydata)); ADD_TEST(test_privatekey_to_pkcs8); + ADD_TEST(test_EVP_PKCS82PKEY_wrong_tag); #ifndef OPENSSL_NO_EC ADD_TEST(test_EVP_PKCS82PKEY); #endif |