Skip to content

Commit

Permalink
Mark pop/clear error stack in der2key_decode_p8
Browse files Browse the repository at this point in the history
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: nodejs/node#38373
Refs: https://github.com/danbev/learning-libcrypto/blob/master/notes/wrong-tag-issue2.md
  • Loading branch information
danbev committed Apr 28, 2021
1 parent 67ea4be commit 82f360a
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 0 deletions.
4 changes: 4 additions & 0 deletions providers/implementations/encode_decode/decode_der2key.c
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,9 @@ 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) {
ERR_clear_last_mark();
char pbuf[PEM_BUFSIZE];
size_t plen = 0;

Expand All @@ -136,6 +138,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
Expand Down
38 changes: 38 additions & 0 deletions test/evp_extra_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -1169,6 +1169,43 @@ static int test_EVP_PKCS82PKEY(void)

return ret;
}

static int test_EVP_PKCS82PKEY_wrong_tag(void)
{
OSSL_PROVIDER *provider = OSSL_PROVIDER_load(NULL, "default");
EVP_PKEY *pkey = NULL;
BIO *membio = NULL;
char *membuf = NULL;
PKCS8_PRIV_KEY_INFO *p8inf = NULL;
long membuf_len = 0;
int ok = 0;

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(membuf_len = BIO_get_mem_data(membio, &membuf), 0)
|| !TEST_ptr(membuf)
|| !TEST_mem_eq(membuf, (size_t)membuf_len,
kExampleRSAKeyPKCS8, sizeof(kExampleRSAKeyPKCS8))
|| !TEST_int_gt(PEM_write_bio_PKCS8PrivateKey(membio, pkey, NULL,
NULL, 0, NULL, NULL),
0)
|| !TEST_ptr(p8inf = d2i_PKCS8_PRIV_KEY_INFO_bio(membio, NULL))
|| !TEST_ptr(pkey = EVP_PKCS82PKEY(p8inf))
|| !TEST_int_eq(ERR_get_error(), 0)) {
goto done;
}

ok = 1;
done:
EVP_PKEY_free(pkey);
PKCS8_PRIV_KEY_INFO_free(p8inf);
BIO_free_all(membio);
OSSL_PROVIDER_unload(provider);
return ok;
}
#endif

/* This uses kExampleRSAKeyDER and kExampleRSAKeyPKCS8 to verify encoding */
Expand Down Expand Up @@ -2891,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
Expand Down

0 comments on commit 82f360a

Please sign in to comment.