From: beck <beck@openbsd.org>
Date: Mon, 25 Mar 2024 00:05:49 +0000 (+0000)
Subject: Remove unnecessary stat() calls from by_dir
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=614b3eb4c8277a065db185d6e3da2175e9b27c57;p=openbsd

Remove unnecessary stat() calls from by_dir

When searching for a CA or CRL file in by_dir, this stat()
was used to short circuit attempting to open the file with
X509_load_cert_file(). This was a deliberate TOCTOU introduced
to avoid setting an error on the error stack, when what you
really want to say is "we couldn't find a CA" and continue
merrily on your way.

As it so happens you really do not care why the load_file failed
in any of these cases, it all boils down to "I can't find the CA
or CRL". Instead we just omit the stat call, and clear the error
stack if the load_file fails. The fact that you don't have a CA or
CRL is caught later in the callers and is what you want, mimicing
the non by_dir behaviour instead of possibly some bizzaro file
system error.

Based on a similar change in Boring.

ok tb@
---

diff --git a/lib/libcrypto/x509/by_dir.c b/lib/libcrypto/x509/by_dir.c
index 7e6949e21c4..bb14e72806d 100644
--- a/lib/libcrypto/x509/by_dir.c
+++ b/lib/libcrypto/x509/by_dir.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: by_dir.c,v 1.46 2023/12/29 05:33:32 tb Exp $ */
+/* $OpenBSD: by_dir.c,v 1.47 2024/03/25 00:05:49 beck Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -56,9 +56,6 @@
  * [including the GNU Public Licence.]
  */
 
-#include <sys/stat.h>
-#include <sys/types.h>
-
 #include <errno.h>
 #include <stdio.h>
 #include <string.h>
@@ -331,23 +328,27 @@ get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name,
 		for (;;) {
 			(void) snprintf(b->data, b->max, "%s/%08lx.%s%d",
 			    ent->dir, h, postfix, k);
-
-			{
-				struct stat st;
-				if (stat(b->data, &st) < 0)
-					break;
-			}
-			/* found one. */
+			/*
+			 * Found one. Attempt to load it. This could fail for
+			 * any number of reasons from the file can't be opened,
+			 * the file contains garbage, etc. Clear the error stack
+			 * to avoid exposing the lower level error. These all
+			 * boil down to "we could not find CA/CRL".
+			 */
 			if (type == X509_LU_X509) {
 				if ((X509_load_cert_file(xl, b->data,
-				    ent->dir_type)) == 0)
+				    ent->dir_type)) == 0) {
+					ERR_clear_error();
 					break;
+				}
 			} else if (type == X509_LU_CRL) {
 				if ((X509_load_crl_file(xl, b->data,
-				    ent->dir_type)) == 0)
+				    ent->dir_type)) == 0) {
+					ERR_clear_error();
 					break;
+				}
 			}
-			/* else case will caught higher up */
+			/* The lack of a CA or CRL will be caught higher up. */
 			k++;
 		}