* [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
* [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
* 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: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 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 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 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
* [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 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
* 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 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
* 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
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
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).