From 66066e1bba041459c2f879666b79e4a2158f5905 Mon Sep 17 00:00:00 2001 From: "Dr. David von Oheimb" Date: Mon, 28 Sep 2020 16:14:14 +0200 Subject: [PATCH] Prune low-level ASN.1 parse errors from error queue in der2key_decode() etc. Also adds error output tests on loading key files with unsupported algorithms to 30-test_evp.t Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/13023) --- crypto/ec/ec_ameth.c | 17 +++------ crypto/encode_decode/decoder_lib.c | 15 +++++++- crypto/evp/evp_pkey.c | 4 +- crypto/store/store_result.c | 1 + crypto/x509/x_pubkey.c | 12 +++--- .../encode_decode/decode_der2key.c | 34 ++++++++++++----- test/certs/server-dsa-pubkey.pem | 20 ++++++++++ test/recipes/30-test_evp.t | 38 ++++++++++++++++++- 8 files changed, 109 insertions(+), 32 deletions(-) create mode 100644 test/certs/server-dsa-pubkey.pem diff --git a/crypto/ec/ec_ameth.c b/crypto/ec/ec_ameth.c index b586a43539..3312faa336 100644 --- a/crypto/ec/ec_ameth.c +++ b/crypto/ec/ec_ameth.c @@ -172,10 +172,8 @@ static int eckey_pub_decode(EVP_PKEY *pkey, const X509_PUBKEY *pubkey) eckey = eckey_type2param(ptype, pval, libctx, propq); - if (!eckey) { - ECerr(EC_F_ECKEY_PUB_DECODE, ERR_R_EC_LIB); + if (!eckey) return 0; - } /* We have parameters now set public key */ if (!o2i_ECPublicKey(&eckey, &p, pklen)) { @@ -224,22 +222,19 @@ static int eckey_priv_decode_with_libctx(EVP_PKEY *pkey, X509_ALGOR_get0(NULL, &ptype, &pval, palg); eckey = eckey_type2param(ptype, pval, libctx, propq); - if (eckey == NULL) - goto ecliberr; + goto err; /* We have parameters now set private key */ if (!d2i_ECPrivateKey(&eckey, &p, pklen)) { ECerr(0, EC_R_DECODE_ERROR); - goto ecerr; + goto err; } EVP_PKEY_assign_EC_KEY(pkey, eckey); return 1; - ecliberr: - ECerr(0, ERR_R_EC_LIB); - ecerr: + err: EC_KEY_free(eckey); return 0; } @@ -472,10 +467,8 @@ static int old_ec_priv_decode(EVP_PKEY *pkey, { EC_KEY *ec; - if ((ec = d2i_ECPrivateKey(NULL, pder, derlen)) == NULL) { - ECerr(EC_F_OLD_EC_PRIV_DECODE, EC_R_DECODE_ERROR); + if ((ec = d2i_ECPrivateKey(NULL, pder, derlen)) == NULL) return 0; - } EVP_PKEY_assign_EC_KEY(pkey, ec); return 1; } diff --git a/crypto/encode_decode/decoder_lib.c b/crypto/encode_decode/decoder_lib.c index 0bc772e43b..0411da41f4 100644 --- a/crypto/encode_decode/decoder_lib.c +++ b/crypto/encode_decode/decoder_lib.c @@ -11,6 +11,9 @@ #include #include #include +#include +#include +#include #include "internal/passphrase.h" #include "crypto/decoder.h" #include "encoder_local.h" @@ -424,7 +427,7 @@ static int decoder_process(const OSSL_PARAM params[], void *arg) BIO *bio = data->bio; long loc; size_t i; - int ok = 0; + int err, ok = 0; /* For recursions */ struct decoder_process_data_st new_data; @@ -532,6 +535,16 @@ static int decoder_process(const OSSL_PARAM params[], void *arg) &new_data.ctx->pwdata); if (ok) break; + err = ERR_peek_last_error(); + if ((ERR_GET_LIB(err) == ERR_LIB_EVP + && ERR_GET_REASON(err) == EVP_R_UNSUPPORTED_PRIVATE_KEY_ALGORITHM) +#ifndef OPENSSL_NO_EC + || (ERR_GET_LIB(err) == ERR_LIB_EC + && ERR_GET_REASON(err) == EC_R_UNKNOWN_GROUP) +#endif + || (ERR_GET_LIB(err) == ERR_LIB_X509 + && ERR_GET_REASON(err) == X509_R_UNSUPPORTED_ALGORITHM)) + break; /* fatal error; preserve it on the error queue and stop */ } end: diff --git a/crypto/evp/evp_pkey.c b/crypto/evp/evp_pkey.c index f31d1d68f8..45666a2c42 100644 --- a/crypto/evp/evp_pkey.c +++ b/crypto/evp/evp_pkey.c @@ -41,10 +41,8 @@ EVP_PKEY *EVP_PKCS82PKEY_with_libctx(const PKCS8_PRIV_KEY_INFO *p8, } if (pkey->ameth->priv_decode_with_libctx != NULL) { - if (!pkey->ameth->priv_decode_with_libctx(pkey, p8, libctx, propq)) { - EVPerr(0, EVP_R_PRIVATE_KEY_DECODE_ERROR); + if (!pkey->ameth->priv_decode_with_libctx(pkey, p8, libctx, propq)) goto error; - } } else if (pkey->ameth->priv_decode != NULL) { if (!pkey->ameth->priv_decode(pkey, p8)) { EVPerr(0, EVP_R_PRIVATE_KEY_DECODE_ERROR); diff --git a/crypto/store/store_result.c b/crypto/store/store_result.c index c3f21eedad..363d25adbf 100644 --- a/crypto/store/store_result.c +++ b/crypto/store/store_result.c @@ -88,6 +88,7 @@ static int try_pkcs12(struct extracted_param_data_st *, OSSL_STORE_INFO **, \ if (ERR_GET_LIB(err) == ERR_LIB_ASN1 \ && (ERR_GET_REASON(err) == ASN1_R_UNKNOWN_PUBLIC_KEY_TYPE \ + || ERR_GET_REASON(err) == ASN1_R_NO_MATCHING_CHOICE_TYPE \ || ERR_GET_REASON(err) == ERR_R_NESTED_ASN1_ERROR)) \ ERR_pop_to_mark(); \ else \ diff --git a/crypto/x509/x_pubkey.c b/crypto/x509/x_pubkey.c index a4d3c9fa5e..d63a33e301 100644 --- a/crypto/x509/x_pubkey.c +++ b/crypto/x509/x_pubkey.c @@ -41,12 +41,12 @@ static int x509_pubkey_decode(EVP_PKEY **pk, const X509_PUBKEY *key); static int pubkey_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, void *exarg) { + X509_PUBKEY *pubkey = (X509_PUBKEY *)*pval; + if (operation == ASN1_OP_FREE_POST) { - X509_PUBKEY *pubkey = (X509_PUBKEY *)*pval; EVP_PKEY_free(pubkey->pkey); } else if (operation == ASN1_OP_D2I_POST) { /* Attempt to decode public key and cache in pubkey structure. */ - X509_PUBKEY *pubkey = (X509_PUBKEY *)*pval; EVP_PKEY_free(pubkey->pkey); pubkey->pkey = NULL; /* @@ -55,8 +55,10 @@ static int pubkey_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, * will return an appropriate error. */ ERR_set_mark(); - if (x509_pubkey_decode(&pubkey->pkey, pubkey) == -1) + if (x509_pubkey_decode(&pubkey->pkey, pubkey) == -1) { + ERR_clear_last_mark(); return 0; + } ERR_pop_to_mark(); } return 1; @@ -180,10 +182,8 @@ static int x509_pubkey_decode(EVP_PKEY **ppkey, const X509_PUBKEY *key) * 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); + if (!pkey->ameth->pub_decode(pkey, key)) goto error; - } } else { X509err(X509_F_X509_PUBKEY_DECODE, X509_R_METHOD_NOT_SUPPORTED); goto error; diff --git a/providers/implementations/encode_decode/decode_der2key.c b/providers/implementations/encode_decode/decode_der2key.c index 64b085673a..0b6debf506 100644 --- a/providers/implementations/encode_decode/decode_der2key.c +++ b/providers/implementations/encode_decode/decode_der2key.c @@ -30,6 +30,25 @@ #include "prov/providercommonerr.h" #include "endecoder_local.h" +#define SET_ERR_MARK() ERR_set_mark() +#define CLEAR_ERR_MARK() \ + do { \ + int err = ERR_peek_last_error(); \ + \ + if (ERR_GET_LIB(err) == ERR_LIB_ASN1 \ + && (ERR_GET_REASON(err) == ASN1_R_HEADER_TOO_LONG \ + || ERR_GET_REASON(err) == ASN1_R_UNSUPPORTED_TYPE \ + || ERR_GET_REASON(err) == ERR_R_NESTED_ASN1_ERROR)) \ + ERR_pop_to_mark(); \ + else \ + ERR_clear_last_mark(); \ + } while(0) +#define RESET_ERR_MARK() \ + do { \ + CLEAR_ERR_MARK(); \ + SET_ERR_MARK(); \ + } while(0) + static int read_der(PROV_CTX *provctx, OSSL_CORE_BIO *cin, unsigned char **data, long *len) { @@ -165,9 +184,9 @@ static int der2key_decode(void *vctx, OSSL_CORE_BIO *cin, long new_der_len; EVP_PKEY *pkey = NULL; void *key = NULL; - int err, ok = 0; + int ok = 0; - ERR_set_mark(); + SET_ERR_MARK(); if (!read_der(ctx->provctx, cin, &der, &der_len)) goto err; @@ -180,16 +199,19 @@ static int der2key_decode(void *vctx, OSSL_CORE_BIO *cin, der = new_der; der_len = new_der_len; } + RESET_ERR_MARK(); derp = der; pkey = d2i_PrivateKey_ex(ctx->desc->type, NULL, &derp, der_len, libctx, NULL); if (pkey == NULL) { + RESET_ERR_MARK(); derp = der; pkey = d2i_PUBKEY_ex(NULL, &derp, der_len, libctx, NULL); } if (pkey == NULL) { + RESET_ERR_MARK(); derp = der; pkey = d2i_KeyParams(ctx->desc->type, NULL, &derp, der_len); } @@ -198,13 +220,7 @@ static int der2key_decode(void *vctx, OSSL_CORE_BIO *cin, * Prune low-level ASN.1 parse errors from error queue, assuming that * this is called by decoder_process() in a loop trying several formats. */ - err = ERR_peek_last_error(); - if (ERR_GET_LIB(err) == ERR_LIB_ASN1 - && (ERR_GET_REASON(err) == ASN1_R_HEADER_TOO_LONG - || ERR_GET_REASON(err) == ERR_R_NESTED_ASN1_ERROR)) - ERR_pop_to_mark(); - else - ERR_clear_last_mark(); + CLEAR_ERR_MARK(); if (pkey != NULL) { /* diff --git a/test/certs/server-dsa-pubkey.pem b/test/certs/server-dsa-pubkey.pem new file mode 100644 index 0000000000..e5b5e6a5af --- /dev/null +++ b/test/certs/server-dsa-pubkey.pem @@ -0,0 +1,20 @@ +-----BEGIN PUBLIC KEY----- +MIIDSDCCAjoGByqGSM44BAEwggItAoIBAQD+P3LcpaA+AYu9M1gSsHi8fixl7VPC +sKK96oaH7/ZJqvOD0TdASkn+4Td8SPvkc+KG2bBbmp39FCxGpa4d8CRLKVbIHAFt +aKHIDFuMlPuFnsiaU0uWN/s3lROhAHWrTiODhehFM+NiPrAOJmtXQURBoeQ07t4H +oyKz7sUyTF2qotw1JDvBRb6JXw+13Z2a1iZGJopLZN3RicvoHee3rYEsM5AHMS3c +ntYX2NhQUHjiQ451iL2OkFJtVeaUoX5JV6KYSzz4lzNlYwJfF/Tzac/+l1aFA1ND +bNFcQ1UC0JXscKeT/J2Wo8kRwpx042UKaayw5jkOv3GndgKCOaCe29UrAiEAh8hM +JV/kKTLolNr6kV87KV8eTaJfrnSRS2E3ToOhWH0CggEBAOd/YKl8svYqvJtThaOs +mVETeXwEvz/MLqpj4hZr029Oqps7z6OmeZ2er7aldxC5+BKMxCfPlhFo0iQ9XITp ++J7UqS3qrRZqAnxMjd6VmEGXKWOoeAc0CpEzR1QNkjKodzgstQj5oYbiiPG0SgCt +BV4I1b/IuKzkjcLxQaF+8Rob/lzLBwA6pFjZNa6FcDjthmtH2pC+zI760sv05rbZ +GcXDj8G0SLsvbkrfiRIn/8LkgBpoTWpKfa8BmvYtt9WI/CYkbeQYIwM9sXUPwRSD +1VONSg5bXTW3Sxmzy3Yfy9RYt+suMKzi78oSv81e5BoL1D2HtfxSAFQbiJU3kipx +vhsDggEGAAKCAQEArDidnkCegHb/itBTFeyGsebv+I8Z93V3jGcKPOs3s1wqB/+H +RL5ERlhQOq/lfYPigUFKhfC8tlCVAM+MtUDqXCzqAkomw0yX8oVkp9plswxHKlqj +zKr6PWLOJGp/NDBAL1ZcUzHB1omvmkUHy9pYiapVVNUuUdL2Z5EvDze8jQoiR0k9 +zgMKiH+MyCfV0tLo8W8djFJPlIM9Ypa7DH4fazcEfRuzq1jvK/uX4+HWmg3Nswdh +5eysb++RqtJSUBtGT3tAQY59WjBf2nXMG0nkZGkT7TCJ6icvNdbSl1AlAGMV/nZN +3PFsFH17L8uMUYS7V5PWiqQTxe5COHqpGumo9A== +-----END PUBLIC KEY----- diff --git a/test/recipes/30-test_evp.t b/test/recipes/30-test_evp.t index 17e2d17007..9a8bb74bb6 100644 --- a/test/recipes/30-test_evp.t +++ b/test/recipes/30-test_evp.t @@ -110,7 +110,8 @@ push @defltfiles, qw(evppkey_sm2.txt) unless $no_sm2; plan tests => ($no_fips ? 0 : 1) # FIPS install test + (scalar(@configs) * scalar(@files)) - + scalar(@defltfiles); + + scalar(@defltfiles) + + 3; # error output tests unless ($no_fips) { my $infile = bldtop_file('providers', platform->dso('fips')); @@ -139,3 +140,38 @@ foreach my $f ( @defltfiles ) { data_file("$f")])), "running evp_test -config $conf $f"); } + +sub test_errors { # actually tests diagnostics of OSSL_STORE + my ($expected, $key, @opts) = @_; + my $infile = srctop_file('test', 'certs', $key); + my @args = qw(openssl pkey -in); + push(@args, $infile, @opts); + my $tmpfile = 'out.txt'; + my $res = !run(app([@args], stderr => $tmpfile)); + my $found = 0; + open(my $in, '<', $tmpfile) or die "Could not open file $tmpfile"; + while(<$in>) { + print; # this may help debugging + $res &&= !m/asn1 encoding/; # output must not include ASN.1 parse errors + $found = 1 if m/$expected/; # output must include $expected + } + close $in; + # $tmpfile is kept to help with investigation in case of failure + return $res && $found; +} + +SKIP: { + skip "DSA not disabled", 2 if !disabled("dsa"); + + ok(test_errors("unsupported algorithm", "server-dsa-key.pem"), + "error loading unsupported dsa private key"); + ok(test_errors("unsupported algorithm", "server-dsa-pubkey.pem", "-pubin"), + "error loading unsupported dsa public key"); +} + +SKIP: { + skip "sm2 not disabled", 1 if !disabled("sm2"); + + ok(test_errors("unknown group|unsupported algorithm", "sm2.key"), + "error loading unsupported sm2 private key"); +} -- 2.34.1