APPS load_key_certs_crls(): Make file access errors much more readable

This reverts part of commit ef0449135c4e4e7f using a less invasive suppression.

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/16452)

(cherry picked from commit 6e2499474cb96b28a51df1da25cc72f1cf342fad)
Reviewed-by: Hugo Landau <hlandau@openssl.org>

diff --git a/apps/lib/apps.c b/apps/lib/apps.c
index 9456a21868a45b95937da520a08a74f3f7bd1bf6..dd72b7fea0a2444e5855c9c18326a14945008b96 100644 (file)
--- a/apps/lib/apps.c
+++ b/apps/lib/apps.c
@@ -79,15 +79,6 @@ static int set_table_opts(unsigned long *flags, const char *arg,
                           const NAME_EX_TBL * in_tbl);
 static int set_multi_opts(unsigned long *flags, const char *arg,
                           const NAME_EX_TBL * in_tbl);
-static
-int load_key_certs_crls_suppress(const char *uri, int format, int maybe_stdin,
-                                 const char *pass, const char *desc,
-                                 EVP_PKEY **ppkey, EVP_PKEY **ppubkey,
-                                 EVP_PKEY **pparams,
-                                 X509 **pcert, STACK_OF(X509) **pcerts,
-                                 X509_CRL **pcrl, STACK_OF(X509_CRL) **pcrls,
-                                 int suppress_decode_errors);
-
 int app_init(long mesgwin);
 
 int chopup_args(ARGS *arg, char *buf)
