STORE: Discard the error report filter in crypto/store/store_result.c

The error report filter was fragile, as it could potentially have to
be updated when other parts of libcrypto got updated, making a goose
chase and a maintenance problem.

We change this to regard d2i errors as something we don't care so much
about, since they are mainly part of the guessing mechanism.  The
success of the ossl_store_handle_load_result() call is based on
whether an object was actually created or not anyway.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14834)

1 files changed, 50 insertions, 38 deletions

diff --git a/crypto/store/store_result.c b/crypto/store/store_result.c
index 72f054be17..f69045994d 100644
--- a/crypto/store/store_result.c
+++ b/crypto/store/store_result.c
@@ -82,25 +82,6 @@ static int try_crl(struct extracted_param_data_st *, OSSL_STORE_INFO **,
 static int try_pkcs12(struct extracted_param_data_st *, OSSL_STORE_INFO **,
                       OSSL_STORE_CTX *, OSSL_LIB_CTX *, const char *);
 
-#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_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                                                            \
-            ERR_clear_last_mark();                                      \
-    } while(0)
-#define RESET_ERR_MARK()                                                \
-    do {                                                                \
-        CLEAR_ERR_MARK();                                               \
-        SET_ERR_MARK();                                                 \
-    } while(0)
-
 int ossl_store_handle_load_result(const OSSL_PARAM params[], void *arg)
 {
     struct ossl_load_result_data_st *cbdata = arg;
@@ -145,22 +126,16 @@ int ossl_store_handle_load_result(const OSSL_PARAM params[], void *arg)
      * The helper functions return 0 on actual errors, otherwise 1, even if
      * they didn't fill out |*v|.
      */
-    SET_ERR_MARK();
     if (!try_name(&helper_data, v))
         goto err;
-    RESET_ERR_MARK();
     if (!try_key(&helper_data, v, ctx, provider, libctx, propq))
         goto err;
-    RESET_ERR_MARK();
     if (!try_cert(&helper_data, v, libctx, propq))
         goto err;
-    RESET_ERR_MARK();
     if (!try_crl(&helper_data, v, libctx, propq))
         goto err;
-    RESET_ERR_MARK();
     if (!try_pkcs12(&helper_data, v, ctx, libctx, propq))
         goto err;
-    CLEAR_ERR_MARK();
 
     return (*v != NULL);
  err:
@@ -304,16 +279,19 @@ static EVP_PKEY *try_key_value_legacy(struct extracted_param_data_st *data,
     const unsigned char *der = data->octet_data, *derp;
     long der_len = (long)data->octet_data_size;
 
-    SET_ERR_MARK();
     /* Try PUBKEY first, that's a real easy target */
     if (ctx->expected_type == 0
         || ctx->expected_type == OSSL_STORE_INFO_PUBKEY) {
+        /* Discard DER decoding errors, it only means we return "empty handed" */
+        if (ctx->expected_type == 0)
+            ERR_set_mark();
         derp = der;
         pk = d2i_PUBKEY_ex(NULL, &derp, der_len, libctx, propq);
+        if (ctx->expected_type == 0)
+            ERR_pop_to_mark();
+
         if (pk != NULL)
             *store_info_new = OSSL_STORE_INFO_new_PUBKEY;
-
-        RESET_ERR_MARK();
     }
 
     /* Try private keys next */
@@ -324,9 +302,18 @@ static EVP_PKEY *try_key_value_legacy(struct extracted_param_data_st *data,
         X509_SIG *p8 = NULL;
         PKCS8_PRIV_KEY_INFO *p8info = NULL;
 
-        /* See if it's an encrypted PKCS#8 and decrypt it */
+        /*
+         * See if it's an encrypted PKCS#8 and decrypt it.  Discard DER
+         * decoding errors, a failed decode only means we return "empty handed"
+         */
+        if (ctx->expected_type == 0)
+            ERR_set_mark();
         derp = der;
-        if ((p8 = d2i_X509_SIG(NULL, &derp, der_len)) != NULL) {
+        p8 = d2i_X509_SIG(NULL, &derp, der_len);
+        if (ctx->expected_type == 0)
+            ERR_pop_to_mark();
+
+        if (p8 != NULL) {
             char pbuf[PEM_BUFSIZE];
             size_t plen = 0;
 
@@ -351,17 +338,22 @@ static EVP_PKEY *try_key_value_legacy(struct extracted_param_data_st *data,
             }
             X509_SIG_free(p8);
         }
-        RESET_ERR_MARK();
 
         /*
          * If the encrypted PKCS#8 couldn't be decrypted,
          * |der| is NULL
          */
         if (der != NULL) {
-            /* Try to unpack an unencrypted PKCS#8, that's easy */
+            /*
+             * Try to unpack an unencrypted PKCS#8, that's easy. Discard DER
+             * decoding errors, a failed decode only means we return "empty
+             * handed"
+             */
+            ERR_set_mark();
             derp = der;
             p8info = d2i_PKCS8_PRIV_KEY_INFO(NULL, &derp, der_len);
-            RESET_ERR_MARK();
+            ERR_pop_to_mark();
+
             if (p8info != NULL) {
                 pk = EVP_PKCS82PKEY_ex(p8info, libctx, propq);
                 PKCS8_PRIV_KEY_INFO_free(p8info);
@@ -373,7 +365,6 @@ static EVP_PKEY *try_key_value_legacy(struct extracted_param_data_st *data,
 
         OPENSSL_free(new_der);
     }
-    CLEAR_ERR_MARK();
 
     return pk;
 }
@@ -473,11 +464,16 @@ static int try_cert(struct extracted_param_data_st *data, OSSL_STORE_INFO **v,
             && (strcasecmp(data->data_type, PEM_STRING_X509_TRUSTED) == 0))
             ignore_trusted = 0;
 
+        /* Discard DER decoding errors, it only means we return "empty handed" */
+        if (data->object_type == OSSL_OBJECT_UNKNOWN)
+            ERR_set_mark();
         cert = d2i_X509_AUX(NULL, (const unsigned char **)&data->octet_data,
                             data->octet_data_size);
         if (cert == NULL && ignore_trusted)
             cert = d2i_X509(NULL, (const unsigned char **)&data->octet_data,
                             data->octet_data_size);
+        if (data->object_type == OSSL_OBJECT_UNKNOWN)
+            ERR_pop_to_mark();
 
         if (cert != NULL)
             /* We determined the object type */
@@ -504,8 +500,14 @@ static int try_crl(struct extracted_param_data_st *data, OSSL_STORE_INFO **v,
         || data->object_type == OSSL_OBJECT_CRL) {
         X509_CRL *crl;
 
+        /* Discard DER decoding errors, it only means we return "empty handed" */
+        if (data->object_type == OSSL_OBJECT_UNKNOWN)
+            ERR_set_mark();
         crl = d2i_X509_CRL(NULL, (const unsigned char **)&data->octet_data,
                            data->octet_data_size);
+        if (data->object_type == OSSL_OBJECT_UNKNOWN)
+            ERR_pop_to_mark();
+
         if (crl != NULL)
             /* We determined the object type */
             data->object_type = OSSL_OBJECT_CRL;
@@ -528,13 +530,20 @@ static int try_pkcs12(struct extracted_param_data_st *data, OSSL_STORE_INFO **v,
                       OSSL_STORE_CTX *ctx,
                       OSSL_LIB_CTX *libctx, const char *propq)
 {
+    int ok = 1;
+
     /* There is no specific object type for PKCS12 */
     if (data->object_type == OSSL_OBJECT_UNKNOWN) {
         /* Initial parsing */
         PKCS12 *p12;
 
-        if ((p12 = d2i_PKCS12(NULL, (const unsigned char **)&data->octet_data,
-                              data->octet_data_size)) != NULL) {
+        /* Discard DER decoding errors, it only means we return "empty handed" */
+        ERR_set_mark();
+        p12 = d2i_PKCS12(NULL, (const unsigned char **)&data->octet_data,
+                         data->octet_data_size);
+        ERR_pop_to_mark();
+
+        if (p12 != NULL) {
             char *pass = NULL;
             char tpass[PEM_BUFSIZE];
             size_t tpass_len;
@@ -544,6 +553,8 @@ static int try_pkcs12(struct extracted_param_data_st *data, OSSL_STORE_INFO **v,
 
             data->object_type = OSSL_OBJECT_PKCS12;
 
+            ok = 0;              /* Assume decryption or parse error */
+
             if (PKCS12_verify_mac(p12, "", 0)
                 || PKCS12_verify_mac(p12, NULL, 0)) {
                 pass = "";
@@ -577,7 +588,8 @@ static int try_pkcs12(struct extracted_param_data_st *data, OSSL_STORE_INFO **v,
                 OSSL_STORE_INFO *osi_pkey = NULL;
                 OSSL_STORE_INFO *osi_cert = NULL;
                 OSSL_STORE_INFO *osi_ca = NULL;
-                int ok = 1;
+
+                ok = 1;          /* Parsing went through correctly! */
 
                 if ((infos = sk_OSSL_STORE_INFO_new_null()) != NULL) {
                     if (pkey != NULL) {
@@ -627,5 +639,5 @@ static int try_pkcs12(struct extracted_param_data_st *data, OSSL_STORE_INFO **v,
         *v = sk_OSSL_STORE_INFO_shift(ctx->cached_info);
     }
 
-    return 1;
+    return ok;
 }