The branch master has been updated via 4a24d6050bee3cafd3e1eb42b800ece23bdba6b5 (commit) via 66066e1bba041459c2f879666b79e4a2158f5905 (commit) via 9032c2c11b2f14dcdbd253b470abc595a07a6c51 (commit) from e1f5a92df4b612de8eac7ca538ef44f4b1deec5a (commit)
- Log ----------------------------------------------------------------- commit 4a24d6050bee3cafd3e1eb42b800ece23bdba6b5 Author: Dr. David von Oheimb <david.von.ohe...@siemens.com> Date: Tue Sep 29 10:33:22 2020 +0200 EC_GROUP_new_by_curve_name_with_libctx(): Add name of unknown group to error output Reviewed-by: Tomas Mraz <tm...@fedoraproject.org> (Merged from https://github.com/openssl/openssl/pull/13023) commit 66066e1bba041459c2f879666b79e4a2158f5905 Author: Dr. David von Oheimb <david.von.ohe...@siemens.com> Date: Mon Sep 28 16:14:14 2020 +0200 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 <tm...@fedoraproject.org> (Merged from https://github.com/openssl/openssl/pull/13023) commit 9032c2c11b2f14dcdbd253b470abc595a07a6c51 Author: Dr. David von Oheimb <david.von.ohe...@siemens.com> Date: Mon Sep 28 19:44:49 2020 +0200 25-test_x509.t: Add test for suitable error report loading unsupported sm2 cert Reviewed-by: Tomas Mraz <tm...@fedoraproject.org> (Merged from https://github.com/openssl/openssl/pull/13023) ----------------------------------------------------------------------- Summary of changes: crypto/ec/ec_ameth.c | 17 +++------- crypto/ec/ec_curve.c | 4 +++ 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 +++---- .../implementations/encode_decode/decode_der2key.c | 34 ++++++++++++++----- test/certs/server-dsa-pubkey.pem | 20 ++++++++++++ test/recipes/25-test_x509.t | 15 ++++++--- test/recipes/30-test_evp.t | 38 +++++++++++++++++++++- 10 files changed, 124 insertions(+), 36 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/ec/ec_curve.c b/crypto/ec/ec_curve.c index bf02c261f7..a63a8535c3 100644 --- a/crypto/ec/ec_curve.c +++ b/crypto/ec/ec_curve.c @@ -18,6 +18,7 @@ #include "ec_local.h" #include <openssl/err.h> #include <openssl/obj_mac.h> +#include <openssl/objects.h> #include <openssl/opensslconf.h> #include "internal/nelem.h" #include "e_os.h" /* strcasecmp required by windows */ @@ -3298,6 +3299,9 @@ EC_GROUP *EC_GROUP_new_by_curve_name_with_libctx(OPENSSL_CTX *libctx, if ((curve = ec_curve_nid2curve(nid)) == NULL || (ret = ec_group_new_from_data(libctx, propq, *curve)) == NULL) { ECerr(0, EC_R_UNKNOWN_GROUP); +#ifndef FIPS_MODULE + ERR_add_error_data(2, "name=", OBJ_nid2sn(nid)); +#endif return NULL; } 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 <openssl/bio.h> #include <openssl/params.h> #include <openssl/provider.h> +#include <openssl/evperr.h> +#include <openssl/ecerr.h> +#include <openssl/x509err.h> #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/25-test_x509.t b/test/recipes/25-test_x509.t index 4b37ee6464..f5b4245960 100644 --- a/test/recipes/25-test_x509.t +++ b/test/recipes/25-test_x509.t @@ -16,7 +16,7 @@ use OpenSSL::Test qw/:DEFAULT srctop_file/; setup("test_x509"); -plan tests => 14; +plan tests => 15; require_ok(srctop_file('test','recipes','tconversion.pl')); @@ -105,9 +105,10 @@ sub test_errors { # actually tests diagnostics of OSSL_STORE my ($expected, $cert, @opts) = @_; my $infile = srctop_file('test', 'certs', $cert); my @args = qw(openssl x509 -in); - push(@args, "$infile", @opts); + push(@args, $infile, @opts); my $tmpfile = 'out.txt'; - my $res = !run(app([@args], stderr => $tmpfile)); + my $res = grep(/-text/, @opts) ? run(app([@args], stdout => $tmpfile)) + : !run(app([@args], stderr => $tmpfile)); my $found = 0; open(my $in, '<', $tmpfile) or die "Could not open file $tmpfile"; while(<$in>) { @@ -116,7 +117,7 @@ sub test_errors { # actually tests diagnostics of OSSL_STORE $found = 1 if m/$expected/; # output must include $expected } close $in; - unlink $tmpfile; + # $tmpfile is kept to help with investigation in case of failure return $res && $found; } @@ -124,3 +125,9 @@ ok(test_errors("Can't open any-dir/", "root-cert.pem", '-out', 'any-dir/'), "load root-cert errors"); ok(test_errors("RC2-40-CBC", "v3-certs-RC2.p12", '-passin', 'pass:v3-certs'), "load v3-certs-RC2 no asn1 errors"); +SKIP: { + skip "sm2 not disabled", 1 if !disabled("sm2"); + + ok(test_errors("unknown group|unsupported algorithm", "sm2.pem", '-text'), + "error loading unsupported sm2 cert"); +} 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"); +}