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 d01bf390cfa912aebe05fb063ab8cfa9e7cab1ef..2f9f94ba87726f32e3dbaf51178d8edefa2b95ec 100644 (file)
--- 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 ce23cedbe318630b3c508d35a300085d1f1cbcc0..90c92a9a094ce2bdee99dced0edf8031b086eced 100644 (file)
--- 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 414d1fed031e0e7e3cd1a68f7eeaf13ad45f0300..fa72774279199c51029bad8367891966ef2713eb 100644 (file)
--- 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