@@ -468,16 +459,17 @@ X509 *load_cert_pass(const char *uri, int format, int maybe_stdin,
 
     if (desc == NULL)
         desc = "certificate";
-    if (IS_HTTPS(uri))
+    if (IS_HTTPS(uri)) {
         BIO_printf(bio_err, "Loading %s over HTTPS is unsupported\n", desc);
-    else if (IS_HTTP(uri))
+    } else if (IS_HTTP(uri)) {
         cert = X509_load_http(uri, NULL, NULL, 0 /* timeout */);
-    else
+        if (cert == NULL) {
+            ERR_print_errors(bio_err);
+            BIO_printf(bio_err, "Unable to load %s from %s\n", desc, uri);
+        }
+    } else {
         (void)load_key_certs_crls(uri, format, maybe_stdin, pass, desc,
                                   NULL, NULL, NULL, &cert, NULL, NULL, NULL);
-    if (cert == NULL) {
-        BIO_printf(bio_err, "Unable to load %s\n", desc);
-        ERR_print_errors(bio_err);
     }
     return cert;
 }
@@ -489,16 +481,17 @@ X509_CRL *load_crl(const char *uri, int format, int maybe_stdin,
 
     if (desc == NULL)
         desc = "CRL";
-    if (IS_HTTPS(uri))
+    if (IS_HTTPS(uri)) {
         BIO_printf(bio_err, "Loading %s over HTTPS is unsupported\n", desc);
-    else if (IS_HTTP(uri))
+    } else if (IS_HTTP(uri)) {
         crl = X509_CRL_load_http(uri, NULL, NULL, 0 /* timeout */);
-    else
+        if (crl == NULL) {
+            ERR_print_errors(bio_err);
+            BIO_printf(bio_err, "Unable to load %s from %s\n", desc, uri);
+        }
+    } else {
         (void)load_key_certs_crls(uri, format, maybe_stdin, NULL, desc,
                                   NULL, NULL,  NULL, NULL, NULL, &crl, NULL);
-    if (crl == NULL) {
-        BIO_printf(bio_err, "Unable to load %s\n", desc);
-        ERR_print_errors(bio_err);
     }
     return crl;
 }
@@ -525,8 +518,8 @@ X509_REQ *load_csr(const char *file, int format, const char *desc)
 
  end:
     if (req == NULL) {
-        BIO_printf(bio_err, "Unable to load %s\n", desc);
         ERR_print_errors(bio_err);
+        BIO_printf(bio_err, "Unable to load %s\n", desc);
     }
     BIO_free(in);
     return req;
@@ -587,23 +580,23 @@ EVP_PKEY *load_keyparams_suppress(const char *uri, int format, int maybe_stdin,
                                  int suppress_decode_errors)
 {
     EVP_PKEY *params = NULL;
+    BIO *bio_bak = bio_err;
 
     if (desc == NULL)
         desc = "key parameters";
-
-    (void)load_key_certs_crls_suppress(uri, format, maybe_stdin, NULL, desc,
-                                       NULL, NULL, &params, NULL, NULL, NULL,
-                                       NULL, suppress_decode_errors);
+    if (suppress_decode_errors)
+        bio_err = NULL;
+    (void)load_key_certs_crls(uri, format, maybe_stdin, NULL, desc,
+                              NULL, NULL, &params, NULL, NULL, NULL, NULL);
     if (params != NULL && keytype != NULL && !EVP_PKEY_is_a(params, keytype)) {
-        if (!suppress_decode_errors) {
-            BIO_printf(bio_err,
-                       "Unable to load %s from %s (unexpected parameters type)\n",
-                       desc, uri);
-            ERR_print_errors(bio_err);
-        }
+        ERR_print_errors(bio_err);
+        BIO_printf(bio_err,
+                   "Unable to load %s from %s (unexpected parameters type)\n",
+                   desc, uri);
         EVP_PKEY_free(params);
         params = NULL;
     }
+    bio_err = bio_bak;
     return params;
 }
 
@@ -688,6 +681,8 @@ int load_cert_certs(const char *uri,
     int ret = 0;
     char *pass_string;
 
+    if (desc == NULL)
+        desc = pcerts == NULL ? "certificate" : "certificates";
     if (exclude_http && (OPENSSL_strncasecmp(uri, "http://", 7) == 0
                          || OPENSSL_strncasecmp(uri, "https://", 8) == 0)) {
         BIO_printf(bio_err, "error: HTTP retrieval not allowed for %s\n", desc);
@@ -695,8 +690,7 @@ int load_cert_certs(const char *uri,
     }
     pass_string = get_passwd(pass, desc);
     ret = load_key_certs_crls(uri, FORMAT_UNDEF, 0, pass_string, desc,
-                              NULL, NULL, NULL,
-                              pcert, pcerts, NULL, NULL);
+                              NULL, NULL, NULL, pcert, pcerts, NULL, NULL);
     clear_free(pass_string);
 
     if (ret) {
@@ -799,10 +793,12 @@ X509_STORE *load_certstore(char *input, const char *pass, const char *desc,
 int load_certs(const char *uri, int maybe_stdin, STACK_OF(X509) **certs,
                const char *pass, const char *desc)
 {
-    int was_NULL = *certs == NULL;
-    int ret = load_key_certs_crls(uri, FORMAT_UNDEF, maybe_stdin,
-                                  pass, desc, NULL, NULL,
-                                  NULL, NULL, certs, NULL, NULL);
+    int ret, was_NULL = *certs == NULL;
+
+    if (desc == NULL)
+        desc = "certificates";
+    ret = load_key_certs_crls(uri, FORMAT_UNDEF, maybe_stdin, pass, desc,
+                              NULL, NULL, NULL, NULL, certs, NULL, NULL);
 
     if (!ret && was_NULL) {
         sk_X509_pop_free(*certs, X509_free);
@@ -818,10 +814,12 @@ int load_certs(const char *uri, int maybe_stdin, STACK_OF(X509) **certs,
 int load_crls(const char *uri, STACK_OF(X509_CRL) **crls,
               const char *pass, const char *desc)
 {
-    int was_NULL = *crls == NULL;
-    int ret = load_key_certs_crls(uri, FORMAT_UNDEF, 0, pass, desc,
-                                  NULL, NULL, NULL,
-                                  NULL, NULL, NULL, crls);
+    int ret, was_NULL = *crls == NULL;
+
+    if (desc == NULL)
+        desc = "CRLs";
+    ret = load_key_certs_crls(uri, FORMAT_UNDEF, 0, pass, desc,
+                              NULL, NULL, NULL, NULL, NULL, NULL, crls);
 
     if (!ret && was_NULL) {
         sk_X509_CRL_pop_free(*crls, X509_CRL_free);
@@ -856,14 +854,12 @@ static const char *format2string(int format)
  * In any case (also on error) the caller is responsible for freeing all members
  * of *pcerts and *pcrls (as far as they are not NULL).
  */
-static
-int load_key_certs_crls_suppress(const char *uri, int format, int maybe_stdin,
-                                 const char *pass, const char *desc,
-                                 EVP_PKEY **ppkey, EVP_PKEY **ppubkey,
-                                 EVP_PKEY **pparams,
-                                 X509 **pcert, STACK_OF(X509) **pcerts,
-                                 X509_CRL **pcrl, STACK_OF(X509_CRL) **pcrls,
-                                 int suppress_decode_errors)
+int load_key_certs_crls(const char *uri, int format, int maybe_stdin,
+                        const char *pass, const char *desc,
+                        EVP_PKEY **ppkey, EVP_PKEY **ppubkey,
+                        EVP_PKEY **pparams,
+                        X509 **pcert, STACK_OF(X509) **pcerts,
+                        X509_CRL **pcrl, STACK_OF(X509_CRL) **pcrls)
 {
     PW_CB_DATA uidata;
     OSSL_STORE_CTX *ctx = NULL;
@@ -882,6 +878,7 @@ int load_key_certs_crls_suppress(const char *uri, int format, int maybe_stdin,
     OSSL_PARAM itp[2];
     const OSSL_PARAM *params = NULL;
 
+    ERR_set_mark();
     if (ppkey != NULL) {
         *ppkey = NULL;
         cnt_expectations++;
@@ -924,9 +921,9 @@ int load_key_certs_crls_suppress(const char *uri, int format, int maybe_stdin,
         SET_EXPECT(expect, OSSL_STORE_INFO_CRL);
     }
     if (cnt_expectations == 0) {
-        BIO_printf(bio_err, "Internal error: nothing to load from %s\n",
-                   uri != NULL ? uri : "<stdin>");
-        return 0;
+        BIO_printf(bio_err, "Internal error: no expectation to load");
+        failed = "anything";
+        goto end;
     }
 
     uidata.password = pass;
@@ -1062,14 +1059,14 @@ int load_key_certs_crls_suppress(const char *uri, int format, int maybe_stdin,
                 any = 1;
             failed = "CRL";
         }
-        if (!suppress_decode_errors) {
-            if (failed != NULL)
-                BIO_printf(bio_err, "Could not read");
-            if (any)
-                BIO_printf(bio_err, " any");
-        }
+        if (failed != NULL)
+            BIO_printf(bio_err, "Could not read");
+        if (any)
+            BIO_printf(bio_err, " any");
     }
-    if (!suppress_decode_errors && failed != NULL) {
+    if (failed != NULL) {
+        unsigned long err = ERR_peek_last_error();
+
         if (desc != NULL && strstr(desc, failed) != NULL) {
             BIO_printf(bio_err, " %s", desc);
         } else {
@@ -1079,27 +1076,23 @@ int load_key_certs_crls_suppress(const char *uri, int format, int maybe_stdin,
         }
         if (uri != NULL)
             BIO_printf(bio_err, " from %s", uri);
+        if (ERR_SYSTEM_ERROR(err)) {
+            /* provide more readable diagnostic output */
+            BIO_printf(bio_err, ": %s", strerror(ERR_GET_REASON(err)));
+            ERR_pop_to_mark();
+            ERR_set_mark();
+        }
         BIO_printf(bio_err, "\n");
         ERR_print_errors(bio_err);
     }
-    if (suppress_decode_errors || failed == NULL)
-        /* clear any spurious errors */
-        ERR_clear_error();
+    if (bio_err == NULL || failed == NULL)
+        /* clear any suppressed or spurious errors */
+        ERR_pop_to_mark();
+    else
+        ERR_clear_last_mark();
     return failed == NULL;
 }
 
-int load_key_certs_crls(const char *uri, int format, int maybe_stdin,
-                        const char *pass, const char *desc,
-                        EVP_PKEY **ppkey, EVP_PKEY **ppubkey,
-                        EVP_PKEY **pparams,
-                        X509 **pcert, STACK_OF(X509) **pcerts,
-                        X509_CRL **pcrl, STACK_OF(X509_CRL) **pcrls)
-{
-    return load_key_certs_crls_suppress(uri, format, maybe_stdin, pass, desc,
-                                        ppkey, ppubkey, pparams, pcert, pcerts,
-                                        pcrl, pcrls, 0);
-}
-
 #define X509V3_EXT_UNKNOWN_MASK         (0xfL << 16)
 /* Return error for unknown extensions */
 #define X509V3_EXT_DEFAULT              0