From: Arran Cudbard-Bell <a.cudbardb@freeradius.org>
Date: Tue, 28 Jun 2022 17:44:23 +0000 (-0500)
Subject: Emit errors if stat fails, and use fstat not stat to avoid TOCTOU issues
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e6f45473cbcab6d0d2ef8810e01ea19db2812a16;p=thirdparty%2Ffreeradius-server.git

Emit errors if stat fails, and use fstat not stat to avoid TOCTOU issues
---

diff --git a/src/lib/util/dict_tokenize.c b/src/lib/util/dict_tokenize.c
index 8d53fb45c88..f1aa70becc9 100644
--- a/src/lib/util/dict_tokenize.c
+++ b/src/lib/util/dict_tokenize.c
@@ -27,6 +27,7 @@ RCSID("$Id$")
 #include <freeradius-devel/util/dict_fixup_priv.h>
 #include <freeradius-devel/util/file.h>
 #include <freeradius-devel/util/rand.h>
+#include <freeradius-devel/util/strerror.h>
 #include <freeradius-devel/util/syserror.h>
 #include <freeradius-devel/util/value.h>
 
@@ -1900,15 +1901,17 @@ static int _dict_from_file(dict_tokenize_ctx_t *ctx,
 	/*
 	 *	If fopen works, this works.
 	 */
-	if (stat(fn, &statbuf) < 0) {
+	if (fstat(fileno(fp), &statbuf) < 0) {
+		fr_strerror_printf_push("Failed stating dictionary \"%s\" - %s", fn, fr_syserror(errno));
+
+	perm_error:
 		fclose(fp);
 		return -1;
 	}
 
 	if (!S_ISREG(statbuf.st_mode)) {
-		fclose(fp);
 		fr_strerror_printf_push("Dictionary is not a regular file: %s", fn);
-		return -1;
+		goto perm_error;
 	}
 
 	/*
@@ -1920,7 +1923,7 @@ static int _dict_from_file(dict_tokenize_ctx_t *ctx,
 		fclose(fp);
 		fr_strerror_printf_push("Dictionary is globally writable: %s. "
 					"Refusing to start due to insecure configuration", fn);
-		return -1;
+		goto perm_error;
 	}
 #endif
 
@@ -2048,8 +2051,8 @@ static int _dict_from_file(dict_tokenize_ctx_t *ctx,
 		 */
 		if (strcasecmp(argv[0], "ENUM") == 0) {
 			if (dict_read_process_enum(ctx,
-							argv + 1, argc - 1,
-							&base_flags) == -1) goto error;
+						   argv + 1, argc - 1,
+						   &base_flags) == -1) goto error;
 			continue;
 		}
 
@@ -2111,15 +2114,13 @@ static int _dict_from_file(dict_tokenize_ctx_t *ctx,
 
 			if (ret < 0) {
 				fr_strerror_printf_push("from $INCLUDE at %s[%d]", fr_cwd_strip(fn), line);
-				fclose(fp);
-				return -1;
+				goto error;
 			}
 
 			if (ctx->stack_depth < stack_depth) {
 				fr_strerror_printf_push("unexpected END-??? in $INCLUDE at %s[%d]",
 							fr_cwd_strip(fn), line);
-				fclose(fp);
-				return -1;
+				goto error;
 			}
 
 			while (ctx->stack_depth > stack_depth) {
@@ -2130,8 +2131,7 @@ static int _dict_from_file(dict_tokenize_ctx_t *ctx,
 
 				fr_strerror_printf_push("BEGIN-??? without END-... in file $INCLUDEd from %s[%d]",
 							fr_cwd_strip(fn), line);
-				fclose(fp);
-				return -1;
+				goto error;
 			}
 
 			/*
@@ -2270,10 +2270,7 @@ static int _dict_from_file(dict_tokenize_ctx_t *ctx,
 			 *	error. So we don't need to do that
 			 *	here.
 			 */
-			if (dict_finalise(ctx) < 0) {
-				fclose(fp);
-				return -1;
-			}
+			if (dict_finalise(ctx) < 0) goto error;
 
 			ctx->stack_depth--;
 			ctx->dict = ctx->stack[ctx->stack_depth].dict;
@@ -2326,8 +2323,6 @@ static int _dict_from_file(dict_tokenize_ctx_t *ctx,
 				goto error;
 			}
 
-
-
 			da = fr_dict_attr_by_oid(NULL, ctx->stack[ctx->stack_depth - 1].da, argv[1]);
 			if (!da) {
 				fr_strerror_const_push("Failed resolving attribute in END-TLV entry");
@@ -2533,8 +2528,8 @@ static int _dict_from_file(dict_tokenize_ctx_t *ctx,
 	 *	was copied from the parent, so there are guaranteed to
 	 *	be missing things.
 	 */
-
 	fclose(fp);
+
 	return 0;
 }