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 <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/13023)

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) {
+    if (!eckey)
-        ECerr(EC_F_ECKEY_PUB_DECODE, ERR_R_EC_LIB);
         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:
+ err:
-    ECerr(0, ERR_R_EC_LIB);
- ecerr:
     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) {
+    if ((ec = d2i_ECPrivateKey(NULL, pder, derlen)) == NULL)
-        ECerr(EC_F_OLD_EC_PRIV_DECODE, EC_R_DECODE_ERROR);
         return 0;
-    }
     EVP_PKEY_assign_EC_KEY(pkey, ec);
     return 1;
 }

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:

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)) {
+        if (!pkey->ameth->priv_decode_with_libctx(pkey, p8, libctx, propq))
-            EVPerr(0, EVP_R_PRIVATE_KEY_DECODE_ERROR);
             goto error;
-        }
     } else if (pkey->ameth->priv_decode != NULL) {
         if (!pkey->ameth->priv_decode(pkey, p8)) {
             EVPerr(0, EVP_R_PRIVATE_KEY_DECODE_ERROR);

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                                                            \

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)) {
+        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_DECODE, X509_R_METHOD_NOT_SUPPORTED);
         goto error;

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();
+    CLEAR_ERR_MARK();
-    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();
 
     if (pkey != NULL) {
         /*

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-----

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");
+}