From fa0a9d715e7e35d4f597683c16b643343245fa26 Mon Sep 17 00:00:00 2001 From: "Dr. Stephen Henson" Date: Wed, 30 Mar 2016 21:46:13 +0100 Subject: [PATCH] Fix X509_PUBKEY cached key handling. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Don't decode a public key in X509_PUBKEY_get0(): that is handled when the key is parsed using x509_pubkey_decode() instead. Reviewed-by: Emilia Käsper --- crypto/x509/x509_err.c | 3 +- crypto/x509/x_pubkey.c | 88 ++++++++++++++++++++++++++++++------------ include/openssl/x509.h | 1 + 3 files changed, 66 insertions(+), 26 deletions(-) diff --git a/crypto/x509/x509_err.c b/crypto/x509/x509_err.c index 9754c128c3..90a22de41c 100644 --- a/crypto/x509/x509_err.c +++ b/crypto/x509/x509_err.c @@ -1,5 +1,5 @@ /* ==================================================================== - * Copyright (c) 1999-2015 The OpenSSL Project. All rights reserved. + * Copyright (c) 1999-2016 The OpenSSL Project. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -110,6 +110,7 @@ static ERR_STRING_DATA X509_str_functs[] = { {ERR_FUNC(X509_F_X509_NAME_ONELINE), "X509_NAME_oneline"}, {ERR_FUNC(X509_F_X509_NAME_PRINT), "X509_NAME_print"}, {ERR_FUNC(X509_F_X509_PRINT_EX_FP), "X509_print_ex_fp"}, + {ERR_FUNC(X509_F_X509_PUBKEY_DECODE), "x509_pubkey_decode"}, {ERR_FUNC(X509_F_X509_PUBKEY_GET0), "X509_PUBKEY_get0"}, {ERR_FUNC(X509_F_X509_PUBKEY_SET), "X509_PUBKEY_set"}, {ERR_FUNC(X509_F_X509_REQ_CHECK_PRIVATE_KEY), diff --git a/crypto/x509/x_pubkey.c b/crypto/x509/x_pubkey.c index b9079b58d1..485d768dc4 100644 --- a/crypto/x509/x_pubkey.c +++ b/crypto/x509/x_pubkey.c @@ -71,6 +71,8 @@ struct X509_pubkey_st { EVP_PKEY *pkey; }; +static int x509_pubkey_decode(EVP_PKEY **pk, X509_PUBKEY *key); + /* Minor tweak to operation: free up EVP_PKEY */ static int pubkey_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, void *exarg) @@ -83,11 +85,13 @@ static int pubkey_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, X509_PUBKEY *pubkey = (X509_PUBKEY *)*pval; EVP_PKEY_free(pubkey->pkey); /* - * Remove any errors from the queue: subsequent decode attempts will - * return an appropriate error. + * Opportunistically decode the key but remove any non fatal errors + * from the queue. Subsequent explicit attempts to decode/use the key + * will return an appropriate error. */ ERR_set_mark(); - pubkey->pkey = X509_PUBKEY_get0(pubkey); + if (x509_pubkey_decode(&pubkey->pkey, pubkey) == -1) + return 0; ERR_pop_to_mark(); } return 1; @@ -128,6 +132,8 @@ int X509_PUBKEY_set(X509_PUBKEY **x, EVP_PKEY *pkey) X509_PUBKEY_free(*x); *x = pk; + pk->pkey = pkey; + EVP_PKEY_up_ref(pkey); return 1; error: @@ -135,44 +141,76 @@ int X509_PUBKEY_set(X509_PUBKEY **x, EVP_PKEY *pkey) return 0; } -EVP_PKEY *X509_PUBKEY_get0(X509_PUBKEY *key) -{ - EVP_PKEY *ret = NULL; - - if (key == NULL) - goto error; +/* + * Attempt to decode a public key. + * Returns 1 on success, 0 for a decode failure and -1 for a fatal + * error e.g. malloc failure. + */ - if (key->pkey != NULL) - return key->pkey; - if (key->public_key == NULL) - goto error; +static int x509_pubkey_decode(EVP_PKEY **ppkey, X509_PUBKEY *key) + { + EVP_PKEY *pkey = EVP_PKEY_new(); - if ((ret = EVP_PKEY_new()) == NULL) { - X509err(X509_F_X509_PUBKEY_GET0, ERR_R_MALLOC_FAILURE); - goto error; + if (pkey == NULL) { + X509err(X509_F_X509_PUBKEY_DECODE, ERR_R_MALLOC_FAILURE); + return -1; } - if (!EVP_PKEY_set_type(ret, OBJ_obj2nid(key->algor->algorithm))) { - X509err(X509_F_X509_PUBKEY_GET0, X509_R_UNSUPPORTED_ALGORITHM); + if (!EVP_PKEY_set_type(pkey, OBJ_obj2nid(key->algor->algorithm))) { + X509err(X509_F_X509_PUBKEY_DECODE, X509_R_UNSUPPORTED_ALGORITHM); goto error; } - if (ret->ameth->pub_decode) { - if (!ret->ameth->pub_decode(ret, key)) { - X509err(X509_F_X509_PUBKEY_GET0, X509_R_PUBLIC_KEY_DECODE_ERROR); + if (pkey->ameth->pub_decode) { + /* + * Treat any failure of pub_decode as a decode error. In + * future we could have different return codes for decode + * errors and fatal errors such as malloc failure. + */ + if (!pkey->ameth->pub_decode(pkey, key)) { + X509err(X509_F_X509_PUBKEY_DECODE, X509_R_PUBLIC_KEY_DECODE_ERROR); goto error; } } else { - X509err(X509_F_X509_PUBKEY_GET0, X509_R_METHOD_NOT_SUPPORTED); + X509err(X509_F_X509_PUBKEY_DECODE, X509_R_METHOD_NOT_SUPPORTED); goto error; } - return ret; + *ppkey = pkey; + return 1; error: - EVP_PKEY_free(ret); - return (NULL); + EVP_PKEY_free(pkey); + return 0; +} + +EVP_PKEY *X509_PUBKEY_get0(X509_PUBKEY *key) +{ + EVP_PKEY *ret = NULL; + + if (key == NULL || key->public_key == NULL) + return NULL; + + if (key->pkey != NULL) + return key->pkey; + + /* + * When the key ASN.1 is initially parsed an attempt is made to + * decode the public key and cache the EVP_PKEY structure. If this + * operation fails the cached value will be NULL. Parsing continues + * to allow parsing of unknown key types or unsupported forms. + * We repeat the decode operation so the appropriate errors are left + * in the queue. + */ + x509_pubkey_decode(&ret, key); + /* If decode doesn't fail something bad happened */ + if (ret != NULL) { + X509err(X509_F_X509_PUBKEY_GET0, ERR_R_INTERNAL_ERROR); + EVP_PKEY_free(ret); + } + + return NULL; } EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key) diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 0ad753c71e..4f22dc3050 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -1078,6 +1078,7 @@ void ERR_load_X509_strings(void); # define X509_F_X509_NAME_ONELINE 116 # define X509_F_X509_NAME_PRINT 117 # define X509_F_X509_PRINT_EX_FP 118 +# define X509_F_X509_PUBKEY_DECODE 148 # define X509_F_X509_PUBKEY_GET0 119 # define X509_F_X509_PUBKEY_SET 120 # define X509_F_X509_REQ_CHECK_PRIVATE_KEY 144 -- 2.25.1