fix race condition allowing attackers to access destination file

Origin: upstream
Bug: https://github.com/facebook/zstd/issues/2491
Bug-Debian: https://github.com/facebook/zstd/issues/2491
Applied-Upstream: commit:a774c5797399040af62db21d8a9b9769e005430e
Reviewed-by: Étienne Mollier <etienne.mollier@mailoo.org>
Last-Update: 2021-02-18

This commit addresses https://github.com/facebook/zstd/issues/2491.

Note that a downside of this solution is that it is global: `umask()` affects
all file creation calls in the process. I believe this is safe since
`fileio.c` functions should only ever be used in the zstd binary, and these
are (almost) the only files ever created by zstd, and AIUI they're only
created in a single thread. So we can get away with messing with global state.

Note that this doesn't change the permissions of files created by `dibio.c`.
I'm not sure what those should be...
Last-Update: 2021-02-18
Gbp-Pq: Name 0018-fix-file-permissions-on-compression.patch

diff --git a/programs/fileio.c b/programs/fileio.c
index 65f2d531a81d923d822c73a59d742a346c787389..e740876f7f7648fdd9c0cc3cff31e387b786722a 100644 (file)
--- a/programs/fileio.c
+++ b/programs/fileio.c
@@ -675,14 +675,11 @@ FIO_openDstFile(FIO_ctx_t* fCtx, FIO_prefs_t* const prefs,
             FIO_removeFile(dstFileName);
     }   }
 
-    {   FILE* const f = fopen( dstFileName, "wb" );
+    {   const int old_umask = UTIL_umask(0177); /* u-x,go-rwx */
+        FILE* const f = fopen( dstFileName, "wb" );
+        UTIL_umask(old_umask);
         if (f == NULL) {
             DISPLAYLEVEL(1, "zstd: %s: %s\n", dstFileName, strerror(errno));
-        } else if (srcFileName != NULL
-               && strcmp (srcFileName, stdinmark)
-               && strcmp(dstFileName, nulmark) ) {
-            /* reduce rights on newly created dst file while compression is ongoing */
-            UTIL_chmod(dstFileName, NULL, 00600);
         }
         return f;
     }

diff --git a/programs/util.c b/programs/util.c
index 8bcfe47e54b623c98b9979c42ec1949aeb51d97b..6f7810c1ddaf0cb9f9f04e16404f3ee9d23ef99d 100644 (file)
--- a/programs/util.c
+++ b/programs/util.c
@@ -159,6 +159,15 @@ int UTIL_chmod(char const* filename, const stat_t* statbuf, mode_t permissions)
     return chmod(filename, permissions);
 }
 
+int UTIL_umask(int mode) {
+#if PLATFORM_POSIX_VERSION > 0
+    return umask(mode);
+#else
+    /* do nothing, fake return value */
+    return mode;
+#endif
+}
+
 int UTIL_setFileStat(const char *filename, const stat_t *statbuf)
 {
     int res = 0;

diff --git a/programs/util.h b/programs/util.h
index 25fa3f53aab424d4d90b647b52237c3fc94eb7d4..2c31a2d990bd8cb0606b9f69df53c9d4d3c6e219 100644 (file)
--- a/programs/util.h
+++ b/programs/util.h
@@ -22,7 +22,7 @@ extern "C" {
 #include "platform.h"     /* PLATFORM_POSIX_VERSION, ZSTD_NANOSLEEP_SUPPORT, ZSTD_SETPRIORITY_SUPPORT */
 #include <stddef.h>       /* size_t, ptrdiff_t */
 #include <sys/types.h>    /* stat, utime */
-#include <sys/stat.h>     /* stat, chmod */
+#include <sys/stat.h>     /* stat, chmod, umask */
 #include "../lib/common/mem.h"          /* U64 */
 
 
@@ -152,6 +152,11 @@ U64 UTIL_getFileSizeStat(const stat_t* statbuf);
  */
 int UTIL_chmod(char const* filename, const stat_t* statbuf, mode_t permissions);
 
+/**
+ * Wraps umask(). Does nothing when the platform doesn't have that concept.
+ */
+int UTIL_umask(int mode);
+
 /*
  * In the absence of a pre-existing stat result on the file in question, these
  * functions will do a stat() call internally and then use that result to