{"thread":{"id":"56915","subject":"[PATCH 2/2] wrapper: use a CSPRNG to generate random file names","startedAt":"2021-11-16T03:36:45Z","lastAt":"2021-11-22T09:10:35Z","messageCount":37,"participants":["brian m. carlson","Jeff King","rsbecker@nexbridge.com","Taylor Blau","Junio C Hamano","Ævar Arnfjörð Bjarmason","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"441234","messageId":"20211116033542.3247094-3-sandals@crustytoothpaste.net","threadId":"56915","inReplyTo":"20211116033542.3247094-1-sandals@crustytoothpaste.net","subject":"[PATCH 2/2] wrapper: use a CSPRNG to generate random file names","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-11-16T03:35:42Z","receivedAt":"2021-11-16T03:36:45Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The current way we generate random file names is by taking the seconds\nand microseconds, plus the PID, and mixing them together, then encoding\nthem. If this fails, we increment the value by 7777, and try again up\nto TMP_MAX times.\n\nUnfortunately, this is not the best idea from a security perspective.\nIf we're writing into TMPDIR, an attacker can guess these values easily\nand prevent us from creating any temporary files at all by creating them\nall first. POSIX only requires TMP_MAX to be 25, so this is achievable\nin some contexts, even if unlikely to occur in practice.\n\nFortunately, we can simply solve this by using the system\ncryptographically secure pseudorandom number generator (CSPRNG) to\ngenerate a random 64-bit value, and use that as before. Note that there\nis still a small bias here, but because a six-character sequence chosen\nout of 62 characters provides about 36 bits of entropy, the bias here is\nless than 2^-28, which is acceptable, especially considering we'll retry\nseveral times.\n\nNote that the use of a CSPRNG in generating temporary file names is also\nused in many libcs. glibc recently changed from an approach similar to\nours to using a CSPRNG, and FreeBSD and OpenBSD also use a CSPRNG in\nthis case. Even if the likelihood of an attack is low, we should still\nbe at least as responsible in creating temporary files as libc is.\n\nSigned-off-by: brian m. carlson \n---\n wrapper.c | 15 ++++-----------\n 1 file changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 0046f32e46..0cdb5b18ff 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -463,8 +463,6 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \tstatic const int num_letters = ARRAY_SIZE(letters) - 1;\n \tstatic const char x_pattern[] = \"XXXXXX\";\n \tstatic const int num_x = ARRAY_SIZE(x_pattern) - 1;\n-\tuint64_t value;\n-\tstruct timeval tv;\n \tchar *filename_template;\n \tsize_t len;\n \tint fd, count;\n@@ -485,12 +483,13 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \t * Replace pattern's XXXXXX characters with randomness.\n \t * Try TMP_MAX different filenames.\n \t */\n-\tgettimeofday(&tv, NULL);\n-\tvalue = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid();\n \tfilename_template = &pattern[len - num_x - suffix_len];\n \tfor (count = 0; count < TMP_MAX; ++count) {\n-\t\tuint64_t v = value;\n \t\tint i;\n+\t\tuint64_t v;\n+\t\tif (csprng_bytes(&v, sizeof(v)) < 0)\n+\t\t\treturn -1;\n+\n \t\t/* Fill in the random bits. */\n \t\tfor (i = 0; i < num_x; i++) {\n \t\t\tfilename_template[i] = letters[v % num_letters];\n@@ -506,12 +505,6 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \t\t */\n \t\tif (errno != EEXIST)\n \t\t\tbreak;\n-\t\t/*\n-\t\t * This is a random value. It is only necessary that\n-\t\t * the next TMP_MAX values generated by adding 7777 to\n-\t\t * VALUE are different with (module 2^32).\n-\t\t */\n-\t\tvalue += 7777;\n \t}\n \t/* We return the null string if we can't find a unique file name. */\n \tpattern[0] = '\\0';\n"},{"id":"441235","messageId":"20211116033542.3247094-2-sandals@crustytoothpaste.net","threadId":"56915","inReplyTo":"20211116033542.3247094-1-sandals@crustytoothpaste.net","subject":"[PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-11-16T03:35:41Z","receivedAt":"2021-11-16T03:36:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"There are many situations in which having access to a cryptographically\nsecure pseudorandom number generator (CSPRNG) is helpful. In the\nfuture, we'll encounter one of these when dealing with temporary files.\nTo make this possible, let's add a function which reads from a system\nCSPRNG and returns some bytes.\n\nBecause this is a security sensitive interface, we take some\nprecautions. We either succeed by filling the buffer completely as we\nrequested, or we fail. We don't return partial data because the caller\nwill almost never find that to be a useful behavior.\n\nThe order of options is also important here. On systems with\narc4random, which is most of the BSDs, we use that, since, except on\nMirBSD, it uses ChaCha20, which is extremely fast, and sits entirely in\nuserspace, avoiding a system call. We then prefer getrandom over\ngetentropy, because the former has been available longer on Linux, and\nfinally, if none of those are available, we use /dev/urandom, because\nmost Unix-like operating systems provide that API. We prefer options\nthat don't involve device files when possible because those work in some\nrestricted environments where device files may not be available.\n\nmacOS appears to have arc4random but not the arc4random_buf function we\nwant to use, so we let it use the fallback of /dev/urandom. Set the\nconfiguration variables appropriately for Linux and the other BSDs. We\nspecifically only consider versions which receive publicly available\nsecurity support; for example, getrandom(2) and getentropy(3) are only\navailable in FreeBSD 12, which is the oldest version with current\nsecurity support. For the same reason, we don't specify getrandom(2) on\nLinux, because CentOS 7 doesn't support it in glibc (although its kernel\ndoes) and we don't want to resort to making syscalls.\n\nFinally, add a self-test option here to make sure that our buffer\nhandling is correct and we aren't truncating data. We simply read 64\nKiB and then make sure we've seen each byte. The probability of this\ntest failing spuriously is less than 10^-100.\n\nSigned-off-by: brian m. carlson \n---\n Makefile | 25 ++++++++++++\n compat/winansi.c | 6 +++\n config.mak.uname | 9 +++++\n contrib/buildsystems/CMakeLists.txt | 2 +-\n git-compat-util.h | 16 ++++++++\n t/helper/test-csprng.c | 63 +++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0000-basic.sh | 4 ++\n wrapper.c | 56 +++++++++++++++++++++++++\n 10 files changed, 182 insertions(+), 1 deletion(-)\n create mode 100644 t/helper/test-csprng.c\n\ndiff --git a/Makefile b/Makefile\nindex 12be39ac49..1d17021f59 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -234,6 +234,14 @@ all::\n # Define NO_TRUSTABLE_FILEMODE if your filesystem may claim to support\n # the executable mode bit, but doesn't really do so.\n #\n+# Define HAVE_ARC4RANDOM if your system has arc4random and arc4random_buf.\n+#\n+# Define HAVE_GETRANDOM if your system has getrandom.\n+#\n+# Define HAVE_GETENTROPY if your system has getentropy.\n+#\n+# Define HAVE_RTLGENRANDOM if your system has RtlGenRandom (Windows only).\n+#\n # Define NEEDS_MODE_TRANSLATION if your OS strays from the typical file type\n # bits in mode values (e.g. z/OS defines I_SFMT to 0xFF000000 as opposed to the\n # usual 0xF000).\n@@ -694,6 +702,7 @@ TEST_BUILTINS_OBJS += test-bloom.o\n TEST_BUILTINS_OBJS += test-chmtime.o\n TEST_BUILTINS_OBJS += test-config.o\n TEST_BUILTINS_OBJS += test-crontab.o\n+TEST_BUILTINS_OBJS += test-csprng.o\n TEST_BUILTINS_OBJS += test-ctype.o\n TEST_BUILTINS_OBJS += test-date.o\n TEST_BUILTINS_OBJS += test-delta.o\n@@ -1900,6 +1909,22 @@ ifdef HAVE_GETDELIM\n \tBASIC_CFLAGS += -DHAVE_GETDELIM\n endif\n \n+ifdef HAVE_ARC4RANDOM\n+\tBASIC_CFLAGS += -DHAVE_ARC4RANDOM\n+endif\n+\n+ifdef HAVE_GETRANDOM\n+\tBASIC_CFLAGS += -DHAVE_GETRANDOM\n+endif\n+\n+ifdef HAVE_GETENTROPY\n+\tBASIC_CFLAGS += -DHAVE_GETENTROPY\n+endif\n+\n+ifdef HAVE_RTLGENRANDOM\n+\tBASIC_CFLAGS += -DHAVE_RTLGENRANDOM\n+endif\n+\n ifneq ($(PROCFS_EXECUTABLE_PATH),)\n \tprocfs_executable_path_SQ = $(subst ','\\'',$(PROCFS_EXECUTABLE_PATH))\n \tBASIC_CFLAGS += '-DPROCFS_EXECUTABLE_PATH=\"$(procfs_executable_path_SQ)\"'\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex c27b20a79d..0e5a9cc82e 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -3,6 +3,12 @@\n */\n \n #undef NOGDI\n+\n+/*\n+ * Including the appropriate header file for RtlGenRandom causes MSVC to see a\n+ * redefinition of types in an incompatible way when including headers below.\n+ */\n+#undef HAVE_RTLGENRANDOM\n #include \"../git-compat-util.h\"\n #include \n #include \ndiff --git a/config.mak.uname b/config.mak.uname\nindex 3236a4918a..5030d3c70b 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -257,6 +257,9 @@ ifeq ($(uname_S),FreeBSD)\n \tHAVE_PATHS_H = YesPlease\n \tHAVE_BSD_SYSCTL = YesPlease\n \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n+\tHAVE_ARC4RANDOM = YesPlease\n+\tHAVE_GETRANDOM = YesPlease\n+\tHAVE_GETENTROPY = YesPlease\n \tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n \tFILENO_IS_A_MACRO = UnfortunatelyYes\n@@ -271,6 +274,8 @@ ifeq ($(uname_S),OpenBSD)\n \tHAVE_PATHS_H = YesPlease\n \tHAVE_BSD_SYSCTL = YesPlease\n \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n+\tHAVE_ARC4RANDOM = YesPlease\n+\tHAVE_GETENTROPY = YesPlease\n \tPROCFS_EXECUTABLE_PATH = /proc/curproc/file\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n \tFILENO_IS_A_MACRO = UnfortunatelyYes\n@@ -282,6 +287,7 @@ ifeq ($(uname_S),MirBSD)\n \tNEEDS_LIBICONV = YesPlease\n \tHAVE_PATHS_H = YesPlease\n \tHAVE_BSD_SYSCTL = YesPlease\n+\tHAVE_ARC4RANDOM = YesPlease\n endif\n ifeq ($(uname_S),NetBSD)\n \tifeq ($(shell expr \"$(uname_R)\" : '[01]\\.'),2)\n@@ -293,6 +299,7 @@ ifeq ($(uname_S),NetBSD)\n \tHAVE_PATHS_H = YesPlease\n \tHAVE_BSD_SYSCTL = YesPlease\n \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n+\tHAVE_ARC4RANDOM = YesPlease\n \tPROCFS_EXECUTABLE_PATH = /proc/curproc/exe\n endif\n ifeq ($(uname_S),AIX)\n@@ -422,6 +429,7 @@ ifeq ($(uname_S),Windows)\n \tNO_STRTOUMAX = YesPlease\n \tNO_MKDTEMP = YesPlease\n \tNO_INTTYPES_H = YesPlease\n+\tHAVE_RTLGENRANDOM = YesPlease\n \t# VS2015 with UCRT claims that snprintf and friends are C99 compliant,\n \t# so we don't need this:\n \t#\n@@ -624,6 +632,7 @@ ifeq ($(uname_S),MINGW)\n \tNO_POSIX_GOODIES = UnfortunatelyYes\n \tDEFAULT_HELP_FORMAT = html\n \tHAVE_PLATFORM_PROCINFO = YesPlease\n+\tHAVE_RTLGENRANDOM = YesPlease\n \tBASIC_LDFLAGS += -municode\n \tCOMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32\n \tCOMPAT_CFLAGS += -DSTRIP_EXTENSION=\\\".exe\\\"\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex fd1399c440..134e00bde3 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -260,7 +260,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL \"Windows\")\n \t\t\t\t_CONSOLE DETECT_MSYS_TTY STRIP_EXTENSION=\".exe\" NO_SYMLINK_HEAD UNRELIABLE_FSTAT\n \t\t\t\tNOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0\n \t\t\t\tUSE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP\n-\t\t\t\tUNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET)\n+\t\t\t\tUNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET HAVE_RTLGENRANDOM)\n \tlist(APPEND compat_SOURCES compat/mingw.c compat/winansi.c compat/win32/path-utils.c\n \t\tcompat/win32/pthread.c compat/win32mmap.c compat/win32/syslog.c\n \t\tcompat/win32/trace2_win32_process_info.c compat/win32/dirent.c\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex d70ce14286..f2cff656e7 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -165,6 +165,12 @@\n #endif\n #include \n #define GIT_WINDOWS_NATIVE\n+#ifdef HAVE_RTLGENRANDOM\n+/* This is required to get access to RtlGenRandom. */\n+#define SystemFunction036 NTAPI SystemFunction036\n+#include \n+#undef SystemFunction036\n+#endif\n #endif\n \n #include \n@@ -235,6 +241,9 @@\n #else\n #include \n #endif\n+#ifdef HAVE_GETRANDOM\n+#include \n+#endif\n #ifdef NO_INTPTR_T\n /*\n * On I16LP32, ILP32 and LP64 \"long\" is the safe bet, however\n@@ -1381,4 +1390,11 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset)\n \n void sleep_millisec(int millisec);\n \n+/*\n+ * Generate len bytes from the system cryptographically secure PRNG.\n+ * Returns 0 on success and -1 on error, setting errno. The inability to\n+ * satisfy the full request is an error.\n+ */\n+int csprng_bytes(void *buf, size_t len);\n+\n #endif\ndiff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c\nnew file mode 100644\nindex 0000000000..196c14e44f\n--- /dev/null\n+++ b/t/helper/test-csprng.c\n@@ -0,0 +1,63 @@\n+#include \"test-tool.h\"\n+#include \"git-compat-util.h\"\n+\n+/*\n+ * Check that we read each byte value at least once when reading 64 KiB from the\n+ * CSPRNG. This is not to test the quality of the CSPRNG, but to test our\n+ * buffer handling of it.\n+ *\n+ * The probability of this failing by random is less than 10^-100.\n+ */\n+static int selftest(void)\n+{\n+\tint buckets[256] = { 0 };\n+\tunsigned char buf[1024];\n+\tunsigned long count = 64 * 1024;\n+\tint i;\n+\n+\twhile (count) {\n+\t\tif (csprng_bytes(buf, sizeof(buf)) < 0) {\n+\t\t\tperror(\"failed to read\");\n+\t\t\treturn 3;\n+\t\t}\n+\t\tfor (i = 0; i < sizeof(buf); i++)\n+\t\t\tbuckets[buf[i]]++;\n+\t\tcount -= sizeof(buf);\n+\t}\n+\tfor (i = 0; i < ARRAY_SIZE(buckets); i++)\n+\t\tif (!buckets[i]) {\n+\t\t\tfprintf(stderr, \"failed to find any bytes with value %02x\\n\", i);\n+\t\t\treturn 4;\n+\t\t}\n+\treturn 0;\n+}\n+\n+int cmd__csprng(int argc, const char **argv)\n+{\n+\tunsigned long count;\n+\tunsigned char buf[1024];\n+\n+\tif (argc > 2) {\n+\t\tfprintf(stderr, \"usage: %s [--selftest | ]\\n\", argv[0]);\n+\t\treturn 2;\n+\t}\n+\n+\tif (!strcmp(argv[1], \"--selftest\")) {\n+\t\treturn selftest();\n+\t}\n+\n+\tcount = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L;\n+\n+\twhile (count) {\n+\t\tunsigned long chunk = count < sizeof(buf) ? count : sizeof(buf);\n+\t\tif (csprng_bytes(buf, chunk) < 0) {\n+\t\t\tperror(\"failed to read\");\n+\t\t\treturn 5;\n+\t\t}\n+\t\tif (fwrite(buf, chunk, 1, stdout) != chunk)\n+\t\t\treturn 1;\n+\t\tcount -= chunk;\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 3ce5585e53..fc0fb86c1b 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -20,6 +20,7 @@ static struct test_cmd cmds[] = {\n \t{ \"chmtime\", cmd__chmtime },\n \t{ \"config\", cmd__config },\n \t{ \"crontab\", cmd__crontab },\n+\t{ \"csprng\", cmd__csprng },\n \t{ \"ctype\", cmd__ctype },\n \t{ \"date\", cmd__date },\n \t{ \"delta\", cmd__delta },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 9f0f522850..077d9bfcca 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -10,6 +10,7 @@ int cmd__bloom(int argc, const char **argv);\n int cmd__chmtime(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n int cmd__crontab(int argc, const char **argv);\n+int cmd__csprng(int argc, const char **argv);\n int cmd__ctype(int argc, const char **argv);\n int cmd__date(int argc, const char **argv);\n int cmd__delta(int argc, const char **argv);\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex b007f0efef..9647ec9629 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1131,4 +1131,8 @@ test_expect_success 'test_must_fail rejects a non-git command with env' '\n \tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n '\n \n+test_expect_success 'CSPRNG handling functions correctly' '\n+\ttest-tool csprng --selftest\n+'\n+\n test_done\ndiff --git a/wrapper.c b/wrapper.c\nindex 36e12119d7..0046f32e46 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -702,3 +702,59 @@ int open_nofollow(const char *path, int flags)\n \treturn open(path, flags);\n #endif\n }\n+\n+int csprng_bytes(void *buf, size_t len)\n+{\n+#if defined(HAVE_ARC4RANDOM)\n+\tarc4random_buf(buf, len);\n+\treturn 0;\n+#elif defined(HAVE_GETRANDOM)\n+\tssize_t res;\n+\tchar *p = buf;\n+\twhile (len) {\n+\t\tres = getrandom(p, len, 0);\n+\t\tif (res < 0)\n+\t\t\treturn -1;\n+\t\tlen -= res;\n+\t\tp += res;\n+\t}\n+\treturn 0;\n+#elif defined(HAVE_GETENTROPY)\n+\tint res;\n+\tchar *p = buf;\n+\twhile (len) {\n+\t\t/* getentropy has a maximum size of 256 bytes. */\n+\t\tsize_t chunk = len < 256 ? len : 256;\n+\t\tres = getentropy(p, chunk);\n+\t\tif (res < 0)\n+\t\t\treturn -1;\n+\t\tlen -= chunk;\n+\t\tp += chunk;\n+\t}\n+\treturn 0;\n+#elif defined(HAVE_RTLGENRANDOM)\n+\tif (!RtlGenRandom(buf, len))\n+\t\treturn -1;\n+\treturn 0;\n+#else\n+\tssize_t res;\n+\tchar *p = buf;\n+\tint fd, err;\n+\tfd = open(\"/dev/urandom\", O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn -1;\n+\twhile (len) {\n+\t\tres = xread(fd, p, len);\n+\t\tif (res < 0) {\n+\t\t\terr = errno;\n+\t\t\tclose(fd);\n+\t\t\terrno = err;\n+\t\t\treturn -1;\n+\t\t}\n+\t\tlen -= res;\n+\t\tp += res;\n+\t}\n+\tclose(fd);\n+\treturn 0;\n+#endif\n+}\n"},{"id":"441248","messageId":"20211116033542.3247094-1-sandals@crustytoothpaste.net","threadId":"56915","inReplyTo":null,"subject":"[PATCH 0/2] Generate temporary files using a CSPRNG","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-11-16T03:35:40Z","receivedAt":"2021-11-16T05:45:13Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Currently, when we generate a temporary file name, we use the seconds,\nmicroseconds, and the PID to generate a unique value. The resulting\nvalue, while changing frequently, is actually predictable and on some\nsystems, it may be possible to cause a DoS by creating all potential\ntemporary files when the temporary file is being created in TMPDIR.\n\nThe solution to this is to use the system CSPRNG to generate the\ntemporary file name. This is the approach taken by FreeBSD, NetBSD, and\nOpenBSD, and glibc also recently switched to this approach from an\napproach that resembled ours in many ways.\n\nEven if this is not practically exploitable on many systems, it seems\nprudent to be at least as careful about temporary file generation as\nlibc is.\n\nThis issue was mentioned on the security list and it was decided that\nthis was not sensitive enough to warrant a coordinated disclosure, a\nsentiment with which I agree. This is difficult to exploit on most\nsystems, but I think it's still worth fixing.\n\nThis series introduces two commits. The first implements a generic\nfunction which calls the system CSPRNG. A reasonably exhaustive attempt\nis made to pick from the options with a preference for performance. The\nsecond changes our temporary file code to use the CSPRNG.\n\nI have added a test helper that can emit bytes from the CSPRNG, as well\nas a self-test mode. The former is not used, but I anticipated it could\nfind utility in the testsuite, and it was useful for testing by hand, so\nI included it.\n\nThe careful reader will notice that the sole additional test is added to\nt0000. That's because temporary file generation is fundamental to how\nGit operates and if it fails, the entire testsuite is broken. Thus, a\nsimple test to verify that it's working seems prudent as part of t0000.\nI was also unable to find a better place to put it, but am open to\nsuggestions if folks have ideas.\n\nThis passes our CI, including on Windows, and I have manually verified\nthe correctness of the other four branches on Linux (the HAVE_ARC4RANDOM\nbranch requiring a small patch which is not necessary on systems which\nhave it in libc and which is therefore not included here).\n\nI am of course interested in hearing from anyone who lacks one of the\nCSPRNG interfaces we have here. Looking at the Go standard library,\n/dev/urandom should be available on at least AIX, Darwin (macOS),\nDragonflyBSD, FreeBSD, Linux, NetBSD, OpenBSD, and Solaris, and I\nbelieve it is available on most other Unix systems as well.\nRtlGenRandom is available on Windows back to XP, which we no longer\nsupport. The bizarre header contortion on Windows comes from Mozilla,\nbut is widely used in other codebases with no substantial changes.\n\nFor those who are interested, I computed the probability of spurious\nfailure for the self-test mode like so:\n\n 256 * (255/256)^65536\n\nThis Ruby one-liner estimates the probability at approximately 10^-108:\n\n ruby -e 'a = 255 ** 65536; b = 256 ** 65536; puts b.to_s.length - a.to_s.length - 3'\n\nIf I have made an error in the calculation, please do feel free to point\nit out.\n\nbrian m. carlson (2):\n wrapper: add a helper to generate numbers from a CSPRNG\n wrapper: use a CSPRNG to generate random file names\n\n Makefile | 25 ++++++++++\n compat/winansi.c | 6 +++\n config.mak.uname | 9 ++++\n contrib/buildsystems/CMakeLists.txt | 2 +-\n git-compat-util.h | 16 +++++++\n t/helper/test-csprng.c | 63 +++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0000-basic.sh | 4 ++\n wrapper.c | 71 ++++++++++++++++++++++++-----\n 10 files changed, 186 insertions(+), 12 deletions(-)\n create mode 100644 t/helper/test-csprng.c\n\n"},{"id":"441330","messageId":"YZPOzqU0UQDVA57R@coredump.intra.peff.net","threadId":"56915","inReplyTo":"20211116033542.3247094-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-16T15:31:26Z","receivedAt":"2021-11-16T15:31:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote:\n\n> The order of options is also important here. On systems with\n> arc4random, which is most of the BSDs, we use that, since, except on\n> MirBSD, it uses ChaCha20, which is extremely fast, and sits entirely in\n> userspace, avoiding a system call. We then prefer getrandom over\n> getentropy, because the former has been available longer on Linux, and\n> finally, if none of those are available, we use /dev/urandom, because\n> most Unix-like operating systems provide that API. We prefer options\n> that don't involve device files when possible because those work in some\n> restricted environments where device files may not be available.\n\nI wonder if we'll need a low-quality fallback for older systems which\ndon't even have /dev/urandom. Because it's going to be used in such a\ncore part of the system (tempfiles), this basically becomes a hard\nrequirement for using Git at all.\n\nI can't say I'm excited in general to be introducing a dependency like\nthis, just because of the portability headaches. But it may be the least\nbad thing (especially if we can fall back to the existing behavior).\nOne alternative would be to build on top of the system mkstemp(), which\nmakes it libc's problem. I'm not sure if we'd run into problems there,\nthough.\n\n> diff --git a/Makefile b/Makefile\n> index 12be39ac49..1d17021f59 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -234,6 +234,14 @@ all::\n> # Define NO_TRUSTABLE_FILEMODE if your filesystem may claim to support\n> # the executable mode bit, but doesn't really do so.\n> #\n> +# Define HAVE_ARC4RANDOM if your system has arc4random and arc4random_buf.\n> +#\n> +# Define HAVE_GETRANDOM if your system has getrandom.\n> +#\n> +# Define HAVE_GETENTROPY if your system has getentropy.\n> +#\n> +# Define HAVE_RTLGENRANDOM if your system has RtlGenRandom (Windows only).\n\nIt seems like these will be mutually exclusive (and indeed, the #ifdef\nin the code ends up defining a particular precedence). Would we be\nbetter off exposing that to the user with a single CSPRNG_METHOD set to\narc4random, getrandom, getentropy, etc?\n\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 3236a4918a..5030d3c70b 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -257,6 +257,9 @@ ifeq ($(uname_S),FreeBSD)\n> \tHAVE_PATHS_H = YesPlease\n> \tHAVE_BSD_SYSCTL = YesPlease\n> \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n> +\tHAVE_ARC4RANDOM = YesPlease\n> +\tHAVE_GETRANDOM = YesPlease\n> +\tHAVE_GETENTROPY = YesPlease\n\nSo here we claim to support a whole bunch of methods, but in practice,\nwe only use arc4random, because these are all in an #elif chain:\n\n> +int csprng_bytes(void *buf, size_t len)\n> +{\n> +#if defined(HAVE_ARC4RANDOM)\n> +\tarc4random_buf(buf, len);\n> +\treturn 0;\n> +#elif defined(HAVE_GETRANDOM)\n\nthough we still respect the others in other places, like including\nheaders that we don't end up using:\n\n> +#ifdef HAVE_GETRANDOM\n> +#include \n> +#endif\n\nIf csprng_bytes() could fallback between methods based on runtime\nerrors, it would make sense to me to allow support for multiple methods\nto be declared. But without that, it just seems to invite confusion (and\nI am not sure runtime fallbacks are really worth the trouble).\n\n> +int csprng_bytes(void *buf, size_t len)\n> +{\n> +#if defined(HAVE_ARC4RANDOM)\n> +\tarc4random_buf(buf, len);\n> +\treturn 0;\n\nOK, presumably this one can't return an error, which is nice.\n\n> +#elif defined(HAVE_GETRANDOM)\n> +\n> +\tssize_t res;\n> +\tchar *p = buf;\n> +\twhile (len) {\n> +\t\tres = getrandom(p, len, 0);\n> +\t\tif (res < 0)\n> +\t\t\treturn -1;\n> +\t\tlen -= res;\n> +\t\tp += res;\n> +\t}\n> +\treturn 0;\n\nDo we ever have to worry about a \"0\" return from getrandom()? I'd expect\nit to block rather than return 0, but what I'm wondering is if we could\never be in a situation where we fail to make progress and loop\ninfinitely.\n\nThe manpage says that reads up to 256 bytes will always return the full\noutput and never be interrupted. So for the caller you add in patch 2,\nwe wouldn't need this loop. However, since cspring_bytes() is generic,\nbeing defensive makes sense. But in that case, do we need to handle\nEINTR when it returns -1?\n\n> +#elif defined(HAVE_GETENTROPY)\n> +\tint res;\n> +\tchar *p = buf;\n> +\twhile (len) {\n> +\t\t/* getentropy has a maximum size of 256 bytes. */\n> +\t\tsize_t chunk = len < 256 ? len : 256;\n> +\t\tres = getentropy(p, chunk);\n> +\t\tif (res < 0)\n> +\t\t\treturn -1;\n> +\t\tlen -= chunk;\n> +\t\tp += chunk;\n> +\t}\n> +\treturn 0;\n\nHeh, I see that getentropy() punted on all of those questions above by\njust insisting you ask for 256 bytes at a time. Cute solution. ;)\n\n> +#elif defined(HAVE_RTLGENRANDOM)\n> +\tif (!RtlGenRandom(buf, len))\n> +\t\treturn -1;\n> +\treturn 0;\n\nI have no comment on this one. :)\n\n> +#else\n> +\tssize_t res;\n> +\tchar *p = buf;\n> +\tint fd, err;\n> +\tfd = open(\"/dev/urandom\", O_RDONLY);\n> +\tif (fd < 0)\n> +\t\treturn -1;\n> +\twhile (len) {\n> +\t\tres = xread(fd, p, len);\n> +\t\tif (res < 0) {\n> +\t\t\terr = errno;\n> +\t\t\tclose(fd);\n> +\t\t\terrno = err;\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\tlen -= res;\n> +\t\tp += res;\n> +\t}\n> +\tclose(fd);\n> +\treturn 0;\n> +#endif\n> +}\n\nThis loop is basically read_in_full(), except that it doesn't treat a\n\"0\" return as an EOF. I'm not sure if that's intentional (because we\nwant to keep trying on a 0 return, though I'd expect the read to block\nin such a case), or if it would be an improvement (because it would\nprevent us from infinite looping if /dev/urandom wanted to signal EOF).\n\n-Peff\n"},{"id":"441331","messageId":"YZPQE+SKVXjexXMT@coredump.intra.peff.net","threadId":"56915","inReplyTo":"20211116033542.3247094-3-sandals@crustytoothpaste.net","subject":"Re: [PATCH 2/2] wrapper: use a CSPRNG to generate random file names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-16T15:36:51Z","receivedAt":"2021-11-16T15:36:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 16, 2021 at 03:35:42AM +0000, brian m. carlson wrote:\n\n> The current way we generate random file names is by taking the seconds\n> and microseconds, plus the PID, and mixing them together, then encoding\n> them. If this fails, we increment the value by 7777, and try again up\n> to TMP_MAX times.\n> \n> Unfortunately, this is not the best idea from a security perspective.\n> If we're writing into TMPDIR, an attacker can guess these values easily\n> and prevent us from creating any temporary files at all by creating them\n> all first. POSIX only requires TMP_MAX to be 25, so this is achievable\n> in some contexts, even if unlikely to occur in practice.\n\nI think we unconditionally define TMP_MAX as 16384. I don't think that\nchanges the fundamental issue that somebody could race us and win,\nthough.\n\n> @@ -485,12 +483,13 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n> \t * Replace pattern's XXXXXX characters with randomness.\n> \t * Try TMP_MAX different filenames.\n> \t */\n> -\tgettimeofday(&tv, NULL);\n> -\tvalue = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid();\n> \tfilename_template = &pattern[len - num_x - suffix_len];\n> \tfor (count = 0; count < TMP_MAX; ++count) {\n> -\t\tuint64_t v = value;\n> \t\tint i;\n> +\t\tuint64_t v;\n> +\t\tif (csprng_bytes(&v, sizeof(v)) < 0)\n> +\t\t\treturn -1;\n\nIf csprng_bytes() fail, the resulting errno is likely to be confusing.\nE.g., if /dev/urandom doesn't exist we'd get ENOENT. But the caller is\nlikely to say something like:\n\n error: unable to create temporary file: no such file or directory\n\nwhich is misleading. It's probably worth doing:\n\n return error_errno(\"unable to get random bytes for temporary file\");\n\nor similar here. That's verbose on top of the error that the caller will\ngive, but this is something we don't expect to fail in practice.\n\nI actually wonder if we should simply die() in such a case. That's not\nvery friendly from a libification stand-point, but we really can't\nprogress on much without being able to generate random bytes.\n\n-Peff\n"},{"id":"441335","messageId":"YZPR4Ya1ocWvKbAf@coredump.intra.peff.net","threadId":"56915","inReplyTo":"20211116033542.3247094-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/2] Generate temporary files using a CSPRNG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-16T15:44:33Z","receivedAt":"2021-11-16T15:44:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 16, 2021 at 03:35:40AM +0000, brian m. carlson wrote:\n\n> For those who are interested, I computed the probability of spurious\n> failure for the self-test mode like so:\n> \n> 256 * (255/256)^65536\n> \n> This Ruby one-liner estimates the probability at approximately 10^-108:\n> \n> ruby -e 'a = 255 ** 65536; b = 256 ** 65536; puts b.to_s.length - a.to_s.length - 3'\n> \n> If I have made an error in the calculation, please do feel free to point\n> it out.\n\nYes, I think your math is correct there.\n\nA more interesting question is whether generating 64k of PRNG bytes per\ntest run is going to a problem for system entropy pools. For that\nmatter, I guess the use of it for tempfiles will produce a similar\nburden, since we run so many commands. My understanding is that modern\nsystems will just produce infinite output for /dev/urandom, etc, but I\nwonder if there are any systems left where that is not true (because\nthey have a misguided notion that they need to stir in more \"real\"\nentropy bits).\n\n-Peff\n"},{"id":"441336","messageId":"009d01d7db03$354ecae0$9fec60a0$@nexbridge.com","threadId":"56915","inReplyTo":"YZPOzqU0UQDVA57R@coredump.intra.peff.net","subject":"RE: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-11-16T16:01:20Z","receivedAt":"2021-11-16T16:01:42Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On November 16, 2021 10:31 AM, Jeff King wrote:\n> On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote:\n> \n> > The order of options is also important here. On systems with\n> > arc4random, which is most of the BSDs, we use that, since, except on\n> > MirBSD, it uses ChaCha20, which is extremely fast, and sits entirely\n> > in userspace, avoiding a system call. We then prefer getrandom over\n> > getentropy, because the former has been available longer on Linux, and\n> > finally, if none of those are available, we use /dev/urandom, because\n> > most Unix-like operating systems provide that API. We prefer options\n> > that don't involve device files when possible because those work in\n> > some restricted environments where device files may not be available.\n> \n> I wonder if we'll need a low-quality fallback for older systems which don't\n> even have /dev/urandom. Because it's going to be used in such a core part of\n> the system (tempfiles), this basically becomes a hard requirement for using\n> Git at all.\n> \n> I can't say I'm excited in general to be introducing a dependency like this, just\n> because of the portability headaches. But it may be the least bad thing\n> (especially if we can fall back to the existing behavior).\n> One alternative would be to build on top of the system mkstemp(), which\n> makes it libc's problem. I'm not sure if we'd run into problems there, though.\n\nNone of /dev/urandom, /dev/random, or mkstemp are available on some platforms, including NonStop. This is not a good dependency to add. One variant PRNGD is used in ia64 OpenSSL, while the CPU random generator in hardware is used on x86. I cannot get behind this at all. Libc is also not used in or available to our port. I am very worried about this direction.\n\n-Randall\n\n"},{"id":"441340","messageId":"YZP3ApaDcMGIsKeb@nand.local","threadId":"56915","inReplyTo":"009d01d7db03$354ecae0$9fec60a0$@nexbridge.com","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-16T18:22:58Z","receivedAt":"2021-11-16T18:23:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 16, 2021 at 11:01:20AM -0500, rsbecker@nexbridge.com wrote:\n> On November 16, 2021 10:31 AM, Jeff King wrote:\n> > On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote:\n> >\n> > > The order of options is also important here. On systems with\n> > > arc4random, which is most of the BSDs, we use that, since, except on\n> > > MirBSD, it uses ChaCha20, which is extremely fast, and sits entirely\n> > > in userspace, avoiding a system call. We then prefer getrandom over\n> > > getentropy, because the former has been available longer on Linux, and\n> > > finally, if none of those are available, we use /dev/urandom, because\n> > > most Unix-like operating systems provide that API. We prefer options\n> > > that don't involve device files when possible because those work in\n> > > some restricted environments where device files may not be available.\n> >\n> > I wonder if we'll need a low-quality fallback for older systems which don't\n> > even have /dev/urandom. Because it's going to be used in such a core part of\n> > the system (tempfiles), this basically becomes a hard requirement for using\n> > Git at all.\n> >\n> > I can't say I'm excited in general to be introducing a dependency like this, just\n> > because of the portability headaches. But it may be the least bad thing\n> > (especially if we can fall back to the existing behavior).\n> > One alternative would be to build on top of the system mkstemp(), which\n> > makes it libc's problem. I'm not sure if we'd run into problems there, though.\n>\n> None of /dev/urandom, /dev/random, or mkstemp are available on some\n> platforms, including NonStop. This is not a good dependency to add.\n> One variant PRNGD is used in ia64 OpenSSL, while the CPU random\n> generator in hardware is used on x86. I cannot get behind this at all.\n> Libc is also not used in or available to our port. I am very worried\n> about this direction.\n\nI share Peff's lack of enthusiasm about the dependency situation. But\nmaking Git depend on having /dev/urandom available is simply not\nfeasible, as you point out.\n\nI wonder if the suitable fall-back should be the existing behavior of\ngit_mkstemps_mode()? That leaves us in a somewhat-disappointing\nsituation of not having fully resolved the DOS attack on all platforms.\nBut it makes our dependency situation less complicated, and leaves\nthings no worse off than the were before on platforms like NonStop.\n\nThanks,\nTaylor\n"},{"id":"441343","messageId":"YZP4XDtF7O7Sxh17@nand.local","threadId":"56915","inReplyTo":"YZPQE+SKVXjexXMT@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] wrapper: use a CSPRNG to generate random file names","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-16T18:28:44Z","receivedAt":"2021-11-16T18:28:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 16, 2021 at 10:36:51AM -0500, Jeff King wrote:\n> On Tue, Nov 16, 2021 at 03:35:42AM +0000, brian m. carlson wrote:\n>\n> > The current way we generate random file names is by taking the seconds\n> > and microseconds, plus the PID, and mixing them together, then encoding\n> > them. If this fails, we increment the value by 7777, and try again up\n> > to TMP_MAX times.\n> >\n> > Unfortunately, this is not the best idea from a security perspective.\n> > If we're writing into TMPDIR, an attacker can guess these values easily\n> > and prevent us from creating any temporary files at all by creating them\n> > all first. POSIX only requires TMP_MAX to be 25, so this is achievable\n> > in some contexts, even if unlikely to occur in practice.\n>\n> I think we unconditionally define TMP_MAX as 16384. I don't think that\n> changes the fundamental issue that somebody could race us and win,\n> though.\n\nYes, we do. Right above the declaration of this function (and so hidden\nfrom the context) we do:\n\n #undef TMP_MAX\n #define TMP_MAX 16384\n\nI don't think that the value of TMP_MAX makes this substantially less\nlikely, so I agree that the fundamental issue is the same.\n\n> > @@ -485,12 +483,13 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n> > \t * Replace pattern's XXXXXX characters with randomness.\n> > \t * Try TMP_MAX different filenames.\n> > \t */\n> > -\tgettimeofday(&tv, NULL);\n> > -\tvalue = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid();\n> > \tfilename_template = &pattern[len - num_x - suffix_len];\n> > \tfor (count = 0; count < TMP_MAX; ++count) {\n> > -\t\tuint64_t v = value;\n> > \t\tint i;\n> > +\t\tuint64_t v;\n> > +\t\tif (csprng_bytes(&v, sizeof(v)) < 0)\n> > +\t\t\treturn -1;\n>\n> If csprng_bytes() fail, the resulting errno is likely to be confusing.\n> E.g., if /dev/urandom doesn't exist we'd get ENOENT. But the caller is\n> likely to say something like:\n>\n> error: unable to create temporary file: no such file or directory\n>\n> which is misleading. It's probably worth doing:\n>\n> return error_errno(\"unable to get random bytes for temporary file\");\n>\n> or similar here. That's verbose on top of the error that the caller will\n> give, but this is something we don't expect to fail in practice.\n>\n> I actually wonder if we should simply die() in such a case. That's not\n> very friendly from a libification stand-point, but we really can't\n> progress on much without being able to generate random bytes.\n\nAlternatively, we could fall back to the existing code paths. This is\nsomewhat connected to my suggestion to Randall earlier in the thread.\nBut I would rather see that fallback done at compile-time for platforms\nthat don't give us an easy-to-use CSPRNG, and avoid masking legitimate\nerrors caused from trying to use a CSPRNG that should exist.\n\nThanks,\nTaylor\n"},{"id":"441348","messageId":"xmqqtugc9bdz.fsf@gitster.g","threadId":"56915","inReplyTo":"YZP4XDtF7O7Sxh17@nand.local","subject":"Re: [PATCH 2/2] wrapper: use a CSPRNG to generate random file names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-16T18:57:28Z","receivedAt":"2021-11-16T18:57:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> I actually wonder if we should simply die() in such a case. That's not\n>> very friendly from a libification stand-point, but we really can't\n>> progress on much without being able to generate random bytes.\n>\n> Alternatively, we could fall back to the existing code paths. This is\n> somewhat connected to my suggestion to Randall earlier in the thread.\n> But I would rather see that fallback done at compile-time for platforms\n> that don't give us an easy-to-use CSPRNG, and avoid masking legitimate\n> errors caused from trying to use a CSPRNG that should exist.\n\nYeah, I do not think we are doing this because the current code is\ncompletely broken and everybody needs to move to CSPRNG that makes\nit absoletely safe---rather this is still just making it safer than\nthe current code, when system support is available. So a fallback\nto the current code would be a good (and easy) thing to have, I\nwould think.\n\nThanks.\n"},{"id":"441352","messageId":"YZQEsjidbBUgvhfu@coredump.intra.peff.net","threadId":"56915","inReplyTo":"xmqqtugc9bdz.fsf@gitster.g","subject":"Re: [PATCH 2/2] wrapper: use a CSPRNG to generate random file names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-16T19:21:22Z","receivedAt":"2021-11-16T19:21:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 16, 2021 at 10:57:28AM -0800, Junio C Hamano wrote:\n\n> Taylor Blau writes:\n> \n> >> I actually wonder if we should simply die() in such a case. That's not\n> >> very friendly from a libification stand-point, but we really can't\n> >> progress on much without being able to generate random bytes.\n> >\n> > Alternatively, we could fall back to the existing code paths. This is\n> > somewhat connected to my suggestion to Randall earlier in the thread.\n> > But I would rather see that fallback done at compile-time for platforms\n> > that don't give us an easy-to-use CSPRNG, and avoid masking legitimate\n> > errors caused from trying to use a CSPRNG that should exist.\n> \n> Yeah, I do not think we are doing this because the current code is\n> completely broken and everybody needs to move to CSPRNG that makes\n> it absoletely safe---rather this is still just making it safer than\n> the current code, when system support is available. So a fallback\n> to the current code would be a good (and easy) thing to have, I\n> would think.\n\nOne challenge for any fallback is that there are security implications.\nIn particular:\n\n - the fallback probably needs to be specific to the mktemp code; we\n don't have any callers yet of csprng_bytes(), but anybody using it\n for, say, actual cryptography would be very unhappy if it quietly\n fell back to insecure bytes.\n\n (I don't have any plans to use it and we don't do very much actual\n crypto ourselves, but one place that _could_ use it is the\n generation of the push-cert nonce seed).\n\n - I'm not sure if we should fallback for runtime errors or not. E.g.,\n if we try to open /dev/urandom and it isn't there, is it OK to fall\n back to the older, less-secure tempfile method? That's convenient in\n some sense; Git continues to work inside a chroot for which you\n haven't set up /dev/urandom. But it may also be surprising, and\n erring on the side of doing the less secure thing is probably a bad\n idea.\n\n So the mktemp code probably needs to be aware of the difference\n between \"we have no CSPRNG source\" and \"we were compiled with\n support for a source, but it didn't work\".\n\n-Peff\n"},{"id":"441354","messageId":"YZQHe+TXPFbafP/o@nand.local","threadId":"56915","inReplyTo":"YZQEsjidbBUgvhfu@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] wrapper: use a CSPRNG to generate random file names","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-16T19:33:15Z","receivedAt":"2021-11-16T19:33:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 16, 2021 at 02:21:22PM -0500, Jeff King wrote:\n> On Tue, Nov 16, 2021 at 10:57:28AM -0800, Junio C Hamano wrote:\n>\n> > Taylor Blau writes:\n> >\n> > >> I actually wonder if we should simply die() in such a case. That's not\n> > >> very friendly from a libification stand-point, but we really can't\n> > >> progress on much without being able to generate random bytes.\n> > >\n> > > Alternatively, we could fall back to the existing code paths. This is\n> > > somewhat connected to my suggestion to Randall earlier in the thread.\n> > > But I would rather see that fallback done at compile-time for platforms\n> > > that don't give us an easy-to-use CSPRNG, and avoid masking legitimate\n> > > errors caused from trying to use a CSPRNG that should exist.\n> >\n> > Yeah, I do not think we are doing this because the current code is\n> > completely broken and everybody needs to move to CSPRNG that makes\n> > it absoletely safe---rather this is still just making it safer than\n> > the current code, when system support is available. So a fallback\n> > to the current code would be a good (and easy) thing to have, I\n> > would think.\n>\n> One challenge for any fallback is that there are security implications.\n> In particular:\n>\n> - the fallback probably needs to be specific to the mktemp code; we\n> don't have any callers yet of csprng_bytes(), but anybody using it\n> for, say, actual cryptography would be very unhappy if it quietly\n> fell back to insecure bytes.\n>\n> (I don't have any plans to use it and we don't do very much actual\n> crypto ourselves, but one place that _could_ use it is the\n> generation of the push-cert nonce seed).\n>\n> - I'm not sure if we should fallback for runtime errors or not. E.g.,\n> if we try to open /dev/urandom and it isn't there, is it OK to fall\n> back to the older, less-secure tempfile method? That's convenient in\n> some sense; Git continues to work inside a chroot for which you\n> haven't set up /dev/urandom. But it may also be surprising, and\n> erring on the side of doing the less secure thing is probably a bad\n> idea.\n>\n> So the mktemp code probably needs to be aware of the difference\n> between \"we have no CSPRNG source\" and \"we were compiled with\n> support for a source, but it didn't work\".\n\nMy opinion is that we should probably not fallback for runtime errors\nwhere we do have a CSPRNG and any errors trying to use it are\nlegitimate.\n\nI would probably have csprng_bytes() itself only be compiled where we\nknow we have a CSPRNG. And then I think our implementation of\ngit_mkstemps_mode() would depend on whether csprng_bytes() was compiled\nor not. If it was, then any errors returned by it are propagated to the\ncaller (or we call die()). If not, then we use the existing, insecure\nimplementation.\n\nAnd I think that basically addresses both of your points, namely that\nthe fallback is specific to the mktemp code, and provides one opinion on\nthe matter of runtime errors.\n\nThanks,\nTaylor\n"},{"id":"441357","messageId":"00af01d7db24$5a54c710$0efe5530$@nexbridge.com","threadId":"56915","inReplyTo":"YZP3ApaDcMGIsKeb@nand.local","subject":"RE: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-11-16T19:58:35Z","receivedAt":"2021-11-16T19:58:56Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On November 16, 2021 1:23 PM, Taylor Blau wrote:\n> On Tue, Nov 16, 2021 at 11:01:20AM -0500, rsbecker@nexbridge.com wrote:\n> > On November 16, 2021 10:31 AM, Jeff King wrote:\n> > > On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote:\n> > >\n> > > > The order of options is also important here. On systems with\n> > > > arc4random, which is most of the BSDs, we use that, since, except\n> > > > on MirBSD, it uses ChaCha20, which is extremely fast, and sits\n> > > > entirely in userspace, avoiding a system call. We then prefer\n> > > > getrandom over getentropy, because the former has been available\n> > > > longer on Linux, and finally, if none of those are available, we\n> > > > use /dev/urandom, because most Unix-like operating systems provide\n> > > > that API. We prefer options that don't involve device files when\n> > > > possible because those work in some restricted environments where\n> device files may not be available.\n> > >\n> > > I wonder if we'll need a low-quality fallback for older systems\n> > > which don't even have /dev/urandom. Because it's going to be used in\n> > > such a core part of the system (tempfiles), this basically becomes a\n> > > hard requirement for using Git at all.\n> > >\n> > > I can't say I'm excited in general to be introducing a dependency\n> > > like this, just because of the portability headaches. But it may be\n> > > the least bad thing (especially if we can fall back to the existing behavior).\n> > > One alternative would be to build on top of the system mkstemp(),\n> > > which makes it libc's problem. I'm not sure if we'd run into problems\n> there, though.\n> >\n> > None of /dev/urandom, /dev/random, or mkstemp are available on some\n> > platforms, including NonStop. This is not a good dependency to add.\n> > One variant PRNGD is used in ia64 OpenSSL, while the CPU random\n> > generator in hardware is used on x86. I cannot get behind this at all.\n> > Libc is also not used in or available to our port. I am very worried\n> > about this direction.\n> \n> I share Peff's lack of enthusiasm about the dependency situation. But making\n> Git depend on having /dev/urandom available is simply not feasible, as you\n> point out.\n> \n> I wonder if the suitable fall-back should be the existing behavior of\n> git_mkstemps_mode()? That leaves us in a somewhat-disappointing\n> situation of not having fully resolved the DOS attack on all platforms.\n> But it makes our dependency situation less complicated, and leaves things no\n> worse off than the were before on platforms like NonStop.\n\nThe general advice on NonStop is to delegate handling DOS attacks to either SSH or firewalls (preferably). I have yet to see anyone publish a git service on that platform outside of using SSH anyway - and if they did, they would get a pretty fierce glare from me.\n-Randall\n\n"},{"id":"441361","messageId":"211116.864k8bq0xm.gmgdl@evledraar.gmail.com","threadId":"56915","inReplyTo":"20211116033542.3247094-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/2] Generate temporary files using a CSPRNG","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-16T20:35:59Z","receivedAt":"2021-11-16T20:51:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 16 2021, brian m. carlson wrote:\n\n> Currently, when we generate a temporary file name, we use the seconds,\n> microseconds, and the PID to generate a unique value. The resulting\n> value, while changing frequently, is actually predictable and on some\n> systems, it may be possible to cause a DoS by creating all potential\n> temporary files when the temporary file is being created in TMPDIR.\n>\n> The solution to this is to use the system CSPRNG to generate the\n> temporary file name. This is the approach taken by FreeBSD, NetBSD, and\n> OpenBSD, and glibc also recently switched to this approach from an\n> approach that resembled ours in many ways.\n>\n> Even if this is not practically exploitable on many systems, it seems\n> prudent to be at least as careful about temporary file generation as\n> libc is.\n> \n> This issue was mentioned on the security list and it was decided that\n> this was not sensitive enough to warrant a coordinated disclosure, a\n> sentiment with which I agree. This is difficult to exploit on most\n> systems, but I think it's still worth fixing.\n\nI skimmed that report on the security list, and having skimmed this\npatch series I think what's missing is something like this summary of\nyours there (which I hope you don't mind me quoting):\n\n Now, in Git's case, I don't think our security model allows untrusted\n users to write directly into the repository, so I don't think this\n constitutes a vulnerability there. We have a function that uses TMPDIR,\n which appears to be used for prepping temporary blobs in diffs and in\n GnuPG verification, which is definitely more questionable.\n\nI tried testing this codepath real quick now with:\n \n diff --git a/wrapper.c b/wrapper.c\n index 36e12119d76..2f3755886fb 100644\n --- a/wrapper.c\n +++ b/wrapper.c\n @@ -497,6 +497,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n v /= num_letters;\n }\n \n + BUG(\"%s\", pattern);\n fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n if (fd >= 0)\n return fd;\n \nAnd then doing:\n\n grep BUG test-results/*.out\n\nAnd the resulting output is all of the form:\n\n .git/objects/9f/tmp_obj_FOzEcZ\n .git/objects/pack/tmp_pack_fJC0RI\n\nAnd a couple of:\n\n .git/info/refs_Lctaew\n\nI.e. these are all cases where we're creating in-repo tempfiles, we're\nnot racing someone in /tmp/ for these, except perhaps in some cases I've\nmissed (but you allude to) where we presumably should just move those\ninto .git/tmp/, at least by default.\n\nDoesn't that entirely solve this security problem going forward? If a\nhostile actor can write into your .git/ they don't need to screw with\nyou in this way, they can just write executable aliases, or the same in\n.git/hook/.\n\nUnless that is we do have some use-case for potentially racing others in\n/tmp/, but then we could make that specifically configurable etc.\n\nI really don't mind us having a better tempfile() function principle,\nbut so far this sort of hardening just seems entirely unnecessary to me.\n\nAs seen from your implementation requires us top dip our toes into\nseeding random data, which I'd think from a security maintenance\nperspective we'd be much better offloading to the OS going forward if at\nall possible.\n\nIf there are cases where we actually need this hardening because we're\nwriting in a shared /tmp/ and not .git/, then surely we're better having\nthose API users call a differently named function, or to move those\nusers to using a .git/tmp/ unless they configure things otherwise?\n"},{"id":"441362","messageId":"YZQdRC9nAA/CbMyM@coredump.intra.peff.net","threadId":"56915","inReplyTo":"211116.864k8bq0xm.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/2] Generate temporary files using a CSPRNG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-16T21:06:12Z","receivedAt":"2021-11-16T21:06:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 16, 2021 at 09:35:59PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> I tried testing this codepath real quick now with:\n> \n> diff --git a/wrapper.c b/wrapper.c\n> index 36e12119d76..2f3755886fb 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -497,6 +497,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n> v /= num_letters;\n> }\n> \n> + BUG(\"%s\", pattern);\n> fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n> if (fd >= 0)\n> return fd;\n> \n> And then doing:\n> \n> grep BUG test-results/*.out\n> \n> And the resulting output is all of the form:\n> \n> .git/objects/9f/tmp_obj_FOzEcZ\n> .git/objects/pack/tmp_pack_fJC0RI\n> \n> And a couple of:\n> \n> .git/info/refs_Lctaew\n> \n> I.e. these are all cases where we're creating in-repo tempfiles, we're\n> not racing someone in /tmp/ for these, except perhaps in some cases I've\n> missed (but you allude to) where we presumably should just move those\n> into .git/tmp/, at least by default.\n\nYour patch is way too aggressive. By bailing via BUG(), most commands\nwill fail, so we never get to the interesting ones (e.g., we would not\never get to the point of writing out a tag signature for gpg to verify,\nbecause we'd barf when trying to create the tag in the first place).\n\nTry:\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 36e12119d7..5218a4b3bd 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -497,6 +497,10 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \t\t\tv /= num_letters;\n \t\t}\n \n+\t\t{\n+\t\t\tstatic struct trace_key t = TRACE_KEY_INIT(TEMPFILE);\n+\t\t\ttrace_printf_key(&t, \"%s\", pattern);\n+\t\t}\n \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n\nAnd then:\n\n GIT_TRACE_TEMPFILE=/tmp/foo make test\n grep ^/tmp /tmp/foo | wc -l\n\nturns up hundreds of hits.\n\n> If there are cases where we actually need this hardening because we're\n> writing in a shared /tmp/ and not .git/, then surely we're better having\n> those API users call a differently named function, or to move those\n> users to using a .git/tmp/ unless they configure things otherwise?\n\nAssuming you can write to .git/tmp means that conceptually read-only\noperations (like verifying tags) require write access to the repository.\n\n-Peff\n"},{"id":"441370","messageId":"YZQuDVMRx6hGEbpe@camp.crustytoothpaste.net","threadId":"56915","inReplyTo":"YZPR4Ya1ocWvKbAf@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] Generate temporary files using a CSPRNG","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-11-16T22:17:49Z","receivedAt":"2021-11-16T22:17:54Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-11-16 at 15:44:33, Jeff King wrote:\n> On Tue, Nov 16, 2021 at 03:35:40AM +0000, brian m. carlson wrote:\n> \n> > For those who are interested, I computed the probability of spurious\n> > failure for the self-test mode like so:\n> > \n> > 256 * (255/256)^65536\n> > \n> > This Ruby one-liner estimates the probability at approximately 10^-108:\n> > \n> > ruby -e 'a = 255 ** 65536; b = 256 ** 65536; puts b.to_s.length - a.to_s.length - 3'\n> > \n> > If I have made an error in the calculation, please do feel free to point\n> > it out.\n> \n> Yes, I think your math is correct there.\n> \n> A more interesting question is whether generating 64k of PRNG bytes per\n> test run is going to a problem for system entropy pools. For that\n> matter, I guess the use of it for tempfiles will produce a similar\n> burden, since we run so many commands. My understanding is that modern\n> systems will just produce infinite output for /dev/urandom, etc, but I\n> wonder if there are any systems left where that is not true (because\n> they have a misguided notion that they need to stir in more \"real\"\n> entropy bits).\n\nI have specifically avoided invoking any sort of potentially blocking\nCSPRNG for that reason. /dev/urandom is specifically not supposed to\nblock, and on the systems that I mentioned, the way Go uses it would\nindicate that it should not. There is a system, which is Plan 9, where\nGo uses /dev/random to seed an X.917 generator, and there I assume there\nis no /dev/urandom, but I also know full well that we are likely\ncompletely broken on Plan 9 already, so this will be the least of the\nrequired fixes.\n\nRtlGenRandom is non-blocking, and as the commit message mentioned,\narc4random uses ChaCha20 in a non-blocking way on all systems I could\nfind, except MirBSD which uses RC4, also without blocking. Linux's\nCSPRNG is also non-blocking.\n\nI've also looked at Rust's getrandom crate, which provides support for\nvarious other systems, and I have no indication that any of the\ninterfaces I've provided are blocking in any way, since that crate would\nnot desire that behavior. Looking at it just now, I did notice that\nmacOS supports getentropy, so if I need to do a reroll, I'll add an\noption for that.\n\nSo I don't think we're likely to run into a problem here. If we do run\ninto systems with that problem, we can add an option to use libbsd,\nwhich provides arc4random and company (using ChaCha20). The tricky part\nis that when using libbsd, arc4random is not in (since that's\na system header file) and is instead in . However, it's\nan easy change if we run into some uncommon system where that's the\ncase.\n\nIf we don't like the test, we can avoid running it by default on the\nrisk of seeing breakage go uncaught.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"441371","messageId":"00c201d7db39$616d4990$2447dcb0$@nexbridge.com","threadId":"56915","inReplyTo":"YZQuDVMRx6hGEbpe@camp.crustytoothpaste.net","subject":"RE: [PATCH 0/2] Generate temporary files using a CSPRNG","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-11-16T22:29:06Z","receivedAt":"2021-11-16T22:29:30Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On November 16, 2021 5:18 PM, brian m. carlson wrote:\n> On 2021-11-16 at 15:44:33, Jeff King wrote:\n> > On Tue, Nov 16, 2021 at 03:35:40AM +0000, brian m. carlson wrote:\n> >\n> > > For those who are interested, I computed the probability of spurious\n> > > failure for the self-test mode like so:\n> > >\n> > > 256 * (255/256)^65536\n> > >\n> > > This Ruby one-liner estimates the probability at approximately 10^-108:\n> > >\n> > > ruby -e 'a = 255 ** 65536; b = 256 ** 65536; puts b.to_s.length -\n> a.to_s.length - 3'\n> > >\n> > > If I have made an error in the calculation, please do feel free to\n> > > point it out.\n> >\n> > Yes, I think your math is correct there.\n> >\n> > A more interesting question is whether generating 64k of PRNG bytes\n> > per test run is going to a problem for system entropy pools. For that\n> > matter, I guess the use of it for tempfiles will produce a similar\n> > burden, since we run so many commands. My understanding is that\n> modern\n> > systems will just produce infinite output for /dev/urandom, etc, but I\n> > wonder if there are any systems left where that is not true (because\n> > they have a misguided notion that they need to stir in more \"real\"\n> > entropy bits).\n> \n> I have specifically avoided invoking any sort of potentially blocking CSPRNG\n> for that reason. /dev/urandom is specifically not supposed to block, and on\n> the systems that I mentioned, the way Go uses it would indicate that it\n> should not. There is a system, which is Plan 9, where Go uses /dev/random\n> to seed an X.917 generator, and there I assume there is no /dev/urandom,\n> but I also know full well that we are likely completely broken on Plan 9\n> already, so this will be the least of the required fixes.\n> \n> RtlGenRandom is non-blocking, and as the commit message mentioned,\n> arc4random uses ChaCha20 in a non-blocking way on all systems I could find,\n> except MirBSD which uses RC4, also without blocking. Linux's CSPRNG is also\n> non-blocking.\n> \n> I've also looked at Rust's getrandom crate, which provides support for\n> various other systems, and I have no indication that any of the interfaces I've\n> provided are blocking in any way, since that crate would not desire that\n> behavior. Looking at it just now, I did notice that macOS supports\n> getentropy, so if I need to do a reroll, I'll add an option for that.\n> \n> So I don't think we're likely to run into a problem here. If we do run into\n> systems with that problem, we can add an option to use libbsd, which\n> provides arc4random and company (using ChaCha20). The tricky part is that\n> when using libbsd, arc4random is not in (since that's a system\n> header file) and is instead in . However, it's an easy change if\n> we run into some uncommon system where that's the case.\n> \n> If we don't like the test, we can avoid running it by default on the risk of\n> seeing breakage go uncaught.\n\nAdding these dependencies are also a problem. libbsd does not port to NonStop. GO is not available yet. Please stay at least somewhat POSIX-like. Begging because I do not want to lose git.\n-Randall\n\n"},{"id":"441372","messageId":"YZQzqjWMzaWVkkfP@camp.crustytoothpaste.net","threadId":"56915","inReplyTo":"009d01d7db03$354ecae0$9fec60a0$@nexbridge.com","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-11-16T22:41:46Z","receivedAt":"2021-11-16T22:41:52Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-11-16 at 16:01:20, rsbecker@nexbridge.com wrote:\n> On November 16, 2021 10:31 AM, Jeff King wrote:\n> > On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote:\n> > \n> > > The order of options is also important here. On systems with\n> > > arc4random, which is most of the BSDs, we use that, since, except on\n> > > MirBSD, it uses ChaCha20, which is extremely fast, and sits entirely\n> > > in userspace, avoiding a system call. We then prefer getrandom over\n> > > getentropy, because the former has been available longer on Linux, and\n> > > finally, if none of those are available, we use /dev/urandom, because\n> > > most Unix-like operating systems provide that API. We prefer options\n> > > that don't involve device files when possible because those work in\n> > > some restricted environments where device files may not be available.\n> > \n> > I wonder if we'll need a low-quality fallback for older systems which don't\n> > even have /dev/urandom. Because it's going to be used in such a core part of\n> > the system (tempfiles), this basically becomes a hard requirement for using\n> > Git at all.\n> > \n> > I can't say I'm excited in general to be introducing a dependency like this, just\n> > because of the portability headaches. But it may be the least bad thing\n> > (especially if we can fall back to the existing behavior).\n> > One alternative would be to build on top of the system mkstemp(), which\n> > makes it libc's problem. I'm not sure if we'd run into problems there, though.\n> \n> None of /dev/urandom, /dev/random, or mkstemp are available on some\n> platforms, including NonStop. This is not a good dependency to add.\n> One variant PRNGD is used in ia64 OpenSSL, while the CPU random\n> generator in hardware is used on x86. I cannot get behind this at all.\n> Libc is also not used in or available to our port. I am very worried\n> about this direction.\n\nI'm really not excited about a fallback here, and I specifically did not\ninclude one for that reason. I'm happy to add an appropriate dependency\non an OpenSSL or libgcrypt PRNG if you're linking against that already\n(e.g., for libcurl) or support for libbsd's arc4random or getentropy if\nthat will work on your system. For example, how are you dealing with\nTLS connections over HTTPS? That library will almost certainly provide\nthe required primitives in a straightforward and portable way.\n\nI do fundamentally believe every operating system and language\nenvironment need to provide a readily available CSPRNG in 2021,\nespecially because in the vast majority of cases, hash tables must be\nrandomized to avoid hash DoS attacks on untrusted input. I'm planning\nto look into our hash tables in the future to see if they are vulnerable\nto that kind of attack, and if so, we'll need to have a CSPRNG for basic\nsecurity reasons, and platforms that can't provide one would be subject\nto a CVE.\n\nIf we really can't find a solution, I won't object to a patch on top\nthat adds an insecure fallback, but I don't want to put my name or\nsign-off on such a patch because I think it's a mistake. But I think we\nalmost certainly can, though.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"441374","messageId":"00e001d7db40$985c61a0$c91524e0$@nexbridge.com","threadId":"56915","inReplyTo":"YZQzqjWMzaWVkkfP@camp.crustytoothpaste.net","subject":"RE: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-11-16T23:20:45Z","receivedAt":"2021-11-16T23:21:04Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On November 16, 2021 5:42 PM, brian m. carlson\n> On 2021-11-16 at 16:01:20, rsbecker@nexbridge.com wrote:\n> > On November 16, 2021 10:31 AM, Jeff King wrote:\n> > > On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote:\n> > >\n> > > > The order of options is also important here. On systems with\n> > > > arc4random, which is most of the BSDs, we use that, since, except\n> > > > on MirBSD, it uses ChaCha20, which is extremely fast, and sits\n> > > > entirely in userspace, avoiding a system call. We then prefer\n> > > > getrandom over getentropy, because the former has been available\n> > > > longer on Linux, and finally, if none of those are available, we\n> > > > use /dev/urandom, because most Unix-like operating systems provide\n> > > > that API. We prefer options that don't involve device files when\n> > > > possible because those work in some restricted environments where\n> device files may not be available.\n> > >\n> > > I wonder if we'll need a low-quality fallback for older systems\n> > > which don't even have /dev/urandom. Because it's going to be used in\n> > > such a core part of the system (tempfiles), this basically becomes a\n> > > hard requirement for using Git at all.\n> > >\n> > > I can't say I'm excited in general to be introducing a dependency\n> > > like this, just because of the portability headaches. But it may be\n> > > the least bad thing (especially if we can fall back to the existing behavior).\n> > > One alternative would be to build on top of the system mkstemp(),\n> > > which makes it libc's problem. I'm not sure if we'd run into problems\n> there, though.\n> >\n> > None of /dev/urandom, /dev/random, or mkstemp are available on some\n> > platforms, including NonStop. This is not a good dependency to add.\n> > One variant PRNGD is used in ia64 OpenSSL, while the CPU random\n> > generator in hardware is used on x86. I cannot get behind this at all.\n> > Libc is also not used in or available to our port. I am very worried\n> > about this direction.\n> \n> I'm really not excited about a fallback here, and I specifically did not include\n> one for that reason. I'm happy to add an appropriate dependency on an\n> OpenSSL or libgcrypt PRNG if you're linking against that already (e.g., for\n> libcurl) or support for libbsd's arc4random or getentropy if that will work on\n> your system. For example, how are you dealing with TLS connections over\n> HTTPS? That library will almost certainly provide the required primitives in a\n> straightforward and portable way.\n> \n> I do fundamentally believe every operating system and language\n> environment need to provide a readily available CSPRNG in 2021, especially\n> because in the vast majority of cases, hash tables must be randomized to\n> avoid hash DoS attacks on untrusted input. I'm planning to look into our hash\n> tables in the future to see if they are vulnerable to that kind of attack, and if\n> so, we'll need to have a CSPRNG for basic security reasons, and platforms\n> that can't provide one would be subject to a CVE.\n> \n> If we really can't find a solution, I won't object to a patch on top that adds an\n> insecure fallback, but I don't want to put my name or sign-off on such a patch\n> because I think it's a mistake. But I think we almost certainly can, though.\n\nWe do link with libcurl and use OpenSSL as a DLL to handle TLS. The underlying random source for the nonstop-* configurations as of OpenSSL 3.0 are PNRG supplied by the vendor (HPE) on ia64 and the hardware rdrand* instructions on x86. I know that part of the OpenSSL code rather intimately.\n--\nRandall Becker\nAlso from the GTA\n\n"},{"id":"441376","messageId":"CAPUEsphYqg4rvvrODL9yYCeL7wkjyR0RcEVaZiYQHy_2D1-oxg@mail.gmail.com","threadId":"56915","inReplyTo":"00e001d7db40$985c61a0$c91524e0$@nexbridge.com","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-17T00:47:48Z","receivedAt":"2021-11-17T00:48:08Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Nov 16, 2021 at 4:01 PM wrote:\n>\n> We do link with libcurl and use OpenSSL as a DLL to handle TLS. The underlying random source for the nonstop-* configurations as of OpenSSL 3.0 are PNRG supplied by the vendor (HPE) on ia64 and the hardware rdrand* instructions on x86. I know that part of the OpenSSL code rather intimately.\n\nOlder versions of OpenSSL exported (AFAIK) a usable version of\narc4random_buf() that could have helped here; it seems to still be\nthere in libressl[1] which is mostly API compatible and might be worth\nlooking into IMHO even if as you pointed out will need an\nimplementation similar to what OpenSSL does internally.\n\n[1] https://cvsweb.openbsd.org/src/lib/libcrypto/arc4random/\n"},{"id":"441383","messageId":"YZRUzHVS32W4Flo/@camp.crustytoothpaste.net","threadId":"56915","inReplyTo":"00e001d7db40$985c61a0$c91524e0$@nexbridge.com","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-11-17T01:03:08Z","receivedAt":"2021-11-17T01:03:14Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-11-16 at 23:20:45, rsbecker@nexbridge.com wrote:\n> We do link with libcurl and use OpenSSL as a DLL to handle TLS. The\n> underlying random source for the nonstop-* configurations as of\n> OpenSSL 3.0 are PNRG supplied by the vendor (HPE) on ia64 and the\n> hardware rdrand* instructions on x86. I know that part of the OpenSSL\n> code rather intimately.\n\nGreat, as long as you don't define NO_OPENSSL, I think I can make this\nwork with OpenSSL by calling RAND_bytes, which will use whatever OpenSSL\nuses. I'll work on that for a v2 to see if that will meet the needs for\nyour platform, and if not, I'll try something else.\n\nThat should also have the pleasant side effect of making this more\nportable even for those people who do have less common platforms, since\nOpenSSL will likely be an option there.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"441386","messageId":"CAPUEspiHnTkwbUJ5o+fT2u4Kn+fwNe-3FoqVtNsjTF+Pg6Tryg@mail.gmail.com","threadId":"56915","inReplyTo":"YZRUzHVS32W4Flo/@camp.crustytoothpaste.net","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-17T01:50:44Z","receivedAt":"2021-11-17T01:51:00Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Nov 16, 2021 at 5:04 PM brian m. carlson\n wrote:\n>\n> On 2021-11-16 at 23:20:45, rsbecker@nexbridge.com wrote:\n> > We do link with libcurl and use OpenSSL as a DLL to handle TLS. The\n> > underlying random source for the nonstop-* configurations as of\n> > OpenSSL 3.0 are PNRG supplied by the vendor (HPE) on ia64 and the\n> > hardware rdrand* instructions on x86. I know that part of the OpenSSL\n> > code rather intimately.\n>\n> Great, as long as you don't define NO_OPENSSL, I think I can make this\n> work with OpenSSL by calling RAND_bytes, which will use whatever OpenSSL\n> uses.\n\nnot that RAND_bytes return high entropy bytes (like /dev/random) and\nis therefore limited and prone to draining, blocking and erroring when\ndrained, so if we are going this route, will most likely need a second\nlayer on top that doesn't block (like arc4random does), and at that\npoint I would think we would rather use something battle tested than\nour own.\n\nfor the little amount of random data we need, it might be wiser to\nfallback to something POSIX like lrand48 which is most likely to be\navailable, but of course your tests that consume lots of random data\nwill need to change.\n\nCarlo\n\nPS. Probably missing context as I don't know what was discussed\npreviously, but indeed making this the libc problem by using mkstemp\n(plus some compatibility on top), like Peff mentioned seems like a\nmore straightforward \"fix\"\n\n\n\n I'll work on that for a v2 to see if that will meet the needs for\n> your platform, and if not, I'll try something else.\n>\n> That should also have the pleasant side effect of making this more\n> portable even for those people who do have less common platforms, since\n> OpenSSL will likely be an option there.\n> --\n> brian m. carlson (he/him or they/them)\n> Toronto, Ontario, CA\n"},{"id":"441397","messageId":"010801d7db5f$bfb25330$3f16f990$@nexbridge.com","threadId":"56915","inReplyTo":"YZRUzHVS32W4Flo/@camp.crustytoothpaste.net","subject":"RE: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-11-17T03:03:45Z","receivedAt":"2021-11-17T03:04:05Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On November 16, 2021 8:03 PM, brian m. carlson wrote:\n> On 2021-11-16 at 23:20:45, rsbecker@nexbridge.com wrote:\n> > We do link with libcurl and use OpenSSL as a DLL to handle TLS. The\n> > underlying random source for the nonstop-* configurations as of\n> > OpenSSL 3.0 are PNRG supplied by the vendor (HPE) on ia64 and the\n> > hardware rdrand* instructions on x86. I know that part of the OpenSSL\n> > code rather intimately.\n> \n> Great, as long as you don't define NO_OPENSSL, I think I can make this work\n> with OpenSSL by calling RAND_bytes, which will use whatever OpenSSL uses.\n> I'll work on that for a v2 to see if that will meet the needs for your platform,\n> and if not, I'll try something else.\n> \n> That should also have the pleasant side effect of making this more portable\n> even for those people who do have less common platforms, since OpenSSL\n> will likely be an option there.\n\nI checked config.mak.uname. We should be fine with that qualification.\n\nRegards,\nRandall\n\n"},{"id":"441398","messageId":"YZRxOrv9JFt2oeSU@coredump.intra.peff.net","threadId":"56915","inReplyTo":"CAPUEspiHnTkwbUJ5o+fT2u4Kn+fwNe-3FoqVtNsjTF+Pg6Tryg@mail.gmail.com","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-17T03:04:26Z","receivedAt":"2021-11-17T03:04:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 16, 2021 at 05:50:44PM -0800, Carlo Arenas wrote:\n\n> for the little amount of random data we need, it might be wiser to\n> fallback to something POSIX like lrand48 which is most likely to be\n> available, but of course your tests that consume lots of random data\n> will need to change.\n\nUnfortunately that won't help. You have to seed lrand48 with something,\nwhich usually means pid and/or timestamp. Which are predictable to an\nattacker, which was the start of the whole conversation. You really need\n_some_ source of entropy, and only the OS can provide that.\n\n> PS. Probably missing context as I don't know what was discussed\n> previously, but indeed making this the libc problem by using mkstemp\n> (plus some compatibility on top), like Peff mentioned seems like a\n> more straightforward \"fix\"\n\nIt might be nice if it works. I don't recall all of the reasons that led\nus to implement our own mkstemp in the first place. So the first step\nwould probably be digging in the history and the archive to find that\nout, and whether it still applies.\n\n-Peff\n"},{"id":"441399","messageId":"010901d7db5f$fdb4f570$f91ee050$@nexbridge.com","threadId":"56915","inReplyTo":"CAPUEsphYqg4rvvrODL9yYCeL7wkjyR0RcEVaZiYQHy_2D1-oxg@mail.gmail.com","subject":"RE: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-11-17T03:05:29Z","receivedAt":"2021-11-17T03:05:47Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On November 16, 2021 7:48 PM, Carlo Arenas wrote:\n> On Tue, Nov 16, 2021 at 4:01 PM wrote:\n> >\n> > We do link with libcurl and use OpenSSL as a DLL to handle TLS. The\n> underlying random source for the nonstop-* configurations as of OpenSSL\n> 3.0 are PNRG supplied by the vendor (HPE) on ia64 and the hardware\n> rdrand* instructions on x86. I know that part of the OpenSSL code rather\n> intimately.\n> \n> Older versions of OpenSSL exported (AFAIK) a usable version of\n> arc4random_buf() that could have helped here; it seems to still be there in\n> libressl[1] which is mostly API compatible and might be worth looking into\n> IMHO even if as you pointed out will need an implementation similar to what\n> OpenSSL does internally.\n> \n> [1] https://cvsweb.openbsd.org/src/lib/libcrypto/arc4random/\n\nI do not see arc4random being used in our builds going back to OpenSSL 1.0.2, which is as far back as I go anyway.\n\n"},{"id":"441400","messageId":"012201d7db60$ea5e9750$bf1bc5f0$@nexbridge.com","threadId":"56915","inReplyTo":"YZRxOrv9JFt2oeSU@coredump.intra.peff.net","subject":"RE: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-11-17T03:12:07Z","receivedAt":"2021-11-17T03:12:20Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On November 16, 2021 10:04 PM, Jeff King wrote:\n> On Tue, Nov 16, 2021 at 05:50:44PM -0800, Carlo Arenas wrote:\n> \n> > for the little amount of random data we need, it might be wiser to\n> > fallback to something POSIX like lrand48 which is most likely to be\n> > available, but of course your tests that consume lots of random data\n> > will need to change.\n> \n> Unfortunately that won't help. You have to seed lrand48 with something,\n> which usually means pid and/or timestamp. Which are predictable to an\n> attacker, which was the start of the whole conversation. You really need\n> _some_ source of entropy, and only the OS can provide that.\n> \n> > PS. Probably missing context as I don't know what was discussed\n> > previously, but indeed making this the libc problem by using mkstemp\n> > (plus some compatibility on top), like Peff mentioned seems like a\n> > more straightforward \"fix\"\n> \n> It might be nice if it works. I don't recall all of the reasons that led us to\n> implement our own mkstemp in the first place. So the first step would\n> probably be digging in the history and the archive to find that out, and\n> whether it still applies.\n\nmkstemp is more recent than mktemp and not implemented everywhere, sadly, and despite my whining about it. That may be why. It is actually available on recent NonStop platforms, so no real issue. mkstemp does allocate a file descriptor, which can be expensive and not always desired.\n--Randall\n\n"},{"id":"441402","messageId":"CAPUEsphh-enSYS66mi7_XaS0n1bmUvGXRcgVp6iqhg94xSHVog@mail.gmail.com","threadId":"56915","inReplyTo":"YZRxOrv9JFt2oeSU@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-17T03:36:51Z","receivedAt":"2021-11-17T03:37:06Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Nov 16, 2021 at 7:04 PM Jeff King wrote:\n>\n> On Tue, Nov 16, 2021 at 05:50:44PM -0800, Carlo Arenas wrote:\n>\n> > for the little amount of random data we need, it might be wiser to\n> > fallback to something POSIX like lrand48 which is most likely to be\n> > available, but of course your tests that consume lots of random data\n> > will need to change.\n>\n> Unfortunately that won't help. You have to seed lrand48 with something,\n> which usually means pid and/or timestamp. Which are predictable to an\n> attacker, which was the start of the whole conversation. You really need\n> _some_ source of entropy, and only the OS can provide that.\n\nagain, showing my ignorance here; but that \"something\" doesn't need to\nbe guessable externally; ex: git add could use as seed contents from\nthe file that is adding, or even better mix it up with the other\nsources as a poor man's /dev/urandom\n\nI agree though that having a true random source will require the OS,\nbut isn't it about generating 6 random letters?\n\nCarlo\n"},{"id":"441408","messageId":"xmqq1r3f5izn.fsf@gitster.g","threadId":"56915","inReplyTo":"20211116033542.3247094-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-17T07:39:08Z","receivedAt":"2021-11-17T07:39:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> Finally, add a self-test option here to make sure that our buffer\n> handling is correct and we aren't truncating data. We simply read 64\n> KiB and then make sure we've seen each byte. The probability of this\n> test failing spuriously is less than 10^-100.\n\nI saw that 10^-100 math in the other message, and have no problem\nwith that, but I am not sure how such a test makes \"sure that our\nbuffer handling is correct and we aren't truncating data.\" If you\nthought you are generate 64kiB of random bytes but a bug caused you\nto actually use 32kiB of random bytes with 32kiB of other garbage,\nwouldn't you still have enough entropy left that you would be likely\nto paint all 256 buckets?\n\nI also agree with Peff's comment about making these look as if many\nof them can be specified at once, when only one of them would\nactually be in effect. Giving one Makefile macro that the builder\ncan set to a single value would be much less confusing.\n\nThanks.\n"},{"id":"441423","messageId":"211117.86a6i387i1.gmgdl@evledraar.gmail.com","threadId":"56915","inReplyTo":"YZQdRC9nAA/CbMyM@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] Generate temporary files using a CSPRNG","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-17T08:36:43Z","receivedAt":"2021-11-17T09:19:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 16 2021, Jeff King wrote:\n\n> On Tue, Nov 16, 2021 at 09:35:59PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> I tried testing this codepath real quick now with:\n>> \n>> diff --git a/wrapper.c b/wrapper.c\n>> index 36e12119d76..2f3755886fb 100644\n>> --- a/wrapper.c\n>> +++ b/wrapper.c\n>> @@ -497,6 +497,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>> v /= num_letters;\n>> }\n>> \n>> + BUG(\"%s\", pattern);\n>> fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n>> if (fd >= 0)\n>> return fd;\n>> \n>> And then doing:\n>> \n>> grep BUG test-results/*.out\n>> \n>> And the resulting output is all of the form:\n>> \n>> .git/objects/9f/tmp_obj_FOzEcZ\n>> .git/objects/pack/tmp_pack_fJC0RI\n>> \n>> And a couple of:\n>> \n>> .git/info/refs_Lctaew\n>> \n>> I.e. these are all cases where we're creating in-repo tempfiles, we're\n>> not racing someone in /tmp/ for these, except perhaps in some cases I've\n>> missed (but you allude to) where we presumably should just move those\n>> into .git/tmp/, at least by default.\n>\n> Your patch is way too aggressive. By bailing via BUG(), most commands\n> will fail, so we never get to the interesting ones (e.g., we would not\n> ever get to the point of writing out a tag signature for gpg to verify,\n> because we'd barf when trying to create the tag in the first place).\n>\n> Try:\n>\n> diff --git a/wrapper.c b/wrapper.c\n> index 36e12119d7..5218a4b3bd 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -497,6 +497,10 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n> \t\t\tv /= num_letters;\n> \t\t}\n> \n> +\t\t{\n> +\t\t\tstatic struct trace_key t = TRACE_KEY_INIT(TEMPFILE);\n> +\t\t\ttrace_printf_key(&t, \"%s\", pattern);\n> +\t\t}\n> \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n> \t\tif (fd >= 0)\n> \t\t\treturn fd;\n>\n> And then:\n>\n> GIT_TRACE_TEMPFILE=/tmp/foo make test\n> grep ^/tmp /tmp/foo | wc -l\n>\n> turns up hundreds of hits.\n\nThanks, there's a long tail of these, but I came up with this crappy\none-liner one regex at a time while looking at it:\n\n cat /tmp/git_mkstemps_mode.trace | perl -pe 's[/[0-9a-f]{2}/][/HH/]; s[/incoming-\\K[^/]+][XXX]; s[/tmp/\\K[^_]+][XXX]; s/tmp_(idx|obj|pack)_\\K[a-zA-Z0-9]+$/XXX/; s[/objects/\\\nK../][$1??/]g; s[^/run/user.*/objects/][/objects/]; s[(vtag_tmp|pack_|refs_)\\K.*][XXX]; '|sort|uniq -c|sort -nr|less\n\nWhich gives us:\n\n 893 .git/objects/pack/tmp_pack_XXX\n 836 ./objects/??/tmp_obj_XXX\n 722 .git/objects/pack/tmp_idx_XXX\n 401 /objects/incoming-XXX/HH/tmp_obj_XXX\n 366 /run/user/1001/tmp/XXX_pack_XXX\n 289 /objects/??/tmp_obj_XXX\n 261 .git/info/refs_XXX\n 258 /tmp/XXX_vtag_tmpXXX\n 185 clone.git/objects/??/tmp_obj_XXX\n 77 /tmp/XXX_file\n 72 marks-test/.git/objects/??/tmp_obj_XXX\n 71 /objects/pack/tmp_pack_XXX\n 69 /objects/pack/tmp_idx_XXX\n 34 objects/pack/tmp_pack_XXX\n 34 objects/pack/tmp_idx_XXX\n 25 /run/user/1001/tmp/XXX.git/objects/??/tmp_obj_XXX\n 20 info/refs_XXX\n 12 /tmp/XXX_text\n 12 foo.git/objects/??/tmp_obj_XXX\n\nI.e. this is stuff that's either already in .git, or a small handful of\nspecial-cases such as \"git verify-tag\".\n\n>> If there are cases where we actually need this hardening because we're\n>> writing in a shared /tmp/ and not .git/, then surely we're better having\n>> those API users call a differently named function, or to move those\n>> users to using a .git/tmp/ unless they configure things otherwise?\n>\n> Assuming you can write to .git/tmp means that conceptually read-only\n> operations (like verifying tags) require write access to the repository.\n\nThat leaves the \"differently named function\" which I think we should\nreally do in either case.\n\nI.e. if I'm verifying lots of tags then I'm better off on a modern\nsystemd system using /run/user/`id -u`, as opposed to /tmp/ which is\noften disk-backed. So being aware of $XDG_RUNTIME_DIR seems like a\nsensible thing in either case.\n\nAnd on those systems the DoS aspect of this becomes a non-issue, that\ndirectory is only writable by one (non-super)user.\n\nI think there's a big advantage to having any tricky CSPRNG-implementing\ncode in its own corner like that.\n\nIt means that e.g. if gpg learns some mode to do this that doesn't\nrequire tempfiles, and we're confident we don't create things in /tmp\notherwise that we could drop it, or users who don't want git shipping a\nCSPRNG can compile it out.\n\nBut I really don't see why it isn't an acceptable solution for git to\njust die here if we fail to create the Nth tempfile in a row.\n\nOr something simpler like having the \"git verify-tag\" code fall back to\nwriting in say $HOME/.cache/git, which is another simple way to avoid\nthe issue entirely in most cases.\n"},{"id":"441523","messageId":"YZVfrhos+lZas7hk@coredump.intra.peff.net","threadId":"56915","inReplyTo":"CAPUEsphh-enSYS66mi7_XaS0n1bmUvGXRcgVp6iqhg94xSHVog@mail.gmail.com","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-17T20:01:50Z","receivedAt":"2021-11-17T20:02:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 16, 2021 at 07:36:51PM -0800, Carlo Arenas wrote:\n\n> > > for the little amount of random data we need, it might be wiser to\n> > > fallback to something POSIX like lrand48 which is most likely to be\n> > > available, but of course your tests that consume lots of random data\n> > > will need to change.\n> >\n> > Unfortunately that won't help. You have to seed lrand48 with something,\n> > which usually means pid and/or timestamp. Which are predictable to an\n> > attacker, which was the start of the whole conversation. You really need\n> > _some_ source of entropy, and only the OS can provide that.\n> \n> again, showing my ignorance here; but that \"something\" doesn't need to\n> be guessable externally; ex: git add could use as seed contents from\n> the file that is adding, or even better mix it up with the other\n> sources as a poor man's /dev/urandom\n\nThose contents are still predictable. So you've made the attacker's job\na little harder (now they have to block tempfiles for, say, each tag\nyou're going to verify), but haven't changed the fundamental problem.\n\nIt definitely would help in _some_ threat models, but I think we should\nstrive for a solution that can be explained clearly as \"nobody can DoS\nyour tempfiles\" without complicated qualifications.\n\n-Peff\n"},{"id":"441524","messageId":"01a501d7dbf0$7c538000$74fa8000$@nexbridge.com","threadId":"56915","inReplyTo":"YZVfrhos+lZas7hk@coredump.intra.peff.net","subject":"RE: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-11-17T20:19:49Z","receivedAt":"2021-11-17T20:20:21Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On November 17, 2021 3:02 PM, Jeff King wrote:\n> On Tue, Nov 16, 2021 at 07:36:51PM -0800, Carlo Arenas wrote:\n> \n> > > > for the little amount of random data we need, it might be wiser to\n> > > > fallback to something POSIX like lrand48 which is most likely to\n> > > > be available, but of course your tests that consume lots of random\n> > > > data will need to change.\n> > >\n> > > Unfortunately that won't help. You have to seed lrand48 with\n> > > something, which usually means pid and/or timestamp. Which are\n> > > predictable to an attacker, which was the start of the whole\n> > > conversation. You really need _some_ source of entropy, and only the OS\n> can provide that.\n> >\n> > again, showing my ignorance here; but that \"something\" doesn't need to\n> > be guessable externally; ex: git add could use as seed contents from\n> > the file that is adding, or even better mix it up with the other\n> > sources as a poor man's /dev/urandom\n> \n> Those contents are still predictable. So you've made the attacker's job a little\n> harder (now they have to block tempfiles for, say, each tag you're going to\n> verify), but haven't changed the fundamental problem.\n> \n> It definitely would help in _some_ threat models, but I think we should strive\n> for a solution that can be explained clearly as \"nobody can DoS your\n> tempfiles\" without complicated qualifications.\n\nI missed this one... lrand48 is also not generally available. I don’t think it is even available on Windows.\n\nIf we need a generalized solution, it probably needs to be abstracted in git-compat-util.h and compat/rand.[ch], so that the platform maintainers can plug in whatever decent platform randomization happens to be available, if any. We know that rand() is vulnerable, but it might be the only generally available fallback. Perhaps get the compat layer in place with a test suite that exercises the implementation before getting into the general git code base - maybe based on jitterentropy or sslrng. Agree on an interface, decide on a period of time to implement, send the word out that this needs to get done, and hope for the best. I have code that passes FIPS-140 for NonStop ia64 (-ish although not jitterentropy) and x86, and I'm happy to contribute some of this.\n\nRandall\n\n"},{"id":"441530","messageId":"YZWJy16Emrkr6qVL@camp.crustytoothpaste.net","threadId":"56915","inReplyTo":"xmqq1r3f5izn.fsf@gitster.g","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-11-17T23:01:31Z","receivedAt":"2021-11-17T23:01:38Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-11-17 at 07:39:08, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n> > Finally, add a self-test option here to make sure that our buffer\n> > handling is correct and we aren't truncating data. We simply read 64\n> > KiB and then make sure we've seen each byte. The probability of this\n> > test failing spuriously is less than 10^-100.\n> \n> I saw that 10^-100 math in the other message, and have no problem\n> with that, but I am not sure how such a test makes \"sure that our\n> buffer handling is correct and we aren't truncating data.\" If you\n> thought you are generate 64kiB of random bytes but a bug caused you\n> to actually use 32kiB of random bytes with 32kiB of other garbage,\n> wouldn't you still have enough entropy left that you would be likely\n> to paint all 256 buckets?\n\nTrue, but our code processes smaller chunks at a time, which means that\ntheoretically we'd notice before then. For example, getentropy(2) won't\nprocess chunks larger than 256 bytes.\n\nIf we don't think there's value, I can just remove it.\n\n> I also agree with Peff's comment about making these look as if many\n> of them can be specified at once, when only one of them would\n> actually be in effect. Giving one Makefile macro that the builder\n> can set to a single value would be much less confusing.\n\nI can use one Makefile macro, sure. I think we'll still need multiple\nmacros for the actual C code because we can't really do a string\ncomparison in the C preprocessor.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"441534","messageId":"YZWQroXwOKIi8Zs4@camp.crustytoothpaste.net","threadId":"56915","inReplyTo":"01a501d7dbf0$7c538000$74fa8000$@nexbridge.com","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-11-17T23:30:54Z","receivedAt":"2021-11-17T23:30:58Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-11-17 at 20:19:49, rsbecker@nexbridge.com wrote:\n> I missed this one... lrand48 is also not generally available. I don’t think it is even available on Windows.\n> \n> If we need a generalized solution, it probably needs to be abstracted in git-compat-util.h and compat/rand.[ch], so that the platform maintainers can plug in whatever decent platform randomization happens to be available, if any. We know that rand() is vulnerable, but it might be the only generally available fallback. Perhaps get the compat layer in place with a test suite that exercises the implementation before getting into the general git code base - maybe based on jitterentropy or sslrng. Agree on an interface, decide on a period of time to implement, send the word out that this needs to get done, and hope for the best. I have code that passes FIPS-140 for NonStop ia64 (-ish although not jitterentropy) and x86, and I'm happy to contribute some of this.\n\nI think in this case I'd like to try to stick with OpenSSL or other\nstandard interfaces if that's going to meet folks' needs. I can write\nan HMAC-DRBG, but getting entropy is the tricky part, and jitterentropy\napproaches are controversial because it's not clear how unpredictable\nthey are. I'm also specifically trying to avoid anything that's\narchitecture specific like RDRAND, since that means we have to carry\nassembly code, and on some systems RDRAND is broken, which means that\nyou have to test for that and then pass the output into another CSPRNG.\nI'm also not sure how maintainable such code is, since I don't think\nthere are many people on the list who would be familiar enough with\nthose algorithms to maintain it. Plus there's always the rule, \"Don't\nwrite your own crypto.\"\n\nUsing OpenSSL or system-provided interfaces is much, much easier, it\nmeans users can use Git in FIPS-certified environments, and it avoids us\nending up with subtly broken code in the future.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"441536","messageId":"01de01d7dc0b$bd3ab050$37b010f0$@nexbridge.com","threadId":"56915","inReplyTo":"YZWQroXwOKIi8Zs4@camp.crustytoothpaste.net","subject":"RE: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-11-17T23:34:55Z","receivedAt":"2021-11-17T23:35:31Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On November 17, 2021 6:31 PM, brian m. carlson wrote:\n> To: rsbecker@nexbridge.com\n> Cc: 'Jeff King' ; 'Carlo Arenas' ;\n> git@vger.kernel.org\n> Subject: Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a\n> CSPRNG\n> \n> On 2021-11-17 at 20:19:49, rsbecker@nexbridge.com wrote:\n> > I missed this one... lrand48 is also not generally available. I don’t think it is\n> even available on Windows.\n> >\n> > If we need a generalized solution, it probably needs to be abstracted in git-\n> compat-util.h and compat/rand.[ch], so that the platform maintainers can\n> plug in whatever decent platform randomization happens to be available, if\n> any. We know that rand() is vulnerable, but it might be the only generally\n> available fallback. Perhaps get the compat layer in place with a test suite that\n> exercises the implementation before getting into the general git code base -\n> maybe based on jitterentropy or sslrng. Agree on an interface, decide on a\n> period of time to implement, send the word out that this needs to get done,\n> and hope for the best. I have code that passes FIPS-140 for NonStop ia64 (-\n> ish although not jitterentropy) and x86, and I'm happy to contribute some of\n> this.\n> \n> I think in this case I'd like to try to stick with OpenSSL or other standard\n> interfaces if that's going to meet folks' needs. I can write an HMAC-DRBG,\n> but getting entropy is the tricky part, and jitterentropy approaches are\n> controversial because it's not clear how unpredictable they are. I'm also\n> specifically trying to avoid anything that's architecture specific like RDRAND,\n> since that means we have to carry assembly code, and on some systems\n> RDRAND is broken, which means that you have to test for that and then pass\n> the output into another CSPRNG.\n> I'm also not sure how maintainable such code is, since I don't think there are\n> many people on the list who would be familiar enough with those algorithms\n> to maintain it. Plus there's always the rule, \"Don't write your own crypto.\"\n> \n> Using OpenSSL or system-provided interfaces is much, much easier, it means\n> users can use Git in FIPS-certified environments, and it avoids us ending up\n> with subtly broken code in the future.\n\nI agree wholeheartedly. git in FIPS-certified environments is one of my actual goals - well, in this case, I am a proxy for my customers'. Sticking with OpenSSL would be far preferable to me than basically reimplementing what OpenSSL does. Even OpenSSH uses OpenSSL.\n\nRegards,\nRandall\n\n"},{"id":"441578","messageId":"xmqqv90qx76b.fsf@gitster.g","threadId":"56915","inReplyTo":"YZWJy16Emrkr6qVL@camp.crustytoothpaste.net","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-18T07:19:08Z","receivedAt":"2021-11-18T07:19:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On 2021-11-17 at 07:39:08, Junio C Hamano wrote:\n>> \"brian m. carlson\" writes:\n>> \n>> > Finally, add a self-test option here to make sure that our buffer\n>> > handling is correct and we aren't truncating data. We simply read 64\n>> > KiB and then make sure we've seen each byte. The probability of this\n>> > test failing spuriously is less than 10^-100.\n>> \n>> I saw that 10^-100 math in the other message, and have no problem\n>> with that, but I am not sure how such a test makes \"sure that our\n>> buffer handling is correct and we aren't truncating data.\" If you\n>> thought you are generate 64kiB of random bytes but a bug caused you\n>> to actually use 32kiB of random bytes with 32kiB of other garbage,\n>> wouldn't you still have enough entropy left that you would be likely\n>> to paint all 256 buckets?\n>\n> True, but our code processes smaller chunks at a time, which means that\n> theoretically we'd notice before then. For example, getentropy(2) won't\n> process chunks larger than 256 bytes.\n\nSorry, you lost me.\n\n> If we don't think there's value, I can just remove it.\n\nIt is not that I do not think there is value. I am not sure where\nthis code is getting its value from.\n\nWe grab 1k at a time and repeat that 64 times. \n\nPresumably csprn_bytes() grabs bytes from underlying mechanism in\nsmaller chunk, but would not return until it fills the buffer---ah,\nyour \"make sure our buffer handling is correct\" is primarily about\nthe check that we get full 1k bytes in the loop? We ask 1k chunk 64\ntimes and we must get full 1k chunk every time?\n\nWhat I was wondering about was the other half of the check, ensuring\nall buckets[] are painted that gave us the cute 10^-100 math.\n\n+\tint buckets[256] = { 0 };\n+\tunsigned char buf[1024];\n+\tunsigned long count = 64 * 1024;\n+\tint i;\n+\n+\twhile (count) {\n+\t\tif (csprng_bytes(buf, sizeof(buf)) < 0) {\n+\t\t\tperror(\"failed to read\");\n+\t\t\treturn 3;\n+\t\t}\n+\t\tfor (i = 0; i < sizeof(buf); i++)\n+\t\t\tbuckets[buf[i]]++;\n+\t\tcount -= sizeof(buf);\n+\t}\n"},{"id":"441669","messageId":"YZbQu3fwfRsdEeXR@camp.crustytoothpaste.net","threadId":"56915","inReplyTo":"xmqqv90qx76b.fsf@gitster.g","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-11-18T22:16:27Z","receivedAt":"2021-11-18T22:16:32Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-11-18 at 07:19:08, Junio C Hamano wrote:\n> Presumably csprn_bytes() grabs bytes from underlying mechanism in\n> smaller chunk, but would not return until it fills the buffer---ah,\n> your \"make sure our buffer handling is correct\" is primarily about\n> the check that we get full 1k bytes in the loop? We ask 1k chunk 64\n> times and we must get full 1k chunk every time?\n\nYes, that's what we'd expect to happen.\n\n> What I was wondering about was the other half of the check, ensuring\n> all buckets[] are painted that gave us the cute 10^-100 math.\n\nSay the buffer handling is incorrect and we read only a few bytes\ninstead of the full 1 KiB. Then we'll end up filling only some of the\nbuckets, and the check will fail much of the time, because we won't get\nsufficient number of random bytes to fill all the buckets.\n\nThe check is that we got enough data that looks like random bytes over\nthe course of our requests.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"441924","messageId":"xmqq5yskk131.fsf@gitster.g","threadId":"56915","inReplyTo":"YZbQu3fwfRsdEeXR@camp.crustytoothpaste.net","subject":"Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a CSPRNG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-22T09:10:26Z","receivedAt":"2021-11-22T09:10:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On 2021-11-18 at 07:19:08, Junio C Hamano wrote:\n>> Presumably csprn_bytes() grabs bytes from underlying mechanism in\n>> smaller chunk, but would not return until it fills the buffer---ah,\n>> your \"make sure our buffer handling is correct\" is primarily about\n>> the check that we get full 1k bytes in the loop? We ask 1k chunk 64\n>> times and we must get full 1k chunk every time?\n>\n> Yes, that's what we'd expect to happen.\n>\n>> What I was wondering about was the other half of the check, ensuring\n>> all buckets[] are painted that gave us the cute 10^-100 math.\n>\n> Say the buffer handling is incorrect and we read only a few bytes\n> instead of the full 1 KiB. Then we'll end up filling only some of the\n> buckets, and the check will fail much of the time, because we won't get\n> sufficient number of random bytes to fill all the buckets.\n\n... meaning (64 * a few bytes) is small enough such that some slots\nin buckets[] will be left untouched (and the remainder of 1kB is\nuntouched --- but the buffer[] is not initialized in any way, so\nit's not like such an \"oops, we only fed a few bytes\" bug would\nleave the rest to NUL or anything)?\n\n> The check is that we got enough data that looks like random bytes over\n> the course of our requests.\n\nIf the check were doing so, yes, I would have understood (whether I\nagreed with it or not), but the check is \"if we taint each and every\nbucket[] even once, we are OK\", not \"bucket[] should be more or less\nevenly touched\", and that is why I do/did not understand the test.\n"}]}