From: Ondřej Surý <ondrej@isc.org>
Date: Wed, 29 Apr 2026 13:30:05 +0000 (+0200)
Subject: Stop isc_file_safecreate from following symlinks
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6082274450ff34d0ce7c16928d059da6f5093d6b;p=thirdparty%2Fbind9.git

Stop isc_file_safecreate from following symlinks

The function existence-checked the target with stat() and then opened
the same path without O_NOFOLLOW, so a symlink at the target path
passed the regular-file test against the link's destination and the
open() that followed truncated and wrote through the link.
rndc-confgen -a is typically run as root and writes the keyfile under
a directory that service accounts may have write access to, so a stray
symlink there would silently redirect the truncate, fchown, and
overwrite to whatever file the link pointed at.

Switch the existence check to lstat() and use S_ISREG() so a symlink's
S_IFLNK mode is detected directly (a plain bitmask of S_IFREG matches
both, since S_IFLNK shares its high bit). Add O_NOFOLLOW to both
open() flag sets to close the lstat/open TOCTOU window. Hardening
against unexpected symlinks on intermediate path components is out of
scope.

Assisted-by: Claude:claude-opus-4-7
---

diff --git a/lib/isc/file.c b/lib/isc/file.c
index d01bf390cfa..2f9f94ba877 100644
--- a/lib/isc/file.c
+++ b/lib/isc/file.c
@@ -91,6 +91,20 @@ file_stats(const char *file, struct stat *stats) {
 	return result;
 }
 
+static isc_result_t
+file_lstats(const char *file, struct stat *stats) {
+	isc_result_t result = ISC_R_SUCCESS;
+
+	REQUIRE(file != NULL);
+	REQUIRE(stats != NULL);
+
+	if (lstat(file, stats) != 0) {
+		result = isc__errno2result(errno);
+	}
+
+	return result;
+}
+
 static isc_result_t
 fd_stats(int fd, struct stat *stats) {
 	isc_result_t result = ISC_R_SUCCESS;
@@ -607,14 +621,14 @@ isc_file_safecreate(const char *filename, FILE **fp) {
 	REQUIRE(filename != NULL);
 	REQUIRE(fp != NULL && *fp == NULL);
 
-	result = file_stats(filename, &sb);
+	result = file_lstats(filename, &sb);
 	if (result == ISC_R_SUCCESS) {
-		if ((sb.st_mode & S_IFREG) == 0) {
+		if (!S_ISREG(sb.st_mode)) {
 			return ISC_R_INVALIDFILE;
 		}
-		flags = O_WRONLY | O_TRUNC;
+		flags = O_WRONLY | O_TRUNC | O_NOFOLLOW;
 	} else if (result == ISC_R_FILENOTFOUND) {
-		flags = O_WRONLY | O_CREAT | O_EXCL;
+		flags = O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW;
 	} else {
 		return result;
 	}
diff --git a/lib/isc/include/isc/file.h b/lib/isc/include/isc/file.h
index ce23cedbe31..90c92a9a094 100644
--- a/lib/isc/include/isc/file.h
+++ b/lib/isc/include/isc/file.h
@@ -288,8 +288,9 @@ isc_file_truncate(const char *filename, off_t size);
 isc_result_t
 isc_file_safecreate(const char *filename, FILE **fp);
 /*%<
- * Open 'filename' for writing, truncating if necessary.  Ensure that
- * if it existed it was a normal file.  If creating the file, ensure
+ * Open 'filename' for writing, truncating if necessary.  If 'filename'
+ * exists it must be a regular file; symbolic links (and any other
+ * non-regular file types) are rejected.  If creating the file, ensure
  * that only the owner can read/write it.
  */
 
diff --git a/tests/isc/file_test.c b/tests/isc/file_test.c
index 414d1fed031..fa727742791 100644
--- a/tests/isc/file_test.c
+++ b/tests/isc/file_test.c
@@ -89,6 +89,34 @@ ISC_RUN_TEST_IMPL(isc_file_sanitize) {
 	unlink(F(SHA));
 }
 
+/* test that isc_file_safecreate refuses to follow symlinks */
+ISC_RUN_TEST_IMPL(isc_file_safecreate_symlink) {
+	char target[] = "safecreate_target.XXXXXX";
+	char link[] = "safecreate_link.XXXXXX";
+	int fd;
+	FILE *fp = NULL;
+	isc_result_t result;
+
+	UNUSED(state);
+
+	fd = mkstemp(target);
+	assert_int_not_equal(fd, -1);
+	close(fd);
+
+	fd = mkstemp(link);
+	assert_int_not_equal(fd, -1);
+	close(fd);
+	unlink(link);
+	assert_return_code(symlink(target, link), 0);
+
+	result = isc_file_safecreate(link, &fp);
+	assert_int_equal(result, ISC_R_INVALIDFILE);
+	assert_null(fp);
+
+	unlink(link);
+	unlink(target);
+}
+
 /* test filename templates */
 ISC_RUN_TEST_IMPL(isc_file_template) {
 	isc_result_t result;
@@ -135,6 +163,7 @@ ISC_RUN_TEST_IMPL(isc_file_template) {
 ISC_TEST_LIST_START
 
 ISC_TEST_ENTRY(isc_file_sanitize)
+ISC_TEST_ENTRY(isc_file_safecreate_symlink)
 ISC_TEST_ENTRY(isc_file_template)
 
 ISC_TEST_LIST_END