* [PATCH v2 0/2] Generate temporary files using a CSPRNG @ 2022-01-04 1:55 brian m. carlson 2022-01-04 1:55 ` [PATCH v2 1/2] wrapper: add a helper to generate numbers from " brian m. carlson ` (3 more replies) 0 siblings, 4 replies; 23+ messages in thread From: brian m. carlson @ 2022-01-04 1:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón Currently, when we generate a temporary file name, we use the seconds, microseconds, and the PID to generate a unique value. The resulting value, while changing frequently, is actually predictable and on some systems, it may be possible to cause a DoS by creating all potential temporary files when the temporary file is being created in TMPDIR. The solution to this is to use the system CSPRNG to generate the temporary file name. This is the approach taken by FreeBSD, NetBSD, and OpenBSD, and glibc also recently switched to this approach from an approach that resembled ours in many ways. Even if this is not practically exploitable on many systems, it seems prudent to be at least as careful about temporary file generation as libc is. In this round, I've switched to using a single Makefile variable instead of multiple. This lets folks set the compile options in a simpler way, but it does require that we strip whitespace when parsing the values, since we have several places in our makefiles where we add extra whitespace. We still have multiple #defines because that's functionally required. I've also added support for the OpenSSL CSPRNG here, which should address concerns for NonStop. Because OpenSSL is very portable across systems and a TLS library is effectively required for a generally useful Git on the Internet today, we can assume that this is a safe fallback. I have noted in my updated commit message that POSIX is set to standardize getentropy in a future revision of the standard. Of course, we don't demand that systems implement a specific version of POSIX, but that does mean that this code will likely shrink somewhat over time instead of increasing in size. That's my hope, at least. I have, in light of the above additions, chosen not to add a fallback to an insecure option. I think we provide plenty of options that should meet the needs of all users and in the unlikely event we find a system which simply cannot provide the relevant interface, we can add one then. There was a proposal to switch to using mkstemp instead. I have not adopted that approach because it is strictly less functional than our current approach (e.g., it would not support modes properly), I have doubts about its portability (in which case we'd have to reimplement it using something like this series anyway), and as mentioned in the thread, I plan to investigate further uses of this code in the future. For those that are interested in the nitty-gritty details after reading the commit message, I did end up determining that modern macOS supports the arc4random interfaces we require. It currently calls out to its own CTR DRBG in CoreCrypto, which is why it doesn't use ChaCha. MirBSD also supports these interfaces, but still uses RC4. We use those interfaces anyway and will let them deal with the consequences, since the interface is stated to use a CSPRNG and as far as I can tell the kernel uses the same algorithm. All the other BSDs I checked (and libbsd) use ChaCha and have the boring and secure OpenBSD-like behavior. A range-diff appears below for your convenience. Changes from v1: * Remove the automatic testing using buckets because it didn't seem to add much. * Switch to a single makefile variable. * Add support for OpenSSL CSPRNG. * Add more defaults for various systems, including macOS and NonStop. * Add an arc4random-libbsd variant for improved testing of the arc4random code paths on Linux. The only difference is the inclusion of an additional header. * Print a more useful error message than before, indicating that the CSPRNG failed. brian m. carlson (2): wrapper: add a helper to generate numbers from a CSPRNG wrapper: use a CSPRNG to generate random file names Makefile | 33 ++++++++++++ compat/winansi.c | 6 +++ config.mak.uname | 8 +++ contrib/buildsystems/CMakeLists.txt | 2 +- git-compat-util.h | 19 +++++++ t/helper/test-csprng.c | 29 +++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + wrapper.c | 81 +++++++++++++++++++++++++---- 9 files changed, 168 insertions(+), 12 deletions(-) create mode 100644 t/helper/test-csprng.c Diff-intervalle contre v1 : 1: 8ffb65a1f8 ! 1: edd623bd9a wrapper: add a helper to generate numbers from a CSPRNG @@ Commit message To make this possible, let's add a function which reads from a system CSPRNG and returns some bytes. + We know that all systems will have such an interface. A CSPRNG is + required for a secure TLS or SSH implementation and a Git implementation + which provided neither would be of little practical use. In addition, + POSIX is set to standardize getentropy(2) in the next version, so in the + (potentially distant) future we can rely on that. + + For systems which lack one of the other interfaces, we provide the + ability to use OpenSSL's CSPRNG. OpenSSL is highly portable and + functions on practically every known OS, and we know it will have access + to some source of cryptographically secure randomness. We also provide + support for the arc4random in libbsd for folks who would prefer to use + that. + Because this is a security sensitive interface, we take some precautions. We either succeed by filling the buffer completely as we requested, or we fail. We don't return partial data because the caller will almost never find that to be a useful behavior. - The order of options is also important here. On systems with - arc4random, which is most of the BSDs, we use that, since, except on - MirBSD, it uses ChaCha20, which is extremely fast, and sits entirely in - userspace, avoiding a system call. We then prefer getrandom over - getentropy, because the former has been available longer on Linux, and - finally, if none of those are available, we use /dev/urandom, because - most Unix-like operating systems provide that API. We prefer options - that don't involve device files when possible because those work in some - restricted environments where device files may not be available. + Specify a makefile knob which users can use to specify their preferred + CSPRNG, and turn the multiple string options into a set of defines, + since we cannot match on strings in the preprocessor. - macOS appears to have arc4random but not the arc4random_buf function we - want to use, so we let it use the fallback of /dev/urandom. Set the - configuration variables appropriately for Linux and the other BSDs. We - specifically only consider versions which receive publicly available - security support; for example, getrandom(2) and getentropy(3) are only - available in FreeBSD 12, which is the oldest version with current - security support. For the same reason, we don't specify getrandom(2) on - Linux, because CentOS 7 doesn't support it in glibc (although its kernel - does) and we don't want to resort to making syscalls. + The order of suggested options is important here. On systems with + arc4random, which is most of the BSDs, we suggest that, since, except on + MirBSD and macOS, it uses ChaCha20, which is extremely fast, and sits + entirely in userspace, avoiding a system call. We then prefer getrandom + over getentropy, because the former has been available longer on Linux, + and then OpenSSL. Finally, if none of those are available, we use + /dev/urandom, because most Unix-like operating systems provide that API. + We prefer to suggest options that don't involve device files when + possible because those work in some restricted environments where device + files may not be available. - Finally, add a self-test option here to make sure that our buffer - handling is correct and we aren't truncating data. We simply read 64 - KiB and then make sure we've seen each byte. The probability of this - test failing spuriously is less than 10^-100. + Set the configuration variables appropriately for Linux and the BSDs, + including macOS, as well as Windows and NonStop. We specifically only + consider versions which receive publicly available security support + here. For the same reason, we don't specify getrandom(2) on Linux, + because CentOS 7 doesn't support it in glibc (although its kernel does) + and we don't want to resort to making syscalls. + + Finally, add a test helper to allow this to be tested by hand and in + tests. We don't add any tests, since invoking the CSPRNG is not likely + to produce interesting, reproducible results. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> @@ Makefile: all:: # Define NO_TRUSTABLE_FILEMODE if your filesystem may claim to support # the executable mode bit, but doesn't really do so. # -+# Define HAVE_ARC4RANDOM if your system has arc4random and arc4random_buf. -+# -+# Define HAVE_GETRANDOM if your system has getrandom. -+# -+# Define HAVE_GETENTROPY if your system has getentropy. -+# -+# Define HAVE_RTLGENRANDOM if your system has RtlGenRandom (Windows only). ++# Define CSPRNG_METHOD to "arc4random" if your system has arc4random and ++# arc4random_buf, "arc4random-libbsd" if your system has those functions from ++# libbsd, "getrandom" if your system has getrandom, "getentropy" if your ++# system has getentropy, "rtlgenrandom" for RtlGenRandom (Windows only), or ++# "openssl" if you'd want to use the OpenSSL CSPRNG. If unset or set to ++# anything else, defaults to using "/dev/urandom". +# # Define NEEDS_MODE_TRANSLATION if your OS strays from the typical file type # bits in mode values (e.g. z/OS defines I_SFMT to 0xFF000000 as opposed to the @@ Makefile: ifdef HAVE_GETDELIM BASIC_CFLAGS += -DHAVE_GETDELIM endif -+ifdef HAVE_ARC4RANDOM ++ifeq ($(strip $(CSPRNG_METHOD)),arc4random) + BASIC_CFLAGS += -DHAVE_ARC4RANDOM +endif + -+ifdef HAVE_GETRANDOM ++ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd) ++ BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD ++ EXTLIBS += -lbsd ++endif ++ ++ifeq ($(strip $(CSPRNG_METHOD)),getrandom) + BASIC_CFLAGS += -DHAVE_GETRANDOM +endif + -+ifdef HAVE_GETENTROPY ++ifeq ($(strip $(CSPRNG_METHOD)),getentropy) + BASIC_CFLAGS += -DHAVE_GETENTROPY +endif + -+ifdef HAVE_RTLGENRANDOM ++ifeq ($(strip $(CSPRNG_METHOD)),rtlgenrandom) + BASIC_CFLAGS += -DHAVE_RTLGENRANDOM +endif ++ ++ifeq ($(strip $(CSPRNG_METHOD)),openssl) ++ BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG ++endif + ifneq ($(PROCFS_EXECUTABLE_PATH),) procfs_executable_path_SQ = $(subst ','\'',$(PROCFS_EXECUTABLE_PATH)) @@ compat/winansi.c #include <winreg.h> ## config.mak.uname ## +@@ config.mak.uname: ifeq ($(uname_S),Darwin) + HAVE_BSD_SYSCTL = YesPlease + FREAD_READS_DIRECTORIES = UnfortunatelyYes + HAVE_NS_GET_EXECUTABLE_PATH = YesPlease ++ CSPRNG_METHOD = arc4random + + # Workaround for `gettext` being keg-only and not even being linked via + # `brew link --force gettext`, should be obsolete as of @@ config.mak.uname: ifeq ($(uname_S),FreeBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease -+ HAVE_ARC4RANDOM = YesPlease -+ HAVE_GETRANDOM = YesPlease -+ HAVE_GETENTROPY = YesPlease ++ CSPRNG_METHOD = arc4random PAGER_ENV = LESS=FRX LV=-c MORE=FRX FREAD_READS_DIRECTORIES = UnfortunatelyYes FILENO_IS_A_MACRO = UnfortunatelyYes @@ config.mak.uname: ifeq ($(uname_S),OpenBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease -+ HAVE_ARC4RANDOM = YesPlease -+ HAVE_GETENTROPY = YesPlease ++ CSPRNG_METHOD = arc4random PROCFS_EXECUTABLE_PATH = /proc/curproc/file FREAD_READS_DIRECTORIES = UnfortunatelyYes FILENO_IS_A_MACRO = UnfortunatelyYes @@ config.mak.uname: ifeq ($(uname_S),MirBSD) NEEDS_LIBICONV = YesPlease HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease -+ HAVE_ARC4RANDOM = YesPlease ++ CSPRNG_METHOD = arc4random endif ifeq ($(uname_S),NetBSD) ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2) @@ config.mak.uname: ifeq ($(uname_S),NetBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease -+ HAVE_ARC4RANDOM = YesPlease ++ CSPRNG_METHOD = arc4random PROCFS_EXECUTABLE_PATH = /proc/curproc/exe endif ifeq ($(uname_S),AIX) @@ config.mak.uname: ifeq ($(uname_S),Windows) NO_STRTOUMAX = YesPlease NO_MKDTEMP = YesPlease NO_INTTYPES_H = YesPlease -+ HAVE_RTLGENRANDOM = YesPlease ++ CSPRNG_METHOD = rtlgenrandom # VS2015 with UCRT claims that snprintf and friends are C99 compliant, # so we don't need this: # +@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL) + NO_MMAP = YesPlease + NO_POLL = YesPlease + NO_INTPTR_T = UnfortunatelyYes ++ CSPRNG_METHOD = openssl + SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin + SHELL_PATH = /usr/coreutils/bin/bash + endif @@ config.mak.uname: ifeq ($(uname_S),MINGW) NO_POSIX_GOODIES = UnfortunatelyYes DEFAULT_HELP_FORMAT = html HAVE_PLATFORM_PROCINFO = YesPlease -+ HAVE_RTLGENRANDOM = YesPlease ++ CSPRNG_METHOD = rtlgenrandom BASIC_LDFLAGS += -municode COMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32 COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" @@ git-compat-util.h #else #include <stdint.h> #endif ++#ifdef HAVE_ARC4RANDOM_LIBBSD ++#include <bsd/stdlib.h> ++#endif +#ifdef HAVE_GETRANDOM +#include <sys/random.h> +#endif @@ t/helper/test-csprng.c (new) +#include "test-tool.h" +#include "git-compat-util.h" + -+/* -+ * Check that we read each byte value at least once when reading 64 KiB from the -+ * CSPRNG. This is not to test the quality of the CSPRNG, but to test our -+ * buffer handling of it. -+ * -+ * The probability of this failing by random is less than 10^-100. -+ */ -+static int selftest(void) -+{ -+ int buckets[256] = { 0 }; -+ unsigned char buf[1024]; -+ unsigned long count = 64 * 1024; -+ int i; -+ -+ while (count) { -+ if (csprng_bytes(buf, sizeof(buf)) < 0) { -+ perror("failed to read"); -+ return 3; -+ } -+ for (i = 0; i < sizeof(buf); i++) -+ buckets[buf[i]]++; -+ count -= sizeof(buf); -+ } -+ for (i = 0; i < ARRAY_SIZE(buckets); i++) -+ if (!buckets[i]) { -+ fprintf(stderr, "failed to find any bytes with value %02x\n", i); -+ return 4; -+ } -+ return 0; -+} + +int cmd__csprng(int argc, const char **argv) +{ @@ t/helper/test-csprng.c (new) + unsigned char buf[1024]; + + if (argc > 2) { -+ fprintf(stderr, "usage: %s [--selftest | <size>]\n", argv[0]); ++ fprintf(stderr, "usage: %s [<size>]\n", argv[0]); + return 2; + } + -+ if (!strcmp(argv[1], "--selftest")) { -+ return selftest(); -+ } -+ + count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L; + + while (count) { @@ t/helper/test-tool.h: int cmd__bloom(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); - ## t/t0000-basic.sh ## -@@ t/t0000-basic.sh: test_expect_success 'test_must_fail rejects a non-git command with env' ' - grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err - ' - -+test_expect_success 'CSPRNG handling functions correctly' ' -+ test-tool csprng --selftest -+' -+ - test_done - ## wrapper.c ## @@ wrapper.c: int open_nofollow(const char *path, int flags) return open(path, flags); @@ wrapper.c: int open_nofollow(const char *path, int flags) + +int csprng_bytes(void *buf, size_t len) +{ -+#if defined(HAVE_ARC4RANDOM) ++#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD) ++ /* This function never returns an error. */ + arc4random_buf(buf, len); + return 0; +#elif defined(HAVE_GETRANDOM) @@ wrapper.c: int open_nofollow(const char *path, int flags) + if (!RtlGenRandom(buf, len)) + return -1; + return 0; ++#elif defined(HAVE_OPENSSL_CSPRNG) ++ int res = RAND_bytes(buf, len); ++ if (res == 1) ++ return 0; ++ if (res == -1) ++ errno = ENOTSUP; ++ else ++ errno = EIO; ++ return -1; +#else + ssize_t res; + char *p = buf; 2: f7212e0dce ! 2: b4cd8700e3 wrapper: use a CSPRNG to generate random file names @@ Commit message Unfortunately, this is not the best idea from a security perspective. If we're writing into TMPDIR, an attacker can guess these values easily and prevent us from creating any temporary files at all by creating them - all first. POSIX only requires TMP_MAX to be 25, so this is achievable + all first. Even though we set TMP_MAX to 16384, this may be achievable in some contexts, even if unlikely to occur in practice. Fortunately, we can simply solve this by using the system @@ wrapper.c: int git_mkstemps_mode(char *pattern, int suffix_len, int mode) int i; + uint64_t v; + if (csprng_bytes(&v, sizeof(v)) < 0) -+ return -1; ++ return error_errno("unable to get random bytes for temporary file"); + /* Fill in the random bits. */ for (i = 0; i < num_x; i++) { ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 1/2] wrapper: add a helper to generate numbers from a CSPRNG 2022-01-04 1:55 [PATCH v2 0/2] Generate temporary files using a CSPRNG brian m. carlson @ 2022-01-04 1:55 ` brian m. carlson 2022-01-04 21:01 ` Junio C Hamano 2022-01-04 1:55 ` [PATCH v2 2/2] wrapper: use a CSPRNG to generate random file names brian m. carlson ` (2 subsequent siblings) 3 siblings, 1 reply; 23+ messages in thread From: brian m. carlson @ 2022-01-04 1:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón There are many situations in which having access to a cryptographically secure pseudorandom number generator (CSPRNG) is helpful. In the future, we'll encounter one of these when dealing with temporary files. To make this possible, let's add a function which reads from a system CSPRNG and returns some bytes. We know that all systems will have such an interface. A CSPRNG is required for a secure TLS or SSH implementation and a Git implementation which provided neither would be of little practical use. In addition, POSIX is set to standardize getentropy(2) in the next version, so in the (potentially distant) future we can rely on that. For systems which lack one of the other interfaces, we provide the ability to use OpenSSL's CSPRNG. OpenSSL is highly portable and functions on practically every known OS, and we know it will have access to some source of cryptographically secure randomness. We also provide support for the arc4random in libbsd for folks who would prefer to use that. Because this is a security sensitive interface, we take some precautions. We either succeed by filling the buffer completely as we requested, or we fail. We don't return partial data because the caller will almost never find that to be a useful behavior. Specify a makefile knob which users can use to specify their preferred CSPRNG, and turn the multiple string options into a set of defines, since we cannot match on strings in the preprocessor. The order of suggested options is important here. On systems with arc4random, which is most of the BSDs, we suggest that, since, except on MirBSD and macOS, it uses ChaCha20, which is extremely fast, and sits entirely in userspace, avoiding a system call. We then prefer getrandom over getentropy, because the former has been available longer on Linux, and then OpenSSL. Finally, if none of those are available, we use /dev/urandom, because most Unix-like operating systems provide that API. We prefer to suggest options that don't involve device files when possible because those work in some restricted environments where device files may not be available. Set the configuration variables appropriately for Linux and the BSDs, including macOS, as well as Windows and NonStop. We specifically only consider versions which receive publicly available security support here. For the same reason, we don't specify getrandom(2) on Linux, because CentOS 7 doesn't support it in glibc (although its kernel does) and we don't want to resort to making syscalls. Finally, add a test helper to allow this to be tested by hand and in tests. We don't add any tests, since invoking the CSPRNG is not likely to produce interesting, reproducible results. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- Makefile | 33 +++++++++++++++ compat/winansi.c | 6 +++ config.mak.uname | 8 ++++ contrib/buildsystems/CMakeLists.txt | 2 +- git-compat-util.h | 19 +++++++++ t/helper/test-csprng.c | 29 +++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + wrapper.c | 66 +++++++++++++++++++++++++++++ 9 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 t/helper/test-csprng.c diff --git a/Makefile b/Makefile index 75ed168adb..c07e9cff01 100644 --- a/Makefile +++ b/Makefile @@ -234,6 +234,13 @@ all:: # Define NO_TRUSTABLE_FILEMODE if your filesystem may claim to support # the executable mode bit, but doesn't really do so. # +# Define CSPRNG_METHOD to "arc4random" if your system has arc4random and +# arc4random_buf, "arc4random-libbsd" if your system has those functions from +# libbsd, "getrandom" if your system has getrandom, "getentropy" if your +# system has getentropy, "rtlgenrandom" for RtlGenRandom (Windows only), or +# "openssl" if you'd want to use the OpenSSL CSPRNG. If unset or set to +# anything else, defaults to using "/dev/urandom". +# # Define NEEDS_MODE_TRANSLATION if your OS strays from the typical file type # bits in mode values (e.g. z/OS defines I_SFMT to 0xFF000000 as opposed to the # usual 0xF000). @@ -693,6 +700,7 @@ TEST_BUILTINS_OBJS += test-bloom.o TEST_BUILTINS_OBJS += test-chmtime.o TEST_BUILTINS_OBJS += test-config.o TEST_BUILTINS_OBJS += test-crontab.o +TEST_BUILTINS_OBJS += test-csprng.o TEST_BUILTINS_OBJS += test-ctype.o TEST_BUILTINS_OBJS += test-date.o TEST_BUILTINS_OBJS += test-delta.o @@ -1908,6 +1916,31 @@ ifdef HAVE_GETDELIM BASIC_CFLAGS += -DHAVE_GETDELIM endif +ifeq ($(strip $(CSPRNG_METHOD)),arc4random) + BASIC_CFLAGS += -DHAVE_ARC4RANDOM +endif + +ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd) + BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD + EXTLIBS += -lbsd +endif + +ifeq ($(strip $(CSPRNG_METHOD)),getrandom) + BASIC_CFLAGS += -DHAVE_GETRANDOM +endif + +ifeq ($(strip $(CSPRNG_METHOD)),getentropy) + BASIC_CFLAGS += -DHAVE_GETENTROPY +endif + +ifeq ($(strip $(CSPRNG_METHOD)),rtlgenrandom) + BASIC_CFLAGS += -DHAVE_RTLGENRANDOM +endif + +ifeq ($(strip $(CSPRNG_METHOD)),openssl) + BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG +endif + ifneq ($(PROCFS_EXECUTABLE_PATH),) procfs_executable_path_SQ = $(subst ','\'',$(PROCFS_EXECUTABLE_PATH)) BASIC_CFLAGS += '-DPROCFS_EXECUTABLE_PATH="$(procfs_executable_path_SQ)"' diff --git a/compat/winansi.c b/compat/winansi.c index c27b20a79d..0e5a9cc82e 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -3,6 +3,12 @@ */ #undef NOGDI + +/* + * Including the appropriate header file for RtlGenRandom causes MSVC to see a + * redefinition of types in an incompatible way when including headers below. + */ +#undef HAVE_RTLGENRANDOM #include "../git-compat-util.h" #include <wingdi.h> #include <winreg.h> diff --git a/config.mak.uname b/config.mak.uname index a3a779327f..ff0710a612 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -141,6 +141,7 @@ ifeq ($(uname_S),Darwin) HAVE_BSD_SYSCTL = YesPlease FREAD_READS_DIRECTORIES = UnfortunatelyYes HAVE_NS_GET_EXECUTABLE_PATH = YesPlease + CSPRNG_METHOD = arc4random # Workaround for `gettext` being keg-only and not even being linked via # `brew link --force gettext`, should be obsolete as of @@ -256,6 +257,7 @@ ifeq ($(uname_S),FreeBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random PAGER_ENV = LESS=FRX LV=-c MORE=FRX FREAD_READS_DIRECTORIES = UnfortunatelyYes FILENO_IS_A_MACRO = UnfortunatelyYes @@ -274,6 +276,7 @@ ifeq ($(uname_S),OpenBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random PROCFS_EXECUTABLE_PATH = /proc/curproc/file FREAD_READS_DIRECTORIES = UnfortunatelyYes FILENO_IS_A_MACRO = UnfortunatelyYes @@ -285,6 +288,7 @@ ifeq ($(uname_S),MirBSD) NEEDS_LIBICONV = YesPlease HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random endif ifeq ($(uname_S),NetBSD) ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2) @@ -296,6 +300,7 @@ ifeq ($(uname_S),NetBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random PROCFS_EXECUTABLE_PATH = /proc/curproc/exe endif ifeq ($(uname_S),AIX) @@ -425,6 +430,7 @@ ifeq ($(uname_S),Windows) NO_STRTOUMAX = YesPlease NO_MKDTEMP = YesPlease NO_INTTYPES_H = YesPlease + CSPRNG_METHOD = rtlgenrandom # VS2015 with UCRT claims that snprintf and friends are C99 compliant, # so we don't need this: # @@ -593,6 +599,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL) NO_MMAP = YesPlease NO_POLL = YesPlease NO_INTPTR_T = UnfortunatelyYes + CSPRNG_METHOD = openssl SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin SHELL_PATH = /usr/coreutils/bin/bash endif @@ -628,6 +635,7 @@ ifeq ($(uname_S),MINGW) NO_POSIX_GOODIES = UnfortunatelyYes DEFAULT_HELP_FORMAT = html HAVE_PLATFORM_PROCINFO = YesPlease + CSPRNG_METHOD = rtlgenrandom BASIC_LDFLAGS += -municode COMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32 COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 5100f56bb3..e44232f85d 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -260,7 +260,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows") _CONSOLE DETECT_MSYS_TTY STRIP_EXTENSION=".exe" NO_SYMLINK_HEAD UNRELIABLE_FSTAT NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 USE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP - UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET) + UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET HAVE_RTLGENRANDOM) list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c compat/win32/path-utils.c compat/win32/pthread.c compat/win32mmap.c compat/win32/syslog.c compat/win32/trace2_win32_process_info.c compat/win32/dirent.c diff --git a/git-compat-util.h b/git-compat-util.h index 5fa54a7afe..50597c76be 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -188,6 +188,12 @@ #endif #include <windows.h> #define GIT_WINDOWS_NATIVE +#ifdef HAVE_RTLGENRANDOM +/* This is required to get access to RtlGenRandom. */ +#define SystemFunction036 NTAPI SystemFunction036 +#include <NTSecAPI.h> +#undef SystemFunction036 +#endif #endif #include <unistd.h> @@ -258,6 +264,12 @@ #else #include <stdint.h> #endif +#ifdef HAVE_ARC4RANDOM_LIBBSD +#include <bsd/stdlib.h> +#endif +#ifdef HAVE_GETRANDOM +#include <sys/random.h> +#endif #ifdef NO_INTPTR_T /* * On I16LP32, ILP32 and LP64 "long" is the safe bet, however @@ -1421,4 +1433,11 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset) void sleep_millisec(int millisec); +/* + * Generate len bytes from the system cryptographically secure PRNG. + * Returns 0 on success and -1 on error, setting errno. The inability to + * satisfy the full request is an error. + */ +int csprng_bytes(void *buf, size_t len); + #endif diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c new file mode 100644 index 0000000000..65d14973c5 --- /dev/null +++ b/t/helper/test-csprng.c @@ -0,0 +1,29 @@ +#include "test-tool.h" +#include "git-compat-util.h" + + +int cmd__csprng(int argc, const char **argv) +{ + unsigned long count; + unsigned char buf[1024]; + + if (argc > 2) { + fprintf(stderr, "usage: %s [<size>]\n", argv[0]); + return 2; + } + + count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L; + + while (count) { + unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf); + if (csprng_bytes(buf, chunk) < 0) { + perror("failed to read"); + return 5; + } + if (fwrite(buf, chunk, 1, stdout) != chunk) + return 1; + count -= chunk; + } + + return 0; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 338a57b104..e6ec69cf32 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -20,6 +20,7 @@ static struct test_cmd cmds[] = { { "chmtime", cmd__chmtime }, { "config", cmd__config }, { "crontab", cmd__crontab }, + { "csprng", cmd__csprng }, { "ctype", cmd__ctype }, { "date", cmd__date }, { "delta", cmd__delta }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 48cee1f4a2..20756eefdd 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -10,6 +10,7 @@ int cmd__bloom(int argc, const char **argv); int cmd__chmtime(int argc, const char **argv); int cmd__config(int argc, const char **argv); int cmd__crontab(int argc, const char **argv); +int cmd__csprng(int argc, const char **argv); int cmd__ctype(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); diff --git a/wrapper.c b/wrapper.c index 36e12119d7..1052356703 100644 --- a/wrapper.c +++ b/wrapper.c @@ -702,3 +702,69 @@ int open_nofollow(const char *path, int flags) return open(path, flags); #endif } + +int csprng_bytes(void *buf, size_t len) +{ +#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD) + /* This function never returns an error. */ + arc4random_buf(buf, len); + return 0; +#elif defined(HAVE_GETRANDOM) + ssize_t res; + char *p = buf; + while (len) { + res = getrandom(p, len, 0); + if (res < 0) + return -1; + len -= res; + p += res; + } + return 0; +#elif defined(HAVE_GETENTROPY) + int res; + char *p = buf; + while (len) { + /* getentropy has a maximum size of 256 bytes. */ + size_t chunk = len < 256 ? len : 256; + res = getentropy(p, chunk); + if (res < 0) + return -1; + len -= chunk; + p += chunk; + } + return 0; +#elif defined(HAVE_RTLGENRANDOM) + if (!RtlGenRandom(buf, len)) + return -1; + return 0; +#elif defined(HAVE_OPENSSL_CSPRNG) + int res = RAND_bytes(buf, len); + if (res == 1) + return 0; + if (res == -1) + errno = ENOTSUP; + else + errno = EIO; + return -1; +#else + ssize_t res; + char *p = buf; + int fd, err; + fd = open("/dev/urandom", O_RDONLY); + if (fd < 0) + return -1; + while (len) { + res = xread(fd, p, len); + if (res < 0) { + err = errno; + close(fd); + errno = err; + return -1; + } + len -= res; + p += res; + } + close(fd); + return 0; +#endif +} ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] wrapper: add a helper to generate numbers from a CSPRNG 2022-01-04 1:55 ` [PATCH v2 1/2] wrapper: add a helper to generate numbers from " brian m. carlson @ 2022-01-04 21:01 ` Junio C Hamano 2022-01-04 21:39 ` Junio C Hamano 2022-01-04 22:56 ` brian m. carlson 0 siblings, 2 replies; 23+ messages in thread From: Junio C Hamano @ 2022-01-04 21:01 UTC (permalink / raw) To: brian m. carlson Cc: git, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón "brian m. carlson" <sandals@crustytoothpaste.net> writes: > +# Define CSPRNG_METHOD to "arc4random" if your system has arc4random and > +# arc4random_buf, "arc4random-libbsd" if your system has those functions from > +# libbsd, "getrandom" if your system has getrandom, "getentropy" if your > +# system has getentropy, "rtlgenrandom" for RtlGenRandom (Windows only), or > +# "openssl" if you'd want to use the OpenSSL CSPRNG. If unset or set to > +# anything else, defaults to using "/dev/urandom". > +# OK. > +ifeq ($(strip $(CSPRNG_METHOD)),arc4random) > + BASIC_CFLAGS += -DHAVE_ARC4RANDOM > +endif > + > +ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd) > + BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD > + EXTLIBS += -lbsd > +endif > + > +ifeq ($(strip $(CSPRNG_METHOD)),getrandom) > + BASIC_CFLAGS += -DHAVE_GETRANDOM > +endif > + > +ifeq ($(strip $(CSPRNG_METHOD)),getentropy) > + BASIC_CFLAGS += -DHAVE_GETENTROPY > +endif > + > +ifeq ($(strip $(CSPRNG_METHOD)),rtlgenrandom) > + BASIC_CFLAGS += -DHAVE_RTLGENRANDOM > +endif > + > +ifeq ($(strip $(CSPRNG_METHOD)),openssl) > + BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG > +endif Use of $(strip ($VAR)) looks a bit different from what everybody else does with ifeq in this Makefile. Was there a particular reason to use it that I am missing? When we see something unrecognized in CSPRNG_METHOD, we do not touch BASIC_CFLAGS (or EXTLIBS) here. I wonder how easy a clue we would have to decide that we need to fall back to urandom. IOW, I would have expected a if/else if/... cascade that has "no we do not have anything else and need to fall back to urandom" at the end. But that's OK, as long as the fallback logic is cleanly done. Let's keep reading... > diff --git a/git-compat-util.h b/git-compat-util.h > index 5fa54a7afe..50597c76be 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -1421,4 +1433,11 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset) > > void sleep_millisec(int millisec); > > +/* > + * Generate len bytes from the system cryptographically secure PRNG. > + * Returns 0 on success and -1 on error, setting errno. The inability to > + * satisfy the full request is an error. > + */ > +int csprng_bytes(void *buf, size_t len); > + > #endif > diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c > new file mode 100644 > index 0000000000..65d14973c5 > --- /dev/null > +++ b/t/helper/test-csprng.c > @@ -0,0 +1,29 @@ > +#include "test-tool.h" > +#include "git-compat-util.h" > + > + > +int cmd__csprng(int argc, const char **argv) > +{ > + unsigned long count; > + unsigned char buf[1024]; > + > + if (argc > 2) { > + fprintf(stderr, "usage: %s [<size>]\n", argv[0]); > + return 2; > + } > + > + count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L; > + > + while (count) { > + unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf); "chunk" should be of type "size_t", no? > diff --git a/wrapper.c b/wrapper.c > index 36e12119d7..1052356703 100644 > --- a/wrapper.c > +++ b/wrapper.c > @@ -702,3 +702,69 @@ int open_nofollow(const char *path, int flags) > return open(path, flags); > #endif > } > + > +int csprng_bytes(void *buf, size_t len) > +{ > +#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD) Shouldn't HAVE_ARC4RANDOM mean "we have arc4random_buf() function available; please use that.", i.e. wouldn't this give us cleaner result in the change to the Makefile? ifeq ($(strip $(CSPRNG_METHOD)),arc4random) BASIC_CFLAGS += -DHAVE_ARC4RANDOM endif ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd) - BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD + BASIC_CFLAGS += -DHAVE_ARC4RANDOM EXTLIBS += -lbsd endif To put it differently, C source, via BASIC_CFLAGS, would not have to care where the function definition comes from. It is linker's job to care and we are already telling it via EXTLIBS, so I am not sure the value of having HAVE_ARC4RANDOM_LIBBSD as a separate symbol. > + /* This function never returns an error. */ > + arc4random_buf(buf, len); > + return 0; > +#elif defined(HAVE_GETRANDOM) > + ssize_t res; > + char *p = buf; > + while (len) { > + res = getrandom(p, len, 0); > + if (res < 0) > + return -1; > + len -= res; > + p += res; > + } > + return 0; > +#elif defined(HAVE_GETENTROPY) > + int res; > + char *p = buf; > + while (len) { > + /* getentropy has a maximum size of 256 bytes. */ > + size_t chunk = len < 256 ? len : 256; > + res = getentropy(p, chunk); > + if (res < 0) > + return -1; > + len -= chunk; > + p += chunk; > + } > + return 0; > +#elif defined(HAVE_RTLGENRANDOM) > + if (!RtlGenRandom(buf, len)) > + return -1; > + return 0; > +#elif defined(HAVE_OPENSSL_CSPRNG) > + int res = RAND_bytes(buf, len); > + if (res == 1) > + return 0; > + if (res == -1) > + errno = ENOTSUP; > + else > + errno = EIO; > + return -1; > +#else > + ssize_t res; > + char *p = buf; > + int fd, err; > + fd = open("/dev/urandom", O_RDONLY); > + if (fd < 0) > + return -1; > + while (len) { > + res = xread(fd, p, len); > + if (res < 0) { > + err = errno; > + close(fd); > + errno = err; > + return -1; > + } > + len -= res; > + p += res; > + } > + close(fd); > + return 0; > +#endif > +} OK, I earlier worried about the lack of explicit "we are using urandom" at the Makefile level, but as long as this will remain the only place that needs to care about the fallback, the above is perfectly fine. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] wrapper: add a helper to generate numbers from a CSPRNG 2022-01-04 21:01 ` Junio C Hamano @ 2022-01-04 21:39 ` Junio C Hamano 2022-01-04 23:12 ` brian m. carlson 2022-01-04 22:56 ` brian m. carlson 1 sibling, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2022-01-04 21:39 UTC (permalink / raw) To: brian m. carlson Cc: git, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón Junio C Hamano <gitster@pobox.com> writes: >> +ifeq ($(strip $(CSPRNG_METHOD)),arc4random) >> + BASIC_CFLAGS += -DHAVE_ARC4RANDOM >> +endif >> + >> +ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd) >> + BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD >> + EXTLIBS += -lbsd >> +endif >> + >> +ifeq ($(strip $(CSPRNG_METHOD)),getrandom) >> + BASIC_CFLAGS += -DHAVE_GETRANDOM >> +endif >> + >> +ifeq ($(strip $(CSPRNG_METHOD)),getentropy) >> + BASIC_CFLAGS += -DHAVE_GETENTROPY >> +endif >> + >> +ifeq ($(strip $(CSPRNG_METHOD)),rtlgenrandom) >> + BASIC_CFLAGS += -DHAVE_RTLGENRANDOM >> +endif >> + >> +ifeq ($(strip $(CSPRNG_METHOD)),openssl) >> + BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG >> +endif > > Use of $(strip ($VAR)) looks a bit different from what everybody > else does with ifeq in this Makefile. Was there a particular reason > to use it that I am missing? Another thought. As far as I can see on the C code side in the later part of the patch, we are prepared to see multiple HAVE_* for CSPRNG defined by the builders, and let us choose the best one for them. I wonder if it makes sense to allow make CSPRNG_METHOD='arc4random getentropy' as a way to tell us that they have these two and want us to pick the best one for them. It does not add much value for human builders, but I suspect that it would make it simpler when we need to add autoconf support. If we do not allow multiple methods listed on the CSPRNG_METHOD variable, the configure script will be forced to pick one in some way when multiple methods are possible on the platform, either by detecting all and picking one, or detecting serially from most preferred and stopping at the first hit. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] wrapper: add a helper to generate numbers from a CSPRNG 2022-01-04 21:39 ` Junio C Hamano @ 2022-01-04 23:12 ` brian m. carlson 0 siblings, 0 replies; 23+ messages in thread From: brian m. carlson @ 2022-01-04 23:12 UTC (permalink / raw) To: Junio C Hamano Cc: git, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón [-- Attachment #1: Type: text/plain, Size: 715 bytes --] On 2022-01-04 at 21:39:37, Junio C Hamano wrote: > Another thought. As far as I can see on the C code side in the > later part of the patch, we are prepared to see multiple HAVE_* for > CSPRNG defined by the builders, and let us choose the best one for > them. I wonder if it makes sense to allow > > make CSPRNG_METHOD='arc4random getentropy' > > as a way to tell us that they have these two and want us to pick the > best one for them. > > It does not add much value for human builders, but I suspect that it > would make it simpler when we need to add autoconf support. That shouldn't be a big deal. I can do that. -- brian m. carlson (he/him or they/them) Toronto, Ontario, CA [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 262 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] wrapper: add a helper to generate numbers from a CSPRNG 2022-01-04 21:01 ` Junio C Hamano 2022-01-04 21:39 ` Junio C Hamano @ 2022-01-04 22:56 ` brian m. carlson 2022-01-04 23:17 ` Junio C Hamano 1 sibling, 1 reply; 23+ messages in thread From: brian m. carlson @ 2022-01-04 22:56 UTC (permalink / raw) To: Junio C Hamano Cc: git, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón [-- Attachment #1: Type: text/plain, Size: 4912 bytes --] On 2022-01-04 at 21:01:19, Junio C Hamano wrote: > "brian m. carlson" <sandals@crustytoothpaste.net> writes: > > +ifeq ($(strip $(CSPRNG_METHOD)),arc4random) > > + BASIC_CFLAGS += -DHAVE_ARC4RANDOM > > +endif > > + > > +ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd) > > + BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD > > + EXTLIBS += -lbsd > > +endif > > + > > +ifeq ($(strip $(CSPRNG_METHOD)),getrandom) > > + BASIC_CFLAGS += -DHAVE_GETRANDOM > > +endif > > + > > +ifeq ($(strip $(CSPRNG_METHOD)),getentropy) > > + BASIC_CFLAGS += -DHAVE_GETENTROPY > > +endif > > + > > +ifeq ($(strip $(CSPRNG_METHOD)),rtlgenrandom) > > + BASIC_CFLAGS += -DHAVE_RTLGENRANDOM > > +endif > > + > > +ifeq ($(strip $(CSPRNG_METHOD)),openssl) > > + BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG > > +endif > > Use of $(strip ($VAR)) looks a bit different from what everybody > else does with ifeq in this Makefile. Was there a particular reason > to use it that I am missing? As far as I'm aware, ifeq includes whitespace in its comparisons (at least, I was led to believe that by the documentation for GNU make). However, in many places, we insert leading spaces before the variable name. Some testing shows that GNU make strips those off, although my earlier testing led me to believe otherwise. So I'll change this in v3. > When we see something unrecognized in CSPRNG_METHOD, we do not touch > BASIC_CFLAGS (or EXTLIBS) here. I wonder how easy a clue we would > have to decide that we need to fall back to urandom. IOW, I would > have expected a if/else if/... cascade that has "no we do not have > anything else and need to fall back to urandom" at the end. I tried to avoid the if/else if cascade for the same reasons that we do in config.mak.uname, which is that it gets pretty hard to reason about after you have more than just a few items. > > diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c > > new file mode 100644 > > index 0000000000..65d14973c5 > > --- /dev/null > > +++ b/t/helper/test-csprng.c > > @@ -0,0 +1,29 @@ > > +#include "test-tool.h" > > +#include "git-compat-util.h" > > + > > + > > +int cmd__csprng(int argc, const char **argv) > > +{ > > + unsigned long count; > > + unsigned char buf[1024]; > > + > > + if (argc > 2) { > > + fprintf(stderr, "usage: %s [<size>]\n", argv[0]); > > + return 2; > > + } > > + > > + count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L; > > + > > + while (count) { > > + unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf); > > "chunk" should be of type "size_t", no? Yes, it should. Will fix in v3. > > diff --git a/wrapper.c b/wrapper.c > > index 36e12119d7..1052356703 100644 > > --- a/wrapper.c > > +++ b/wrapper.c > > @@ -702,3 +702,69 @@ int open_nofollow(const char *path, int flags) > > return open(path, flags); > > #endif > > } > > + > > +int csprng_bytes(void *buf, size_t len) > > +{ > > +#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD) > > Shouldn't HAVE_ARC4RANDOM mean "we have arc4random_buf() function > available; please use that.", i.e. wouldn't this give us cleaner > result in the change to the Makefile? > > ifeq ($(strip $(CSPRNG_METHOD)),arc4random) > BASIC_CFLAGS += -DHAVE_ARC4RANDOM > endif > > ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd) > - BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD > + BASIC_CFLAGS += -DHAVE_ARC4RANDOM > EXTLIBS += -lbsd > endif > > To put it differently, C source, via BASIC_CFLAGS, would not have to > care where the function definition comes from. It is linker's job > to care and we are already telling it via EXTLIBS, so I am not sure > the value of having HAVE_ARC4RANDOM_LIBBSD as a separate symbol. There's an important additional difference here. On real BSD systems, the prototypes for this family of functions live in <stdlib.h>. Obviously, on Linux with libbsd, that's not the case. So, in git-compat-util.h, we need to include <bsd/stdlib.h> to get the prototype, and that's done only if HAVE_ARC4RANDOM_LIBBSD is set, since on a real BSD system, that header isn't present. If you'd prefer, I can set both flags here, one which controls the function use and one which controls the #define, or I can leave it as it is. > OK, I earlier worried about the lack of explicit "we are using > urandom" at the Makefile level, but as long as this will remain the > only place that needs to care about the fallback, the above is > perfectly fine. Yes, I tried very hard to make this the only place the default mattered. And, for the record, I think /dev/urandom is the sensible default here, because I know it exists on some of the proprietary Unix systems, like Solaris, where I believe it originated, and QNX, as well as Linux and the BSDs. -- brian m. carlson (he/him or they/them) Toronto, Ontario, CA [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 262 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] wrapper: add a helper to generate numbers from a CSPRNG 2022-01-04 22:56 ` brian m. carlson @ 2022-01-04 23:17 ` Junio C Hamano 0 siblings, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2022-01-04 23:17 UTC (permalink / raw) To: brian m. carlson Cc: git, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón "brian m. carlson" <sandals@crustytoothpaste.net> writes: > There's an important additional difference here. On real BSD systems, > the prototypes for this family of functions live in <stdlib.h>. Ah, I missed that one. So "C side does not care between HAVE_ARC4RANDOM_LIBBSD and HAVE_ARC4RANDOM" is not true. We do care and the patch as posted is fine. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 2/2] wrapper: use a CSPRNG to generate random file names 2022-01-04 1:55 [PATCH v2 0/2] Generate temporary files using a CSPRNG brian m. carlson 2022-01-04 1:55 ` [PATCH v2 1/2] wrapper: add a helper to generate numbers from " brian m. carlson @ 2022-01-04 1:55 ` brian m. carlson 2022-01-04 12:44 ` [PATCH v2 0/2] Generate temporary files using a CSPRNG Johannes Schindelin 2022-01-17 21:56 ` [PATCH v3 " brian m. carlson 3 siblings, 0 replies; 23+ messages in thread From: brian m. carlson @ 2022-01-04 1:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón The current way we generate random file names is by taking the seconds and microseconds, plus the PID, and mixing them together, then encoding them. If this fails, we increment the value by 7777, and try again up to TMP_MAX times. Unfortunately, this is not the best idea from a security perspective. If we're writing into TMPDIR, an attacker can guess these values easily and prevent us from creating any temporary files at all by creating them all first. Even though we set TMP_MAX to 16384, this may be achievable in some contexts, even if unlikely to occur in practice. Fortunately, we can simply solve this by using the system cryptographically secure pseudorandom number generator (CSPRNG) to generate a random 64-bit value, and use that as before. Note that there is still a small bias here, but because a six-character sequence chosen out of 62 characters provides about 36 bits of entropy, the bias here is less than 2^-28, which is acceptable, especially considering we'll retry several times. Note that the use of a CSPRNG in generating temporary file names is also used in many libcs. glibc recently changed from an approach similar to ours to using a CSPRNG, and FreeBSD and OpenBSD also use a CSPRNG in this case. Even if the likelihood of an attack is low, we should still be at least as responsible in creating temporary files as libc is. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- wrapper.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/wrapper.c b/wrapper.c index 1052356703..3258cdb171 100644 --- a/wrapper.c +++ b/wrapper.c @@ -463,8 +463,6 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) static const int num_letters = ARRAY_SIZE(letters) - 1; static const char x_pattern[] = "XXXXXX"; static const int num_x = ARRAY_SIZE(x_pattern) - 1; - uint64_t value; - struct timeval tv; char *filename_template; size_t len; int fd, count; @@ -485,12 +483,13 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) * Replace pattern's XXXXXX characters with randomness. * Try TMP_MAX different filenames. */ - gettimeofday(&tv, NULL); - value = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid(); filename_template = &pattern[len - num_x - suffix_len]; for (count = 0; count < TMP_MAX; ++count) { - uint64_t v = value; int i; + uint64_t v; + if (csprng_bytes(&v, sizeof(v)) < 0) + return error_errno("unable to get random bytes for temporary file"); + /* Fill in the random bits. */ for (i = 0; i < num_x; i++) { filename_template[i] = letters[v % num_letters]; @@ -506,12 +505,6 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) */ if (errno != EEXIST) break; - /* - * This is a random value. It is only necessary that - * the next TMP_MAX values generated by adding 7777 to - * VALUE are different with (module 2^32). - */ - value += 7777; } /* We return the null string if we can't find a unique file name. */ pattern[0] = '\0'; ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 0/2] Generate temporary files using a CSPRNG 2022-01-04 1:55 [PATCH v2 0/2] Generate temporary files using a CSPRNG brian m. carlson 2022-01-04 1:55 ` [PATCH v2 1/2] wrapper: add a helper to generate numbers from " brian m. carlson 2022-01-04 1:55 ` [PATCH v2 2/2] wrapper: use a CSPRNG to generate random file names brian m. carlson @ 2022-01-04 12:44 ` Johannes Schindelin 2022-01-17 21:56 ` [PATCH v3 " brian m. carlson 3 siblings, 0 replies; 23+ messages in thread From: Johannes Schindelin @ 2022-01-04 12:44 UTC (permalink / raw) To: brian m. carlson Cc: git, Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón Hi brian, On Tue, 4 Jan 2022, brian m. carlson wrote: > Currently, when we generate a temporary file name, we use the seconds, > microseconds, and the PID to generate a unique value. The resulting > value, while changing frequently, is actually predictable and on some > systems, it may be possible to cause a DoS by creating all potential > temporary files when the temporary file is being created in TMPDIR. > > The solution to this is to use the system CSPRNG to generate the > temporary file name. This is the approach taken by FreeBSD, NetBSD, and > OpenBSD, and glibc also recently switched to this approach from an > approach that resembled ours in many ways. > > [...] This gets my happy Reviewed-By:, and I very much liked the thoroughness and diligence of your work. It does an excellent job of motivating the change, and documents the consideration and diligence that has gone into the patches. Thank you for this refreshing patch series, which provided me with a nice start into the new year. To show that I did not just glance over this for five minutes and then moved on to the next email in order to throw as many emails at the Git mailing list as possible, I'd like to state that I mulled for a while over the change in `compat/winansi.c`. It first got me concerned that the changes to `git-compat-util.h` might be done at the wrong layer: if something as unrelated to the winansi emulation as temporary files creation requires fiddling, so that it can continue to include the headers it requires, wouldn't it make more sense to create a new file called `csprng.c` and add the new conditional `#include`s _there? Alas, it _is_ Git's custom to hide platform-dependent parts in `git-compat-util.h` as much as possible. And personal opinions should always stand back in favor of consistency of the code base. In short: I agree with your choice to add the conditional `#include`s to `git-compat-util.h` even if it requires a somewhat stray (but of course well-documented, thank you for that!) `#undef` in `compat/winansi.c`. Thanks! Dscho ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 0/2] Generate temporary files using a CSPRNG 2022-01-04 1:55 [PATCH v2 0/2] Generate temporary files using a CSPRNG brian m. carlson ` (2 preceding siblings ...) 2022-01-04 12:44 ` [PATCH v2 0/2] Generate temporary files using a CSPRNG Johannes Schindelin @ 2022-01-17 21:56 ` brian m. carlson 2022-01-17 21:56 ` [PATCH v3 1/2] wrapper: add a helper to generate numbers from " brian m. carlson 2022-01-17 21:56 ` [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names brian m. carlson 3 siblings, 2 replies; 23+ messages in thread From: brian m. carlson @ 2022-01-17 21:56 UTC (permalink / raw) To: git Cc: Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin Currently, when we generate a temporary file name, we use the seconds, microseconds, and the PID to generate a unique value. The resulting value, while changing frequently, is actually predictable and on some systems, it may be possible to cause a DoS by creating all potential temporary files when the temporary file is being created in TMPDIR. The solution to this is to use the system CSPRNG to generate the temporary file name. This is the approach taken by FreeBSD, NetBSD, and OpenBSD, and glibc also recently switched to this approach from an approach that resembled ours in many ways. Even if this is not practically exploitable on many systems, it seems prudent to be at least as careful about temporary file generation as libc is. In this round, I've switched to allow multiple values for the makefile variable to make our usage in autoconf easier. I have not submitted any changes to the autoconf code, since the defaults are fine for most situations, and even a fallback to /dev/urandom should be okay in most places. I also haven't included multiple values in config.mak.uname since there's little reason to do so: the BSDs all have had arc4random for a long time, Windows and NonStop have only one good option, and no other systems have a value set. A range-diff appears below for your convenience. Changes from v2: * Switch from a single option in the makefile variable to multiple options. * Change the name of the libbsd option to "libbsd" so it doesn't share a substring with other options due to the function we use. Changes from v1: * Remove the automatic testing using buckets because it didn't seem to add much. * Switch to a single makefile variable. * Add support for OpenSSL CSPRNG. * Add more defaults for various systems, including macOS and NonStop. * Add an arc4random-libbsd variant for improved testing of the arc4random code paths on Linux. The only difference is the inclusion of an additional header. * Print a more useful error message than before, indicating that the CSPRNG failed. brian m. carlson (2): wrapper: add a helper to generate numbers from a CSPRNG wrapper: use a CSPRNG to generate random file names Makefile | 34 ++++++++++++ compat/winansi.c | 6 +++ config.mak.uname | 8 +++ contrib/buildsystems/CMakeLists.txt | 2 +- git-compat-util.h | 19 +++++++ t/helper/test-csprng.c | 29 +++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + wrapper.c | 81 +++++++++++++++++++++++++---- 9 files changed, 169 insertions(+), 12 deletions(-) create mode 100644 t/helper/test-csprng.c Diff-intervalle contre v2 : 1: edd623bd9a ! 1: 6644235af2 wrapper: add a helper to generate numbers from a CSPRNG @@ Commit message requested, or we fail. We don't return partial data because the caller will almost never find that to be a useful behavior. - Specify a makefile knob which users can use to specify their preferred - CSPRNG, and turn the multiple string options into a set of defines, - since we cannot match on strings in the preprocessor. + Specify a makefile knob which users can use to specify one or more + suitable CSPRNGs, and turn the multiple string options into a set of + defines, since we cannot match on strings in the preprocessor. We allow + multiple options to make the job of handling this in autoconf easier. - The order of suggested options is important here. On systems with - arc4random, which is most of the BSDs, we suggest that, since, except on - MirBSD and macOS, it uses ChaCha20, which is extremely fast, and sits - entirely in userspace, avoiding a system call. We then prefer getrandom - over getentropy, because the former has been available longer on Linux, - and then OpenSSL. Finally, if none of those are available, we use + The order of options is important here. On systems with arc4random, + which is most of the BSDs, we use that, since, except on MirBSD and + macOS, it uses ChaCha20, which is extremely fast, and sits entirely in + userspace, avoiding a system call. We then prefer getrandom over + getentropy, because the former has been available longer on Linux, and + then OpenSSL. Finally, if none of those are available, we use /dev/urandom, because most Unix-like operating systems provide that API. - We prefer to suggest options that don't involve device files when - possible because those work in some restricted environments where device - files may not be available. + We prefer options that don't involve device files when possible because + those work in some restricted environments where device files may not be + available. Set the configuration variables appropriately for Linux and the BSDs, including macOS, as well as Windows and NonStop. We specifically only @@ Makefile: all:: # the executable mode bit, but doesn't really do so. # +# Define CSPRNG_METHOD to "arc4random" if your system has arc4random and -+# arc4random_buf, "arc4random-libbsd" if your system has those functions from -+# libbsd, "getrandom" if your system has getrandom, "getentropy" if your -+# system has getentropy, "rtlgenrandom" for RtlGenRandom (Windows only), or -+# "openssl" if you'd want to use the OpenSSL CSPRNG. If unset or set to ++# arc4random_buf, "libbsd" if your system has those functions from libbsd, ++# "getrandom" if your system has getrandom, "getentropy" if your system has ++# getentropy, "rtlgenrandom" for RtlGenRandom (Windows only), or "openssl" if ++# you'd want to use the OpenSSL CSPRNG. You may set multiple options with ++# spaces, in which case a suitable option will be chosen. If unset or set to +# anything else, defaults to using "/dev/urandom". +# # Define NEEDS_MODE_TRANSLATION if your OS strays from the typical file type @@ Makefile: ifdef HAVE_GETDELIM BASIC_CFLAGS += -DHAVE_GETDELIM endif -+ifeq ($(strip $(CSPRNG_METHOD)),arc4random) ++ifneq ($(findstring arc4random,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_ARC4RANDOM +endif + -+ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd) ++ifneq ($(findstring libbsd,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD + EXTLIBS += -lbsd +endif + -+ifeq ($(strip $(CSPRNG_METHOD)),getrandom) ++ifneq ($(findstring getrandom,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_GETRANDOM +endif + -+ifeq ($(strip $(CSPRNG_METHOD)),getentropy) ++ifneq ($(findstring getentropy,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_GETENTROPY +endif + -+ifeq ($(strip $(CSPRNG_METHOD)),rtlgenrandom) ++ifneq ($(findstring rtlgenrandom,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_RTLGENRANDOM +endif + -+ifeq ($(strip $(CSPRNG_METHOD)),openssl) ++ifneq ($(findstring openssl,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG +endif + 2: b4cd8700e3 = 2: c6d7d686f2 wrapper: use a CSPRNG to generate random file names ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 1/2] wrapper: add a helper to generate numbers from a CSPRNG 2022-01-17 21:56 ` [PATCH v3 " brian m. carlson @ 2022-01-17 21:56 ` brian m. carlson 2022-01-18 9:45 ` Ævar Arnfjörð Bjarmason 2022-01-17 21:56 ` [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names brian m. carlson 1 sibling, 1 reply; 23+ messages in thread From: brian m. carlson @ 2022-01-17 21:56 UTC (permalink / raw) To: git Cc: Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin There are many situations in which having access to a cryptographically secure pseudorandom number generator (CSPRNG) is helpful. In the future, we'll encounter one of these when dealing with temporary files. To make this possible, let's add a function which reads from a system CSPRNG and returns some bytes. We know that all systems will have such an interface. A CSPRNG is required for a secure TLS or SSH implementation and a Git implementation which provided neither would be of little practical use. In addition, POSIX is set to standardize getentropy(2) in the next version, so in the (potentially distant) future we can rely on that. For systems which lack one of the other interfaces, we provide the ability to use OpenSSL's CSPRNG. OpenSSL is highly portable and functions on practically every known OS, and we know it will have access to some source of cryptographically secure randomness. We also provide support for the arc4random in libbsd for folks who would prefer to use that. Because this is a security sensitive interface, we take some precautions. We either succeed by filling the buffer completely as we requested, or we fail. We don't return partial data because the caller will almost never find that to be a useful behavior. Specify a makefile knob which users can use to specify one or more suitable CSPRNGs, and turn the multiple string options into a set of defines, since we cannot match on strings in the preprocessor. We allow multiple options to make the job of handling this in autoconf easier. The order of options is important here. On systems with arc4random, which is most of the BSDs, we use that, since, except on MirBSD and macOS, it uses ChaCha20, which is extremely fast, and sits entirely in userspace, avoiding a system call. We then prefer getrandom over getentropy, because the former has been available longer on Linux, and then OpenSSL. Finally, if none of those are available, we use /dev/urandom, because most Unix-like operating systems provide that API. We prefer options that don't involve device files when possible because those work in some restricted environments where device files may not be available. Set the configuration variables appropriately for Linux and the BSDs, including macOS, as well as Windows and NonStop. We specifically only consider versions which receive publicly available security support here. For the same reason, we don't specify getrandom(2) on Linux, because CentOS 7 doesn't support it in glibc (although its kernel does) and we don't want to resort to making syscalls. Finally, add a test helper to allow this to be tested by hand and in tests. We don't add any tests, since invoking the CSPRNG is not likely to produce interesting, reproducible results. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- Makefile | 34 +++++++++++++++ compat/winansi.c | 6 +++ config.mak.uname | 8 ++++ contrib/buildsystems/CMakeLists.txt | 2 +- git-compat-util.h | 19 +++++++++ t/helper/test-csprng.c | 29 +++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + wrapper.c | 66 +++++++++++++++++++++++++++++ 9 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 t/helper/test-csprng.c diff --git a/Makefile b/Makefile index 75ed168adb..198b759e76 100644 --- a/Makefile +++ b/Makefile @@ -234,6 +234,14 @@ all:: # Define NO_TRUSTABLE_FILEMODE if your filesystem may claim to support # the executable mode bit, but doesn't really do so. # +# Define CSPRNG_METHOD to "arc4random" if your system has arc4random and +# arc4random_buf, "libbsd" if your system has those functions from libbsd, +# "getrandom" if your system has getrandom, "getentropy" if your system has +# getentropy, "rtlgenrandom" for RtlGenRandom (Windows only), or "openssl" if +# you'd want to use the OpenSSL CSPRNG. You may set multiple options with +# spaces, in which case a suitable option will be chosen. If unset or set to +# anything else, defaults to using "/dev/urandom". +# # Define NEEDS_MODE_TRANSLATION if your OS strays from the typical file type # bits in mode values (e.g. z/OS defines I_SFMT to 0xFF000000 as opposed to the # usual 0xF000). @@ -693,6 +701,7 @@ TEST_BUILTINS_OBJS += test-bloom.o TEST_BUILTINS_OBJS += test-chmtime.o TEST_BUILTINS_OBJS += test-config.o TEST_BUILTINS_OBJS += test-crontab.o +TEST_BUILTINS_OBJS += test-csprng.o TEST_BUILTINS_OBJS += test-ctype.o TEST_BUILTINS_OBJS += test-date.o TEST_BUILTINS_OBJS += test-delta.o @@ -1908,6 +1917,31 @@ ifdef HAVE_GETDELIM BASIC_CFLAGS += -DHAVE_GETDELIM endif +ifneq ($(findstring arc4random,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_ARC4RANDOM +endif + +ifneq ($(findstring libbsd,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD + EXTLIBS += -lbsd +endif + +ifneq ($(findstring getrandom,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_GETRANDOM +endif + +ifneq ($(findstring getentropy,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_GETENTROPY +endif + +ifneq ($(findstring rtlgenrandom,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_RTLGENRANDOM +endif + +ifneq ($(findstring openssl,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG +endif + ifneq ($(PROCFS_EXECUTABLE_PATH),) procfs_executable_path_SQ = $(subst ','\'',$(PROCFS_EXECUTABLE_PATH)) BASIC_CFLAGS += '-DPROCFS_EXECUTABLE_PATH="$(procfs_executable_path_SQ)"' diff --git a/compat/winansi.c b/compat/winansi.c index c27b20a79d..0e5a9cc82e 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -3,6 +3,12 @@ */ #undef NOGDI + +/* + * Including the appropriate header file for RtlGenRandom causes MSVC to see a + * redefinition of types in an incompatible way when including headers below. + */ +#undef HAVE_RTLGENRANDOM #include "../git-compat-util.h" #include <wingdi.h> #include <winreg.h> diff --git a/config.mak.uname b/config.mak.uname index a3a779327f..ff0710a612 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -141,6 +141,7 @@ ifeq ($(uname_S),Darwin) HAVE_BSD_SYSCTL = YesPlease FREAD_READS_DIRECTORIES = UnfortunatelyYes HAVE_NS_GET_EXECUTABLE_PATH = YesPlease + CSPRNG_METHOD = arc4random # Workaround for `gettext` being keg-only and not even being linked via # `brew link --force gettext`, should be obsolete as of @@ -256,6 +257,7 @@ ifeq ($(uname_S),FreeBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random PAGER_ENV = LESS=FRX LV=-c MORE=FRX FREAD_READS_DIRECTORIES = UnfortunatelyYes FILENO_IS_A_MACRO = UnfortunatelyYes @@ -274,6 +276,7 @@ ifeq ($(uname_S),OpenBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random PROCFS_EXECUTABLE_PATH = /proc/curproc/file FREAD_READS_DIRECTORIES = UnfortunatelyYes FILENO_IS_A_MACRO = UnfortunatelyYes @@ -285,6 +288,7 @@ ifeq ($(uname_S),MirBSD) NEEDS_LIBICONV = YesPlease HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random endif ifeq ($(uname_S),NetBSD) ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2) @@ -296,6 +300,7 @@ ifeq ($(uname_S),NetBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random PROCFS_EXECUTABLE_PATH = /proc/curproc/exe endif ifeq ($(uname_S),AIX) @@ -425,6 +430,7 @@ ifeq ($(uname_S),Windows) NO_STRTOUMAX = YesPlease NO_MKDTEMP = YesPlease NO_INTTYPES_H = YesPlease + CSPRNG_METHOD = rtlgenrandom # VS2015 with UCRT claims that snprintf and friends are C99 compliant, # so we don't need this: # @@ -593,6 +599,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL) NO_MMAP = YesPlease NO_POLL = YesPlease NO_INTPTR_T = UnfortunatelyYes + CSPRNG_METHOD = openssl SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin SHELL_PATH = /usr/coreutils/bin/bash endif @@ -628,6 +635,7 @@ ifeq ($(uname_S),MINGW) NO_POSIX_GOODIES = UnfortunatelyYes DEFAULT_HELP_FORMAT = html HAVE_PLATFORM_PROCINFO = YesPlease + CSPRNG_METHOD = rtlgenrandom BASIC_LDFLAGS += -municode COMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32 COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 5100f56bb3..e44232f85d 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -260,7 +260,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows") _CONSOLE DETECT_MSYS_TTY STRIP_EXTENSION=".exe" NO_SYMLINK_HEAD UNRELIABLE_FSTAT NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 USE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP - UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET) + UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET HAVE_RTLGENRANDOM) list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c compat/win32/path-utils.c compat/win32/pthread.c compat/win32mmap.c compat/win32/syslog.c compat/win32/trace2_win32_process_info.c compat/win32/dirent.c diff --git a/git-compat-util.h b/git-compat-util.h index 5fa54a7afe..50597c76be 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -188,6 +188,12 @@ #endif #include <windows.h> #define GIT_WINDOWS_NATIVE +#ifdef HAVE_RTLGENRANDOM +/* This is required to get access to RtlGenRandom. */ +#define SystemFunction036 NTAPI SystemFunction036 +#include <NTSecAPI.h> +#undef SystemFunction036 +#endif #endif #include <unistd.h> @@ -258,6 +264,12 @@ #else #include <stdint.h> #endif +#ifdef HAVE_ARC4RANDOM_LIBBSD +#include <bsd/stdlib.h> +#endif +#ifdef HAVE_GETRANDOM +#include <sys/random.h> +#endif #ifdef NO_INTPTR_T /* * On I16LP32, ILP32 and LP64 "long" is the safe bet, however @@ -1421,4 +1433,11 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset) void sleep_millisec(int millisec); +/* + * Generate len bytes from the system cryptographically secure PRNG. + * Returns 0 on success and -1 on error, setting errno. The inability to + * satisfy the full request is an error. + */ +int csprng_bytes(void *buf, size_t len); + #endif diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c new file mode 100644 index 0000000000..65d14973c5 --- /dev/null +++ b/t/helper/test-csprng.c @@ -0,0 +1,29 @@ +#include "test-tool.h" +#include "git-compat-util.h" + + +int cmd__csprng(int argc, const char **argv) +{ + unsigned long count; + unsigned char buf[1024]; + + if (argc > 2) { + fprintf(stderr, "usage: %s [<size>]\n", argv[0]); + return 2; + } + + count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L; + + while (count) { + unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf); + if (csprng_bytes(buf, chunk) < 0) { + perror("failed to read"); + return 5; + } + if (fwrite(buf, chunk, 1, stdout) != chunk) + return 1; + count -= chunk; + } + + return 0; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 338a57b104..e6ec69cf32 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -20,6 +20,7 @@ static struct test_cmd cmds[] = { { "chmtime", cmd__chmtime }, { "config", cmd__config }, { "crontab", cmd__crontab }, + { "csprng", cmd__csprng }, { "ctype", cmd__ctype }, { "date", cmd__date }, { "delta", cmd__delta }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 48cee1f4a2..20756eefdd 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -10,6 +10,7 @@ int cmd__bloom(int argc, const char **argv); int cmd__chmtime(int argc, const char **argv); int cmd__config(int argc, const char **argv); int cmd__crontab(int argc, const char **argv); +int cmd__csprng(int argc, const char **argv); int cmd__ctype(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); diff --git a/wrapper.c b/wrapper.c index 36e12119d7..1052356703 100644 --- a/wrapper.c +++ b/wrapper.c @@ -702,3 +702,69 @@ int open_nofollow(const char *path, int flags) return open(path, flags); #endif } + +int csprng_bytes(void *buf, size_t len) +{ +#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD) + /* This function never returns an error. */ + arc4random_buf(buf, len); + return 0; +#elif defined(HAVE_GETRANDOM) + ssize_t res; + char *p = buf; + while (len) { + res = getrandom(p, len, 0); + if (res < 0) + return -1; + len -= res; + p += res; + } + return 0; +#elif defined(HAVE_GETENTROPY) + int res; + char *p = buf; + while (len) { + /* getentropy has a maximum size of 256 bytes. */ + size_t chunk = len < 256 ? len : 256; + res = getentropy(p, chunk); + if (res < 0) + return -1; + len -= chunk; + p += chunk; + } + return 0; +#elif defined(HAVE_RTLGENRANDOM) + if (!RtlGenRandom(buf, len)) + return -1; + return 0; +#elif defined(HAVE_OPENSSL_CSPRNG) + int res = RAND_bytes(buf, len); + if (res == 1) + return 0; + if (res == -1) + errno = ENOTSUP; + else + errno = EIO; + return -1; +#else + ssize_t res; + char *p = buf; + int fd, err; + fd = open("/dev/urandom", O_RDONLY); + if (fd < 0) + return -1; + while (len) { + res = xread(fd, p, len); + if (res < 0) { + err = errno; + close(fd); + errno = err; + return -1; + } + len -= res; + p += res; + } + close(fd); + return 0; +#endif +} ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/2] wrapper: add a helper to generate numbers from a CSPRNG 2022-01-17 21:56 ` [PATCH v3 1/2] wrapper: add a helper to generate numbers from " brian m. carlson @ 2022-01-18 9:45 ` Ævar Arnfjörð Bjarmason 2022-01-18 13:31 ` René Scharfe 0 siblings, 1 reply; 23+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-01-18 9:45 UTC (permalink / raw) To: brian m. carlson Cc: git, Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin On Mon, Jan 17 2022, brian m. carlson wrote: > @@ -234,6 +234,14 @@ all:: > # Define NO_TRUSTABLE_FILEMODE if your filesystem may claim to support > # the executable mode bit, but doesn't really do so. > # > +# Define CSPRNG_METHOD to "arc4random" if your system has arc4random and > +# arc4random_buf, "libbsd" if your system has those functions from libbsd, > +# "getrandom" if your system has getrandom, "getentropy" if your system has > +# getentropy, "rtlgenrandom" for RtlGenRandom (Windows only), or "openssl" if > +# you'd want to use the OpenSSL CSPRNG. You may set multiple options with > +# spaces, in which case a suitable option will be chosen. If unset or set to > +# anything else, defaults to using "/dev/urandom". > +# > # Define NEEDS_MODE_TRANSLATION if your OS strays from the typical file type > # bits in mode values (e.g. z/OS defines I_SFMT to 0xFF000000 as opposed to the > # usual 0xF000). > @@ -693,6 +701,7 @@ TEST_BUILTINS_OBJS += test-bloom.o > TEST_BUILTINS_OBJS += test-chmtime.o > TEST_BUILTINS_OBJS += test-config.o > TEST_BUILTINS_OBJS += test-crontab.o > +TEST_BUILTINS_OBJS += test-csprng.o > TEST_BUILTINS_OBJS += test-ctype.o > TEST_BUILTINS_OBJS += test-date.o > TEST_BUILTINS_OBJS += test-delta.o > @@ -1908,6 +1917,31 @@ ifdef HAVE_GETDELIM > BASIC_CFLAGS += -DHAVE_GETDELIM > endif > > +ifneq ($(findstring arc4random,$(CSPRNG_METHOD)),) > + BASIC_CFLAGS += -DHAVE_ARC4RANDOM > +endif > + > +ifneq ($(findstring libbsd,$(CSPRNG_METHOD)),) > + BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD > + EXTLIBS += -lbsd > +endif > + > +ifneq ($(findstring getrandom,$(CSPRNG_METHOD)),) > + BASIC_CFLAGS += -DHAVE_GETRANDOM > +endif > + > +ifneq ($(findstring getentropy,$(CSPRNG_METHOD)),) > + BASIC_CFLAGS += -DHAVE_GETENTROPY > +endif > + > +ifneq ($(findstring rtlgenrandom,$(CSPRNG_METHOD)),) > + BASIC_CFLAGS += -DHAVE_RTLGENRANDOM > +endif > + > +ifneq ($(findstring openssl,$(CSPRNG_METHOD)),) > + BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG > +endif > + Just an implementation nit: I think: ifdef HAVE_CSPRNG_ARC4RANDOM endif ifdef HAVE_CSPRNG_GETRANDOM endif etc. makes this sort of thing much simpler. It piggy-backs on make's default "is defined?" semantics, which avoids a lot of verbosity. The "findstring" function you're using is also designed for one-letter flags like those used for MAKEFLAGS. See /if.*filter/ for a better pattern for space-separated tokens if you'd like to use this "variable takes a space separated list" pattern.... > diff --git a/config.mak.uname b/config.mak.uname > index a3a779327f..ff0710a612 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -141,6 +141,7 @@ ifeq ($(uname_S),Darwin) > HAVE_BSD_SYSCTL = YesPlease > FREAD_READS_DIRECTORIES = UnfortunatelyYes > HAVE_NS_GET_EXECUTABLE_PATH = YesPlease > + CSPRNG_METHOD = arc4random > > # Workaround for `gettext` being keg-only and not even being linked via > # `brew link --force gettext`, should be obsolete as of ..another reason to use the "defined?" pattern is that if you're using an older version of OSX (or one of the other OS's) and this is wrong you can just: HAVE_CSPRNG_WHATEVER = But with space-separated you'll need a more verbose $(filter-out ...). > +/* nit: API comments with "/**" comments. > + * Generate len bytes from the system cryptographically secure PRNG. > + * Returns 0 on success and -1 on error, setting errno. The inability to > + * satisfy the full request is an error. > + */ > +int csprng_bytes(void *buf, size_t len); > + > #endif > diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c > new file mode 100644 > index 0000000000..65d14973c5 > --- /dev/null > +++ b/t/helper/test-csprng.c > @@ -0,0 +1,29 @@ > +#include "test-tool.h" > +#include "git-compat-util.h" > + > + nit: extra line, also git-compat-util.h doesn't need to be included, test-tool.h has it. > +int cmd__csprng(int argc, const char **argv) > +{ > + unsigned long count; > + unsigned char buf[1024]; > + > + if (argc > 2) { > + fprintf(stderr, "usage: %s [<size>]\n", argv[0]); > + return 2; > + } > + > + count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L; > + > + while (count) { > + unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf); > + if (csprng_bytes(buf, chunk) < 0) { > + perror("failed to read"); > + return 5; > + } > + if (fwrite(buf, chunk, 1, stdout) != chunk) > + return 1; > + count -= chunk; > + } > + > + return 0; > +} I know this is just a "demo", but why not add a trivial test *.sh file for whatever low-level wrapper test we have, so we at least know it won't segfault etc. These return codes seem quite specific, any reason we need 2 and 5, not just "return 1" everywhere on error? error_errno() instead of perror()? > +int csprng_bytes(void *buf, size_t len) > +{ > +#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD) > + /* This function never returns an error. */ > + arc4random_buf(buf, len); > + return 0; > +#elif defined(HAVE_GETRANDOM) > + ssize_t res; > + char *p = buf; > + while (len) { > + res = getrandom(p, len, 0); > + if (res < 0) > + return -1; > + len -= res; > + p += res; > + } > + return 0; > +#elif defined(HAVE_GETENTROPY) > + int res; > + char *p = buf; > + while (len) { > + /* getentropy has a maximum size of 256 bytes. */ > + size_t chunk = len < 256 ? len : 256; > + res = getentropy(p, chunk); > + if (res < 0) > + return -1; > + len -= chunk; > + p += chunk; > + } > + return 0; > +#elif defined(HAVE_RTLGENRANDOM) > + if (!RtlGenRandom(buf, len)) > + return -1; > + return 0; > +#elif defined(HAVE_OPENSSL_CSPRNG) > + int res = RAND_bytes(buf, len); > + if (res == 1) > + return 0; > + if (res == -1) > + errno = ENOTSUP; > + else > + errno = EIO; > + return -1; Why fake up errno here instead of just returning -1? In 2/2 you call error_errno(). This seems buggy for a function that doesn't clear errno and doesn't guarantee that it's set on failure.... > +#else > + ssize_t res; > + char *p = buf; > + int fd, err; > + fd = open("/dev/urandom", O_RDONLY); > + if (fd < 0) > + return -1; > + while (len) { > + res = xread(fd, p, len); > + if (res < 0) { > + err = errno; > + close(fd); > + errno = err; > + return -1; > + } > + len -= res; > + p += res; > + } > + close(fd); > + return 0; > +#endif > +} ...seems better to turn it into a "int *failure_errno" parameter instead, or just have the function itself call error_errno() in these cases. You can't just check "if (errno)" either due to the return value of close() not being checked here... ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/2] wrapper: add a helper to generate numbers from a CSPRNG 2022-01-18 9:45 ` Ævar Arnfjörð Bjarmason @ 2022-01-18 13:31 ` René Scharfe 0 siblings, 0 replies; 23+ messages in thread From: René Scharfe @ 2022-01-18 13:31 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, brian m. carlson Cc: git, Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin Am 18.01.22 um 10:45 schrieb Ævar Arnfjörð Bjarmason: > > On Mon, Jan 17 2022, brian m. carlson wrote: > >> +int csprng_bytes(void *buf, size_t len) >> +{ >> +#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD) >> + /* This function never returns an error. */ >> + arc4random_buf(buf, len); >> + return 0; >> +#elif defined(HAVE_GETRANDOM) >> + ssize_t res; >> + char *p = buf; >> + while (len) { >> + res = getrandom(p, len, 0); >> + if (res < 0) >> + return -1; >> + len -= res; >> + p += res; >> + } >> + return 0; >> +#elif defined(HAVE_GETENTROPY) >> + int res; >> + char *p = buf; >> + while (len) { >> + /* getentropy has a maximum size of 256 bytes. */ >> + size_t chunk = len < 256 ? len : 256; >> + res = getentropy(p, chunk); >> + if (res < 0) >> + return -1; >> + len -= chunk; >> + p += chunk; >> + } >> + return 0; >> +#elif defined(HAVE_RTLGENRANDOM) >> + if (!RtlGenRandom(buf, len)) >> + return -1; >> + return 0; >> +#elif defined(HAVE_OPENSSL_CSPRNG) >> + int res = RAND_bytes(buf, len); >> + if (res == 1) >> + return 0; >> + if (res == -1) >> + errno = ENOTSUP; >> + else >> + errno = EIO; >> + return -1; > > Why fake up errno here instead of just returning -1? In 2/2 you call > error_errno(). This seems buggy for a function that doesn't clear errno > and doesn't guarantee that it's set on failure.... Clearing errno is unnecessary as long as it's always set on error and the return code indicates whether to look at it. Translating the return codes of RAND_bytes to suitable errno values makes sense if the goal is to consistently report errors that way on all platforms. arc4random_buf never fails, getrandom and getentropy do set errno, so no translation is needed for them. RAND_bytes doesn't set errno according to [1], and the translation above looks sensible. RtlGenRandom also doesn't set errno according to [2], but a translation is missing above. Should it set errno to EIO in the error case? [1] https://www.openssl.org/docs/manmaster/man3/RAND_bytes.html [2] https://docs.microsoft.com/en-us/windows/win32/api/ntsecapi/nf-ntsecapi-rtlgenrandom > >> +#else >> + ssize_t res; >> + char *p = buf; >> + int fd, err; >> + fd = open("/dev/urandom", O_RDONLY); >> + if (fd < 0) >> + return -1; >> + while (len) { >> + res = xread(fd, p, len); >> + if (res < 0) { >> + err = errno; >> + close(fd); >> + errno = err; >> + return -1; >> + } >> + len -= res; >> + p += res; >> + } >> + close(fd); >> + return 0; >> +#endif >> +} > > ...seems better to turn it into a "int *failure_errno" parameter > instead, or just have the function itself call error_errno() in these > cases. > > You can't just check "if (errno)" either due to the return value of > close() not being checked here... If the last close(2) call fails for some reason then callers of csprng_bytes() won't notice due to the return code being zero, nor do they care -- they got their random data and they cannot do anything about the file descriptor that now hangs in limbo. So this code looks OK to me. René ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names 2022-01-17 21:56 ` [PATCH v3 " brian m. carlson 2022-01-17 21:56 ` [PATCH v3 1/2] wrapper: add a helper to generate numbers from " brian m. carlson @ 2022-01-17 21:56 ` brian m. carlson 2022-01-18 9:24 ` Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 23+ messages in thread From: brian m. carlson @ 2022-01-17 21:56 UTC (permalink / raw) To: git Cc: Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin The current way we generate random file names is by taking the seconds and microseconds, plus the PID, and mixing them together, then encoding them. If this fails, we increment the value by 7777, and try again up to TMP_MAX times. Unfortunately, this is not the best idea from a security perspective. If we're writing into TMPDIR, an attacker can guess these values easily and prevent us from creating any temporary files at all by creating them all first. Even though we set TMP_MAX to 16384, this may be achievable in some contexts, even if unlikely to occur in practice. Fortunately, we can simply solve this by using the system cryptographically secure pseudorandom number generator (CSPRNG) to generate a random 64-bit value, and use that as before. Note that there is still a small bias here, but because a six-character sequence chosen out of 62 characters provides about 36 bits of entropy, the bias here is less than 2^-28, which is acceptable, especially considering we'll retry several times. Note that the use of a CSPRNG in generating temporary file names is also used in many libcs. glibc recently changed from an approach similar to ours to using a CSPRNG, and FreeBSD and OpenBSD also use a CSPRNG in this case. Even if the likelihood of an attack is low, we should still be at least as responsible in creating temporary files as libc is. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- wrapper.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/wrapper.c b/wrapper.c index 1052356703..3258cdb171 100644 --- a/wrapper.c +++ b/wrapper.c @@ -463,8 +463,6 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) static const int num_letters = ARRAY_SIZE(letters) - 1; static const char x_pattern[] = "XXXXXX"; static const int num_x = ARRAY_SIZE(x_pattern) - 1; - uint64_t value; - struct timeval tv; char *filename_template; size_t len; int fd, count; @@ -485,12 +483,13 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) * Replace pattern's XXXXXX characters with randomness. * Try TMP_MAX different filenames. */ - gettimeofday(&tv, NULL); - value = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid(); filename_template = &pattern[len - num_x - suffix_len]; for (count = 0; count < TMP_MAX; ++count) { - uint64_t v = value; int i; + uint64_t v; + if (csprng_bytes(&v, sizeof(v)) < 0) + return error_errno("unable to get random bytes for temporary file"); + /* Fill in the random bits. */ for (i = 0; i < num_x; i++) { filename_template[i] = letters[v % num_letters]; @@ -506,12 +505,6 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) */ if (errno != EEXIST) break; - /* - * This is a random value. It is only necessary that - * the next TMP_MAX values generated by adding 7777 to - * VALUE are different with (module 2^32). - */ - value += 7777; } /* We return the null string if we can't find a unique file name. */ pattern[0] = '\0'; ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names 2022-01-17 21:56 ` [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names brian m. carlson @ 2022-01-18 9:24 ` Ævar Arnfjörð Bjarmason 2022-01-18 14:42 ` René Scharfe 2022-01-19 3:28 ` brian m. carlson 0 siblings, 2 replies; 23+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-01-18 9:24 UTC (permalink / raw) To: brian m. carlson Cc: git, Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin On Mon, Jan 17 2022, brian m. carlson wrote: > The current way we generate random file names is by taking the seconds > and microseconds, plus the PID, and mixing them together, then encoding > them. If this fails, we increment the value by 7777, and try again up > to TMP_MAX times. > > Unfortunately, this is not the best idea from a security perspective. > If we're writing into TMPDIR, an attacker can guess these values easily > and prevent us from creating any temporary files at all by creating them > all first. Even though we set TMP_MAX to 16384, this may be achievable > in some contexts, even if unlikely to occur in practice. > [...] I had a comment on v1[1] of this series which still applies here, i.e. the "if we're writing into TMPDIR[...]" here elides the fact that much of the time we're writing a tempfile into .git, so the security issue ostensibly being solved here won't be a practical issue at all. Then for out-of-repo tempfiles some OS's have a per-user $TEMPDIR you can use (e.g. systemd's /run/user/`id -u`). Finally... > Note that the use of a CSPRNG in generating temporary file names is also > used in many libcs. glibc recently changed from an approach similar to > ours to using a CSPRNG, and FreeBSD and OpenBSD also use a CSPRNG in > this case. Even if the likelihood of an attack is low, we should still > be at least as responsible in creating temporary files as libc is. ...we already have in-tree users of mkstemp(), which on glibc ostensibly tries to solve the same security issues you note here, and the reftable/* user has been added since earlier iterations of this series: o $ git grep -E '\bmkstemp\(' -- '*.[ch]' compat/mingw.c:int mkstemp(char *template) compat/mingw.h:int mkstemp(char *template); entry.c: return mkstemp(path); reftable/stack.c: tab_fd = mkstemp(temp_tab_file_name.buf); reftable/stack.c: tab_fd = mkstemp(temp_tab->buf); reftable/stack_test.c: int fd = mkstemp(fn); wrapper.c: fd = mkstemp(filename_template); This series really feels like it's adding too much complexity and potential auditing headaches (distributors worrying about us shipping a CSPRNG, having to audit it) to a low-level codepath that most of the time won't need this at all. So instead of: A. Add CSPRNG with demo test helper B. Use it in git_mkstemps_mode() I'd think we'd be much better off with: A. Split out callers of tempfile.c and mk.*temp in wrapper.c that create tempfiles in .git B. <the rest> I honestly haven't looked too much at what <the rest> is, other than what I wrote in [1], which seems to suggest that most of our codepaths won't need this. I'd also think that given the reference to CSPRNG in e.g. some glibc versions that instead of the ifdefs in csprng_bytes() we should instead directly use a secure mkstemp() (or similar) for the not-.git cases that remain after the "mktemp in a dir we chown" v.s. "mktemp in shared /tmp" are split up. Maybe these are all things you looked at and considered, but from my recollection (I didn't go back and re-read the whole discussion) you didn't chime in on this point, so *bump* :) 1. https://lore.kernel.org/git/211116.864k8bq0xm.gmgdl@evledraar.gmail.com/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names 2022-01-18 9:24 ` Ævar Arnfjörð Bjarmason @ 2022-01-18 14:42 ` René Scharfe 2022-01-18 14:51 ` Ævar Arnfjörð Bjarmason 2022-01-18 17:25 ` Junio C Hamano 2022-01-19 3:28 ` brian m. carlson 1 sibling, 2 replies; 23+ messages in thread From: René Scharfe @ 2022-01-18 14:42 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, brian m. carlson Cc: git, Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin Am 18.01.22 um 10:24 schrieb Ævar Arnfjörð Bjarmason: > > On Mon, Jan 17 2022, brian m. carlson wrote: > >> The current way we generate random file names is by taking the seconds >> and microseconds, plus the PID, and mixing them together, then encoding >> them. If this fails, we increment the value by 7777, and try again up >> to TMP_MAX times. >> >> Unfortunately, this is not the best idea from a security perspective. >> If we're writing into TMPDIR, an attacker can guess these values easily >> and prevent us from creating any temporary files at all by creating them >> all first. Even though we set TMP_MAX to 16384, this may be achievable >> in some contexts, even if unlikely to occur in practice. >> [...] > > I had a comment on v1[1] of this series which still applies here, > i.e. the "if we're writing into TMPDIR[...]" here elides the fact that > much of the time we're writing a tempfile into .git, so the security > issue ostensibly being solved here won't be a practical issue at all. > > Then for out-of-repo tempfiles some OS's have a per-user $TEMPDIR you > can use (e.g. systemd's /run/user/`id -u`). Finally... > >> Note that the use of a CSPRNG in generating temporary file names is also >> used in many libcs. glibc recently changed from an approach similar to >> ours to using a CSPRNG, and FreeBSD and OpenBSD also use a CSPRNG in >> this case. Even if the likelihood of an attack is low, we should still >> be at least as responsible in creating temporary files as libc is. > > ...we already have in-tree users of mkstemp(), which on glibc ostensibly > tries to solve the same security issues you note here, and the > reftable/* user has been added since earlier iterations of this series: > o > $ git grep -E '\bmkstemp\(' -- '*.[ch]' > compat/mingw.c:int mkstemp(char *template) > compat/mingw.h:int mkstemp(char *template); > entry.c: return mkstemp(path); > reftable/stack.c: tab_fd = mkstemp(temp_tab_file_name.buf); > reftable/stack.c: tab_fd = mkstemp(temp_tab->buf); > reftable/stack_test.c: int fd = mkstemp(fn); > wrapper.c: fd = mkstemp(filename_template); > > This series really feels like it's adding too much complexity and > potential auditing headaches (distributors worrying about us shipping a > CSPRNG, having to audit it) to a low-level codepath that most of the > time won't need this at all. Good point. Please let me think out loud for a moment. mkstemp() is secure (right?) and used already. mkstemps() was used as well until b2d593a779 (wrapper.c: remove unused gitmkstemps() function, 2017-02-28). What's the difference? The former requires the random characters at the end (e.g. "name-XXXXXX"), while the latter allows a suffix (e.g. "name-XXXXXX.ext"); that's what the added "s" in the name means, I guess. And apparently mkstemp is everywhere, but mkstemps is a GNU extension. git_mkstemps_mode is used by mks_tempfile_sm, mks_tempfile_tsm and git_mkstemp_mode. The latter uses no suffix, so could be implemented using mkstemp and fchmod instead. mks_tempfile_sm is called by write_locked_index, mks_tempfile_s, mks_tempfile_m and mks_tempfile. Only mks_tempfile_s uses a suffix, but is itself unused. So an implementation based on mkstemp and fchmod would suffice for mks_tempfile_sm users as well. mks_tempfile_tsm is used by mks_tempfile_ts, mks_tempfile_tm and mks_tempfile_t. Only mks_tempfile_ts uses a suffix. Its only caller is diff.c::prep_temp_blob, called only by diff.c::prepare_temp_file, called by add_external_diff_name and run_textconv in the same file. So if I didn't take a wrong turn somewhere the only temporary file name templates with suffixes are those for external diffs and textconv filters. The rest of the git_mkstemps_mode users could be switched to mkstemp+fchmod. Temporary files for external diffs and textconv filters *should* be placed in $TMPDIR. Do they need suffixes? I guess for testconv filters it doesn't matter, but graphical diff viewers might do syntax highlighting based on the extension. Can we get extensions without mktemps or git_mkstemps_mode? Yes, by using mkdtemp to create a temporary directory with a random name and creating the files in it. There already are mkdtemp users. So AFAICS all use cases of git_mkstemps_mode can be served by mkstemp+fchmod or mkdtemp. Would that help? > So instead of: > > A. Add CSPRNG with demo test helper > B. Use it in git_mkstemps_mode() > > I'd think we'd be much better off with: > > A. Split out callers of tempfile.c and mk.*temp in wrapper.c that create tempfiles in .git > B. <the rest> > > I honestly haven't looked too much at what <the rest> is, other than > what I wrote in [1], which seems to suggest that most of our codepaths > won't need this. > > I'd also think that given the reference to CSPRNG in e.g. some glibc > versions that instead of the ifdefs in csprng_bytes() we should instead > directly use a secure mkstemp() (or similar) for the not-.git cases that > remain after the "mktemp in a dir we chown" v.s. "mktemp in shared /tmp" > are split up. > > Maybe these are all things you looked at and considered, but from my > recollection (I didn't go back and re-read the whole discussion) you > didn't chime in on this point, so *bump* :) > > 1. https://lore.kernel.org/git/211116.864k8bq0xm.gmgdl@evledraar.gmail.com/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names 2022-01-18 14:42 ` René Scharfe @ 2022-01-18 14:51 ` Ævar Arnfjörð Bjarmason 2022-01-18 16:44 ` René Scharfe 2022-01-18 17:25 ` Junio C Hamano 1 sibling, 1 reply; 23+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-01-18 14:51 UTC (permalink / raw) To: René Scharfe Cc: brian m. carlson, git, Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin On Tue, Jan 18 2022, René Scharfe wrote: > Am 18.01.22 um 10:24 schrieb Ævar Arnfjörð Bjarmason: >> >> On Mon, Jan 17 2022, brian m. carlson wrote: >> >>> The current way we generate random file names is by taking the seconds >>> and microseconds, plus the PID, and mixing them together, then encoding >>> them. If this fails, we increment the value by 7777, and try again up >>> to TMP_MAX times. >>> >>> Unfortunately, this is not the best idea from a security perspective. >>> If we're writing into TMPDIR, an attacker can guess these values easily >>> and prevent us from creating any temporary files at all by creating them >>> all first. Even though we set TMP_MAX to 16384, this may be achievable >>> in some contexts, even if unlikely to occur in practice. >>> [...] >> >> I had a comment on v1[1] of this series which still applies here, >> i.e. the "if we're writing into TMPDIR[...]" here elides the fact that >> much of the time we're writing a tempfile into .git, so the security >> issue ostensibly being solved here won't be a practical issue at all. >> >> Then for out-of-repo tempfiles some OS's have a per-user $TEMPDIR you >> can use (e.g. systemd's /run/user/`id -u`). Finally... >> >>> Note that the use of a CSPRNG in generating temporary file names is also >>> used in many libcs. glibc recently changed from an approach similar to >>> ours to using a CSPRNG, and FreeBSD and OpenBSD also use a CSPRNG in >>> this case. Even if the likelihood of an attack is low, we should still >>> be at least as responsible in creating temporary files as libc is. >> >> ...we already have in-tree users of mkstemp(), which on glibc ostensibly >> tries to solve the same security issues you note here, and the >> reftable/* user has been added since earlier iterations of this series: >> o >> $ git grep -E '\bmkstemp\(' -- '*.[ch]' >> compat/mingw.c:int mkstemp(char *template) >> compat/mingw.h:int mkstemp(char *template); >> entry.c: return mkstemp(path); >> reftable/stack.c: tab_fd = mkstemp(temp_tab_file_name.buf); >> reftable/stack.c: tab_fd = mkstemp(temp_tab->buf); >> reftable/stack_test.c: int fd = mkstemp(fn); >> wrapper.c: fd = mkstemp(filename_template); >> >> This series really feels like it's adding too much complexity and >> potential auditing headaches (distributors worrying about us shipping a >> CSPRNG, having to audit it) to a low-level codepath that most of the >> time won't need this at all. > > Good point. Please let me think out loud for a moment. > > mkstemp() is secure (right?) and used already. mkstemps() was used as > well until b2d593a779 (wrapper.c: remove unused gitmkstemps() function, > 2017-02-28). What's the difference? The former requires the random > characters at the end (e.g. "name-XXXXXX"), while the latter allows a > suffix (e.g. "name-XXXXXX.ext"); that's what the added "s" in the name > means, I guess. And apparently mkstemp is everywhere, but mkstemps is > a GNU extension. > > git_mkstemps_mode is used by mks_tempfile_sm, mks_tempfile_tsm and > git_mkstemp_mode. The latter uses no suffix, so could be implemented > using mkstemp and fchmod instead. > > mks_tempfile_sm is called by write_locked_index, mks_tempfile_s, > mks_tempfile_m and mks_tempfile. Only mks_tempfile_s uses a suffix, > but is itself unused. So an implementation based on mkstemp and fchmod > would suffice for mks_tempfile_sm users as well. > > mks_tempfile_tsm is used by mks_tempfile_ts, mks_tempfile_tm and > mks_tempfile_t. Only mks_tempfile_ts uses a suffix. Its only caller > is diff.c::prep_temp_blob, called only by diff.c::prepare_temp_file, > called by add_external_diff_name and run_textconv in the same file. > > So if I didn't take a wrong turn somewhere the only temporary file name > templates with suffixes are those for external diffs and textconv > filters. The rest of the git_mkstemps_mode users could be switched to > mkstemp+fchmod. > > Temporary files for external diffs and textconv filters *should* be > placed in $TMPDIR. Do they need suffixes? I guess for testconv > filters it doesn't matter, but graphical diff viewers might do syntax > highlighting based on the extension. > > Can we get extensions without mktemps or git_mkstemps_mode? Yes, by > using mkdtemp to create a temporary directory with a random name and > creating the files in it. There already are mkdtemp users. > > So AFAICS all use cases of git_mkstemps_mode can be served by > mkstemp+fchmod or mkdtemp. Would that help? That seems sensible, as a further practical suggestion it seems like we'll retry around 16k times to create these files on failure, perhaps trying with a custom extension 8k times would be OK, followed by the minor UI degradation of doing the final 8k retries with the more-random OS-provided extension-less variant. More generally I'm still not sure if this is still a purely hypothetical attack mitigation, or whether there are actually users out there that have to deal with this. Wouldn't something like this also be an acceptable "solution" to this class of problem? #define TMP_MAX 1024 [...] if (count >= TMP_MAX) die(_("we tried and failed to create a tempfile using the '%s' template after %d tries!\n\n" "Is someone actively DoS you? Is your sysadmin known to be your mortal enemy?\n" "are you using Satan's shared hosting services? In any case, we give up!\n\n" "To \"retry\" set TEMPDIR to some location where other users won't race us to death"), template, count); For a bonus grade we could add a few more lines and try to stat() some of the files we failed to create, and report what UID it is that's racing you for all of these tempfile creations. >> So instead of: >> >> A. Add CSPRNG with demo test helper >> B. Use it in git_mkstemps_mode() >> >> I'd think we'd be much better off with: >> >> A. Split out callers of tempfile.c and mk.*temp in wrapper.c that create tempfiles in .git >> B. <the rest> >> >> I honestly haven't looked too much at what <the rest> is, other than >> what I wrote in [1], which seems to suggest that most of our codepaths >> won't need this. >> >> I'd also think that given the reference to CSPRNG in e.g. some glibc >> versions that instead of the ifdefs in csprng_bytes() we should instead >> directly use a secure mkstemp() (or similar) for the not-.git cases that >> remain after the "mktemp in a dir we chown" v.s. "mktemp in shared /tmp" >> are split up. >> >> Maybe these are all things you looked at and considered, but from my >> recollection (I didn't go back and re-read the whole discussion) you >> didn't chime in on this point, so *bump* :) >> >> 1. https://lore.kernel.org/git/211116.864k8bq0xm.gmgdl@evledraar.gmail.com/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names 2022-01-18 14:51 ` Ævar Arnfjörð Bjarmason @ 2022-01-18 16:44 ` René Scharfe 0 siblings, 0 replies; 23+ messages in thread From: René Scharfe @ 2022-01-18 16:44 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: brian m. carlson, git, Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin Am 18.01.22 um 15:51 schrieb Ævar Arnfjörð Bjarmason: > > On Tue, Jan 18 2022, René Scharfe wrote: > >> Am 18.01.22 um 10:24 schrieb Ævar Arnfjörð Bjarmason: >>> >>> On Mon, Jan 17 2022, brian m. carlson wrote: >>> >>>> The current way we generate random file names is by taking the seconds >>>> and microseconds, plus the PID, and mixing them together, then encoding >>>> them. If this fails, we increment the value by 7777, and try again up >>>> to TMP_MAX times. >>>> >>>> Unfortunately, this is not the best idea from a security perspective. >>>> If we're writing into TMPDIR, an attacker can guess these values easily >>>> and prevent us from creating any temporary files at all by creating them >>>> all first. Even though we set TMP_MAX to 16384, this may be achievable >>>> in some contexts, even if unlikely to occur in practice. >>>> [...] >>> >>> I had a comment on v1[1] of this series which still applies here, >>> i.e. the "if we're writing into TMPDIR[...]" here elides the fact that >>> much of the time we're writing a tempfile into .git, so the security >>> issue ostensibly being solved here won't be a practical issue at all. >>> >>> Then for out-of-repo tempfiles some OS's have a per-user $TEMPDIR you >>> can use (e.g. systemd's /run/user/`id -u`). Finally... >>> >>>> Note that the use of a CSPRNG in generating temporary file names is also >>>> used in many libcs. glibc recently changed from an approach similar to >>>> ours to using a CSPRNG, and FreeBSD and OpenBSD also use a CSPRNG in >>>> this case. Even if the likelihood of an attack is low, we should still >>>> be at least as responsible in creating temporary files as libc is. >>> >>> ...we already have in-tree users of mkstemp(), which on glibc ostensibly >>> tries to solve the same security issues you note here, and the >>> reftable/* user has been added since earlier iterations of this series: >>> o >>> $ git grep -E '\bmkstemp\(' -- '*.[ch]' >>> compat/mingw.c:int mkstemp(char *template) >>> compat/mingw.h:int mkstemp(char *template); >>> entry.c: return mkstemp(path); >>> reftable/stack.c: tab_fd = mkstemp(temp_tab_file_name.buf); >>> reftable/stack.c: tab_fd = mkstemp(temp_tab->buf); >>> reftable/stack_test.c: int fd = mkstemp(fn); >>> wrapper.c: fd = mkstemp(filename_template); >>> >>> This series really feels like it's adding too much complexity and >>> potential auditing headaches (distributors worrying about us shipping a >>> CSPRNG, having to audit it) to a low-level codepath that most of the >>> time won't need this at all. >> >> Good point. Please let me think out loud for a moment. >> >> mkstemp() is secure (right?) and used already. mkstemps() was used as >> well until b2d593a779 (wrapper.c: remove unused gitmkstemps() function, >> 2017-02-28). What's the difference? The former requires the random >> characters at the end (e.g. "name-XXXXXX"), while the latter allows a >> suffix (e.g. "name-XXXXXX.ext"); that's what the added "s" in the name >> means, I guess. And apparently mkstemp is everywhere, but mkstemps is >> a GNU extension. >> >> git_mkstemps_mode is used by mks_tempfile_sm, mks_tempfile_tsm and >> git_mkstemp_mode. The latter uses no suffix, so could be implemented >> using mkstemp and fchmod instead. >> >> mks_tempfile_sm is called by write_locked_index, mks_tempfile_s, >> mks_tempfile_m and mks_tempfile. Only mks_tempfile_s uses a suffix, >> but is itself unused. So an implementation based on mkstemp and fchmod >> would suffice for mks_tempfile_sm users as well. >> >> mks_tempfile_tsm is used by mks_tempfile_ts, mks_tempfile_tm and >> mks_tempfile_t. Only mks_tempfile_ts uses a suffix. Its only caller >> is diff.c::prep_temp_blob, called only by diff.c::prepare_temp_file, >> called by add_external_diff_name and run_textconv in the same file. >> >> So if I didn't take a wrong turn somewhere the only temporary file name >> templates with suffixes are those for external diffs and textconv >> filters. The rest of the git_mkstemps_mode users could be switched to >> mkstemp+fchmod. >> >> Temporary files for external diffs and textconv filters *should* be >> placed in $TMPDIR. Do they need suffixes? I guess for testconv >> filters it doesn't matter, but graphical diff viewers might do syntax >> highlighting based on the extension. >> >> Can we get extensions without mktemps or git_mkstemps_mode? Yes, by >> using mkdtemp to create a temporary directory with a random name and >> creating the files in it. There already are mkdtemp users. >> >> So AFAICS all use cases of git_mkstemps_mode can be served by >> mkstemp+fchmod or mkdtemp. Would that help? > > That seems sensible, as a further practical suggestion it seems like > we'll retry around 16k times to create these files on failure, perhaps > trying with a custom extension 8k times would be OK, followed by the > minor UI degradation of doing the final 8k retries with the more-random > OS-provided extension-less variant. You can't use the suffix-less mkstemp if a suffix is necessary. Retries would be handled by mkstemp and mkdtemp IIUC. To an extent. E.g. https://cgit.freebsd.org/src/tree/lib/libc/stdio/mktemp.c seems to just count up, which doesn't help if an attacker guessed the first name correctly. So maybe some kind of EEXIST loop is still necessary. > More generally I'm still not sure if this is still a purely hypothetical > attack mitigation, or whether there are actually users out there that > have to deal with this. > > Wouldn't something like this also be an acceptable "solution" to this > class of problem? > > #define TMP_MAX 1024 > [...] > if (count >= TMP_MAX) > die(_("we tried and failed to create a tempfile using the '%s' template after %d tries!\n\n" > "Is someone actively DoS you? Is your sysadmin known to be your mortal enemy?\n" > "are you using Satan's shared hosting services? In any case, we give up!\n\n" > "To \"retry\" set TEMPDIR to some location where other users won't race us to death"), > template, count); You mean users should be educated to stay away from shared temporary directories on multi-user systems? Good advice. I'm also not sure how relevant it is these days -- doesn't everybody get their own VM these days? :) Anyway, there are probably some who cannot follow it, e.g. because their $HOME quota is exhausted. > For a bonus grade we could add a few more lines and try to stat() some > of the files we failed to create, and report what UID it is that's > racing you for all of these tempfile creations. Sounds fun, can enliven the office. Once the fisticuffs are over -- PLOT TWIST! Turns out the RNG handed out the same "random" numbers to everyone. ;) > >>> So instead of: >>> >>> A. Add CSPRNG with demo test helper >>> B. Use it in git_mkstemps_mode() >>> >>> I'd think we'd be much better off with: >>> >>> A. Split out callers of tempfile.c and mk.*temp in wrapper.c that create tempfiles in .git >>> B. <the rest> >>> >>> I honestly haven't looked too much at what <the rest> is, other than >>> what I wrote in [1], which seems to suggest that most of our codepaths >>> won't need this. >>> >>> I'd also think that given the reference to CSPRNG in e.g. some glibc >>> versions that instead of the ifdefs in csprng_bytes() we should instead >>> directly use a secure mkstemp() (or similar) for the not-.git cases that >>> remain after the "mktemp in a dir we chown" v.s. "mktemp in shared /tmp" >>> are split up. >>> >>> Maybe these are all things you looked at and considered, but from my >>> recollection (I didn't go back and re-read the whole discussion) you >>> didn't chime in on this point, so *bump* :) >>> >>> 1. https://lore.kernel.org/git/211116.864k8bq0xm.gmgdl@evledraar.gmail.com/ > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names 2022-01-18 14:42 ` René Scharfe 2022-01-18 14:51 ` Ævar Arnfjörð Bjarmason @ 2022-01-18 17:25 ` Junio C Hamano 2022-01-19 17:49 ` René Scharfe 2022-01-22 10:41 ` René Scharfe 1 sibling, 2 replies; 23+ messages in thread From: Junio C Hamano @ 2022-01-18 17:25 UTC (permalink / raw) To: René Scharfe Cc: Ævar Arnfjörð Bjarmason, brian m. carlson, git, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin René Scharfe <l.s.r@web.de> writes: >> This series really feels like it's adding too much complexity and >> potential auditing headaches (distributors worrying about us shipping a >> CSPRNG, having to audit it) to a low-level codepath that most of the >> time won't need this at all. > > Good point. Please let me think out loud for a moment. Yeah, I agree you and Ævar that the topic may be over-engineering the solution for problem that we shouldn't be the ones who solve. I agree with your analysis that the "diff" tempfiles do need suffix, we SHOULD create them in $TMPDIR (not in the working tree or $GIT_DIR) to support operation in a read-only repository, but we can create a unique temporary directory and place a file (even under its original name) in it as a workaround. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names 2022-01-18 17:25 ` Junio C Hamano @ 2022-01-19 17:49 ` René Scharfe 2022-01-22 10:41 ` René Scharfe 1 sibling, 0 replies; 23+ messages in thread From: René Scharfe @ 2022-01-19 17:49 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, brian m. carlson, git, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin Am 18.01.22 um 18:25 schrieb Junio C Hamano: > I agree with your analysis that the "diff" tempfiles do need suffix, > we SHOULD create them in $TMPDIR (not in the working tree or > $GIT_DIR) to support operation in a read-only repository, but we can > create a unique temporary directory and place a file (even under its > original name) in it as a workaround. Here's a proof-of-concept patch for handling just that diff use-case using mkdtemp. Indeed it's nice to see the actual filename with difftool. diff.c | 4 ++-- t/t4020-diff-external.sh | 2 +- tempfile.c | 20 ++++++++++++++++++++ tempfile.h | 1 + wrapper.c | 12 ++++++++++++ 5 files changed, 36 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index c862771a58..37db4743b0 100644 --- a/diff.c +++ b/diff.c @@ -4098,8 +4098,8 @@ static void prep_temp_blob(struct index_state *istate, init_checkout_metadata(&meta, NULL, NULL, oid); - /* Generate "XXXXXX_basename.ext" */ - strbuf_addstr(&tempfile, "XXXXXX_"); + /* Generate "git-blob-XXXXXX/basename.ext" */ + strbuf_addstr(&tempfile, "git-blob-XXXXXX/"); strbuf_addstr(&tempfile, base); temp->tempfile = mks_tempfile_ts(tempfile.buf, strlen(base) + 1); diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh index 54bb8ef27e..e7f93f36f5 100755 --- a/t/t4020-diff-external.sh +++ b/t/t4020-diff-external.sh @@ -217,7 +217,7 @@ test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths' ' touch file.ext && git add file.ext && echo with extension > file.ext && - GIT_EXTERNAL_DIFF=echo git diff file.ext | grep ......_file\.ext && + GIT_EXTERNAL_DIFF=echo git diff file.ext | grep git-blob-....../file\.ext && git update-index --force-remove file.ext && rm file.ext ' diff --git a/tempfile.c b/tempfile.c index 94aa18f3f7..80cc9fb512 100644 --- a/tempfile.c +++ b/tempfile.c @@ -56,6 +56,21 @@ static VOLATILE_LIST_HEAD(tempfile_list); +static void remove_template_directory(struct tempfile *tempfile, + int in_signal_handler) +{ + char *end = tempfile->filename.buf + tempfile->filename.len; + char *suffix = end - tempfile->suffixlen; + if (*suffix != '/') + return; + *suffix = '\0'; + if (in_signal_handler) + rmdir(tempfile->filename.buf); + else + rmdir_or_warn(tempfile->filename.buf); + *suffix = '/'; +} + static void remove_tempfiles(int in_signal_handler) { pid_t me = getpid(); @@ -74,6 +89,7 @@ static void remove_tempfiles(int in_signal_handler) unlink(p->filename.buf); else unlink_or_warn(p->filename.buf); + remove_template_directory(p, in_signal_handler); p->active = 0; } @@ -100,6 +116,7 @@ static struct tempfile *new_tempfile(void) tempfile->owner = 0; INIT_LIST_HEAD(&tempfile->list); strbuf_init(&tempfile->filename, 0); + tempfile->suffixlen = 0; return tempfile; } @@ -170,6 +187,7 @@ struct tempfile *mks_tempfile_sm(const char *filename_template, int suffixlen, i struct tempfile *tempfile = new_tempfile(); strbuf_add_absolute_path(&tempfile->filename, filename_template); + tempfile->suffixlen = suffixlen; tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode); if (tempfile->fd < 0) { deactivate_tempfile(tempfile); @@ -189,6 +207,7 @@ struct tempfile *mks_tempfile_tsm(const char *filename_template, int suffixlen, tmpdir = "/tmp"; strbuf_addf(&tempfile->filename, "%s/%s", tmpdir, filename_template); + tempfile->suffixlen = suffixlen; tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode); if (tempfile->fd < 0) { deactivate_tempfile(tempfile); @@ -316,6 +335,7 @@ void delete_tempfile(struct tempfile **tempfile_p) close_tempfile_gently(tempfile); unlink_or_warn(tempfile->filename.buf); + remove_template_directory(tempfile, 0); deactivate_tempfile(tempfile); *tempfile_p = NULL; } diff --git a/tempfile.h b/tempfile.h index 4de3bc77d2..b9a60f2431 100644 --- a/tempfile.h +++ b/tempfile.h @@ -82,6 +82,7 @@ struct tempfile { FILE *volatile fp; volatile pid_t owner; struct strbuf filename; + size_t suffixlen; }; /* diff --git a/wrapper.c b/wrapper.c index 36e12119d7..358db282f9 100644 --- a/wrapper.c +++ b/wrapper.c @@ -481,6 +481,18 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) return -1; } + if (pattern[len - suffix_len] == '/') { + if (mode != 0600) { + errno = EINVAL; + return -1; + } + pattern[len - suffix_len] = '\0'; + if (!mkdtemp(pattern)) + return -1; + pattern[len - suffix_len] = '/'; + return open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); + } + /* * Replace pattern's XXXXXX characters with randomness. * Try TMP_MAX different filenames. ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names 2022-01-18 17:25 ` Junio C Hamano 2022-01-19 17:49 ` René Scharfe @ 2022-01-22 10:41 ` René Scharfe 2022-01-24 17:08 ` Junio C Hamano 1 sibling, 1 reply; 23+ messages in thread From: René Scharfe @ 2022-01-22 10:41 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, brian m. carlson, git, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin Am 18.01.22 um 18:25 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >>> This series really feels like it's adding too much complexity and >>> potential auditing headaches (distributors worrying about us shipping a >>> CSPRNG, having to audit it) to a low-level codepath that most of the >>> time won't need this at all. >> >> Good point. Please let me think out loud for a moment. > > Yeah, I agree you and Ævar that the topic may be over-engineering > the solution for problem that we shouldn't be the ones who solve. > > I agree with your analysis that the "diff" tempfiles do need suffix, > we SHOULD create them in $TMPDIR (not in the working tree or > $GIT_DIR) to support operation in a read-only repository, but we can > create a unique temporary directory and place a file (even under its > original name) in it as a workaround. I forgot one crucial aspect, though: umask. The "m" variants of the tempfile functions set a mode, with umask applied. git_mkstemps_mode() does that by passing the mode to open(2), which applies the umask internally. Neither mkstemp(3) nor chmod(2) do that for us, so a replacement needs to call umask(2) to get it and again to restore it, which requires two system calls and is racy if multiple threads do this. Diff doesn't need a custom mode, so we can still use mkdtemp() there and thus make the suffix feature of git_mkstemps_mode() unnecessary. But a replacement for git_mkstemp_mode() with two umask(2) calls looks less attractive to me than fortifying git_mkstemps_mode() with a good source of randomness. René ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names 2022-01-22 10:41 ` René Scharfe @ 2022-01-24 17:08 ` Junio C Hamano 0 siblings, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2022-01-24 17:08 UTC (permalink / raw) To: René Scharfe Cc: Ævar Arnfjörð Bjarmason, brian m. carlson, git, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin René Scharfe <l.s.r@web.de> writes: > ... But > a replacement for git_mkstemp_mode() with two umask(2) calls looks less > attractive to me than fortifying git_mkstemps_mode() with a good source > of randomness. True. Also, it is not like we are supplying our own implementation of random source, but are just pluggig various system-supplied random source into our code, so I do not see the "auditatiblity" problem we heard earlier too much of an issue. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names 2022-01-18 9:24 ` Ævar Arnfjörð Bjarmason 2022-01-18 14:42 ` René Scharfe @ 2022-01-19 3:28 ` brian m. carlson 1 sibling, 0 replies; 23+ messages in thread From: brian m. carlson @ 2022-01-19 3:28 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, rsbecker, Taylor Blau, Carlo Marcelo Arenas Belón, Johannes Schindelin [-- Attachment #1: Type: text/plain, Size: 5057 bytes --] On 2022-01-18 at 09:24:58, Ævar Arnfjörð Bjarmason wrote: > I had a comment on v1[1] of this series which still applies here, > i.e. the "if we're writing into TMPDIR[...]" here elides the fact that > much of the time we're writing a tempfile into .git, so the security > issue ostensibly being solved here won't be a practical issue at all. > > Then for out-of-repo tempfiles some OS's have a per-user $TEMPDIR you > can use (e.g. systemd's /run/user/`id -u`). Finally... Some OSes do have that, but it is not universal and we can't rely on environment variables being set. They are stripped out by some programs, including Homebrew, even on OSes where they're provided. /run/user is also not a suitable temporary directory on Linux. Temporary files can be large, and /run is almost always a tmpfs, which means it sits entirely in memory. Setting anything in /run as TMPDIR is a mistake. > ...we already have in-tree users of mkstemp(), which on glibc ostensibly > tries to solve the same security issues you note here, and the > reftable/* user has been added since earlier iterations of this series: > o > $ git grep -E '\bmkstemp\(' -- '*.[ch]' > compat/mingw.c:int mkstemp(char *template) > compat/mingw.h:int mkstemp(char *template); > entry.c: return mkstemp(path); > reftable/stack.c: tab_fd = mkstemp(temp_tab_file_name.buf); > reftable/stack.c: tab_fd = mkstemp(temp_tab->buf); > reftable/stack_test.c: int fd = mkstemp(fn); > wrapper.c: fd = mkstemp(filename_template); > > This series really feels like it's adding too much complexity and > potential auditing headaches (distributors worrying about us shipping a > CSPRNG, having to audit it) to a low-level codepath that most of the > time won't need this at all. Every Rust program or Go program includes code to call a CSPRNG because it's required to avoid hash collision DoS attacks. I do plan to look into that in the future, because my guess right now is that we are in fact vulnerable to DoS attacks if someone creates crafted object IDs.[0] When I do that, I'll need this code. I also don't think adding code for a CSPRNG is an auditing problem. We use the system CSPRNG, which is the thing that literally everybody should be doing if they need good quality random numbers. If we were shipping a custom CSPRNG, then that would be an auditing problem, but I am explicitly not doing that because it's not necessary here and I'm willing to insist that the system provide one somewhere so we don't have to. > So instead of: > > A. Add CSPRNG with demo test helper > B. Use it in git_mkstemps_mode() > > I'd think we'd be much better off with: > > A. Split out callers of tempfile.c and mk.*temp in wrapper.c that create tempfiles in .git > B. <the rest> > > I honestly haven't looked too much at what <the rest> is, other than > what I wrote in [1], which seems to suggest that most of our codepaths > won't need this. > > I'd also think that given the reference to CSPRNG in e.g. some glibc > versions that instead of the ifdefs in csprng_bytes() we should instead > directly use a secure mkstemp() (or similar) for the not-.git cases that > remain after the "mktemp in a dir we chown" v.s. "mktemp in shared /tmp" > are split up. > > Maybe these are all things you looked at and considered, but from my > recollection (I didn't go back and re-read the whole discussion) you > didn't chime in on this point, so *bump* :) I did explain it in the cover letter for v2, along with the explanation in the paragraph above. The situation is that mkstemp doesn't handle all our use cases, and Randall said in the followups to v1 that mkstemp is not available on NonStop. I therefore must conclude that we'll need to implement this somewhere, even if only as a fallback. If we think mkstemp is going to work here and we don't need this, then I'll drop this series. However, I am annoyed that I'm getting conflicting information about what code is portable on different platforms, which is made especially difficult by trying to support Unix systems that don't support a 21-year-old standard and which lack suitable public documentation. If I write and polish a series based on a set of information I've been given and then it turns out that we decide to do something completely different based on conflicting information, that's not a motivator to continue sending patches. This will not be the first time I've dropped a series after several rounds of review because we totally decided to change course and do something different, and I don't want to repeat this again. [0] This type of attack is extremely trivial because the number of collisions necessary for this attack is usually on the order of 2^16, which means the work can be done in a couple seconds on a typical system. I have a proof of concept of this for PHP online. -- brian m. carlson (he/him or they/them) Toronto, Ontario, CA [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 262 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2022-01-24 17:08 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-04 1:55 [PATCH v2 0/2] Generate temporary files using a CSPRNG brian m. carlson 2022-01-04 1:55 ` [PATCH v2 1/2] wrapper: add a helper to generate numbers from " brian m. carlson 2022-01-04 21:01 ` Junio C Hamano 2022-01-04 21:39 ` Junio C Hamano 2022-01-04 23:12 ` brian m. carlson 2022-01-04 22:56 ` brian m. carlson 2022-01-04 23:17 ` Junio C Hamano 2022-01-04 1:55 ` [PATCH v2 2/2] wrapper: use a CSPRNG to generate random file names brian m. carlson 2022-01-04 12:44 ` [PATCH v2 0/2] Generate temporary files using a CSPRNG Johannes Schindelin 2022-01-17 21:56 ` [PATCH v3 " brian m. carlson 2022-01-17 21:56 ` [PATCH v3 1/2] wrapper: add a helper to generate numbers from " brian m. carlson 2022-01-18 9:45 ` Ævar Arnfjörð Bjarmason 2022-01-18 13:31 ` René Scharfe 2022-01-17 21:56 ` [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names brian m. carlson 2022-01-18 9:24 ` Ævar Arnfjörð Bjarmason 2022-01-18 14:42 ` René Scharfe 2022-01-18 14:51 ` Ævar Arnfjörð Bjarmason 2022-01-18 16:44 ` René Scharfe 2022-01-18 17:25 ` Junio C Hamano 2022-01-19 17:49 ` René Scharfe 2022-01-22 10:41 ` René Scharfe 2022-01-24 17:08 ` Junio C Hamano 2022-01-19 3:28 ` brian m. carlson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.