harden output file handling against race conditions and symlink attacks

programs/fileio.c

@@ -850,6 +850,7 @@ FIO_openDstFile(FIO_ctx_t* fCtx, FIO_prefs_t* const prefs,
                 const char* srcFileName, const char* dstFileName,
                 const int mode)
 {
+    int overwriteDstFile = prefs->overwrite;
     if (prefs->testMode) return NULL;  /* do not open file in test mode */
 
     assert(dstFileName != NULL);
@@ -879,7 +880,7 @@ FIO_openDstFile(FIO_ctx_t* fCtx, FIO_prefs_t* const prefs,
                         dstFileName);
         }
 #endif
-        if (!prefs->overwrite) {
+        if (!overwriteDstFile) {
             if (g_display_prefs.displayLevel <= 1) {
                 /* No interaction possible */
                 DISPLAYLEVEL(1, "zstd: %s already exists; not overwritten  \n",
@@ -889,24 +890,31 @@ FIO_openDstFile(FIO_ctx_t* fCtx, FIO_prefs_t* const prefs,
             DISPLAY("zstd: %s already exists; ", dstFileName);
             if (UTIL_requireUserConfirmation("overwrite (y/n) ? ", "Not overwritten  \n", "yY", fCtx->hasStdinInput))
                 return NULL;
+            overwriteDstFile = 1;
         }
-        /* need to unlink */
-        FIO_removeFile(dstFileName);
     }
 
     {
         int isDstRegFile;
 #if defined(_WIN32)
         /* Windows requires opening the file as a "binary" file to avoid
          * mangling. This macro doesn't exist on unix. */
-        const int openflags = O_WRONLY|O_CREAT|O_TRUNC|O_BINARY;
+        int openflags = O_WRONLY|O_CREAT|O_BINARY;
+        openflags |= overwriteDstFile ? O_TRUNC : O_EXCL;
         const int fd = _open(dstFileName, openflags, mode);
         FILE* f = NULL;
         if (fd != -1) {
             f = _fdopen(fd, "wb");
         }
 #else
-        const int openflags = O_WRONLY|O_CREAT|O_TRUNC;
+        int openflags = O_WRONLY|O_CREAT;
+        openflags |= overwriteDstFile ? O_TRUNC : O_EXCL;
+    #  ifdef O_CLOEXEC
+        openflags |= O_CLOEXEC;
+    #  endif
+    #  ifdef O_NOFOLLOW
+        openflags |= O_NOFOLLOW;
+    #  endif
         const int fd = open(dstFileName, openflags, mode);
         FILE* f = NULL;
         if (fd != -1) {
@@ -925,8 +933,14 @@ FIO_openDstFile(FIO_ctx_t* fCtx, FIO_prefs_t* const prefs,
         }
 
         if (f == NULL) {
-            if (UTIL_isFileDescriptorPipe(dstFileName)) {
+            if (!overwriteDstFile && errno == EEXIST) {
+                DISPLAYLEVEL(1, "zstd: %s already exists; not overwritten  \n", dstFileName);
+            } else if (UTIL_isFileDescriptorPipe(dstFileName)) {
                 DISPLAYLEVEL(1, "zstd: error: no output specified (use -o or -c). \n");
+#if defined(O_NOFOLLOW) && defined(ELOOP)
+            } else if (errno == ELOOP) {
+                DISPLAYLEVEL(1, "zstd: refusing to write through symbolic link: %s\n", dstFileName);
+#endif
             } else {
                 DISPLAYLEVEL(1, "zstd: %s: %s\n", dstFileName, strerror(errno));
             }

tests/cli-tests/file-handling/output-symlink-hardening.sh

@@ -0,0 +1,27 @@
+#!/bin/sh
+set -e
+
+. "$COMMON/platform.sh"
+
+echo "sensitive-data" > sensitive
+echo "payload-data" > payload
+
+# Skip if symlink creation is unavailable in this environment.
+if ! ln -s sensitive out.zst 2>"$INTOVOID"; then
+    exit 0
+fi
+if zstd -q -f payload -o out.zst 2>"$INTOVOID"; then
+    die "compression to symlink output must fail"
+fi
+
+test "$(cat sensitive)" = "sensitive-data" || die "symlink target was modified"
+test -L out.zst || die "output path should remain a symlink"
+
+zstd -q -f payload -o payload.zst
+ln -s sensitive decoded
+if zstd -q -d -f payload.zst -o decoded 2>"$INTOVOID"; then
+    die "decompression to symlink output must fail"
+fi
+
+test "$(cat sensitive)" = "sensitive-data" || die "symlink target was modified"
+test -L decoded || die "decoded output path should remain a symlink"