{"thread":{"id":"62759","subject":"[PATCH 0/2] reftable/stack: stop dying on exhausted entropy pool","startedAt":"2025-01-07T15:27:10Z","lastAt":"2025-01-08T22:44:56Z","messageCount":19,"participants":["Patrick Steinhardt","rsbecker@nexbridge.com","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"510084","messageId":"20250107-b4-pks-reftable-csprng-v1-0-6109a54a8756@pks.im","threadId":"62759","inReplyTo":null,"subject":"[PATCH 0/2] reftable/stack: stop dying on exhausted entropy pool","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-07T15:26:58Z","receivedAt":"2025-01-07T15:27:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series fixes the issue reported by Randall [1], where\nan exhausted entropy pool can cause us to die when writing a new table\nto the reftable stack. I _think_ that this is only an issue with the\nOpenSSL backend of `csprng_bytes()`:\n\n  - `arc4random_buf()` never returns an error.\n\n  - `getrandom()` pulls from \"/dev/urandom\" by default.\n\n  - `getentropy()` seems to block when there is not enough randomness\n    available.\n\n  - `GtlGenRandom()` I cannot really tell.\n\n  - The fallback reads from \"/dev/urandom\", which also returns bytes in\n    case the entropy pool is drained.\n\nSo OpenSSL's `RAND_bytes()` seems to be the only one that returns an\nerror when the entropy pool is empty. I did wonder whether we even need\nto introduce the new flag in the first place, or whether we cannot just\nuse `RAND_pseudo_bytes()` unconditionally. But I'm a bit uneasy about it\ngiven that OpenSSL has this doc:\n\n    RAND_pseudo_bytes() puts num pseudo-random bytes into buf.\n    Pseudo-random byte sequences generated by RAND_pseudo_bytes() will\n    be unique if they are of sufficient length, but are not necessarily\n    unpredictable. They can be used for non-cryptographic purposes and\n    for certain purposes in cryptographic protocols, but usually not for\n    key generation etc.\n\nIt might be too easy to accidentally rely on `csprng_bytes()` where it\nactually requires strong cryptographic data, so I was erring on the side\nof caution.\n\nThanks!\n\n---\nPatrick Steinhardt (2):\n      wrapper: allow generating insecure random bytes\n      reftable/stack: accept insecure random bytes\n\n builtin/gc.c                        |  2 +-\n reftable/stack.c                    |  4 ++--\n t/helper/test-csprng.c              |  2 +-\n t/unit-tests/t-reftable-readwrite.c |  6 +++---\n wrapper.c                           | 24 ++++++++++++++----------\n wrapper.h                           | 16 ++++++++++++----\n 6 files changed, 33 insertions(+), 21 deletions(-)\n\n\n---\nbase-commit: b74ff38af58464688b211140b90ec90598d340c6\nchange-id: 20250107-b4-pks-reftable-csprng-9ed4e8dd83c4\n\n"},{"id":"510086","messageId":"20250107-b4-pks-reftable-csprng-v1-1-6109a54a8756@pks.im","threadId":"62759","inReplyTo":"20250107-b4-pks-reftable-csprng-v1-0-6109a54a8756@pks.im","subject":"[PATCH 1/2] wrapper: allow generating insecure random bytes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-07T15:26:59Z","receivedAt":"2025-01-07T15:27:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `csprng_bytes()` function generates randomness and writes it into a\ncaller-provided buffer. It abstracts over a couple of implementations,\nwhere the exact one that is used depends on the platform.\n\nThese implementations have different guarantees: while some guarantee to\nnever fail (arc4random(3)), others may fail. There are two significant\nfailures to distinguish from one another:\n\n  - Systemic failure, where e.g. opening \"/dev/urandom\" fails or when\n    OpenSSL doesn't have a provider configured.\n\n  - Entropy failure, where the entropy pool is exhausted, and thus the\n    function cannot guarantee strong cryptographic randomness.\n\nWhile we cannot do anything about the former, the latter failure can be\nacceptable in some situations where we don't care whether or not the\nrandomness can be predicted.\n\nIntroduce a new `CSPRNG_BYTES_INSECURE` flag that allows callers to opt\ninto weak cryptographic randomness. The exact behaviour of the flag\ndepends on the underlying implementation:\n\n    - `arc4random_buf()` never returns an error, so it doesn't change.\n\n    - `getrandom()` pulls from \"/dev/urandom\" by default, which never\n      blocks on modern systems even when the entropy pool is empty.\n\n    - `getentropy()` seems to block when there is not enough randomness\n      available, and there is no way of changing that behaviour.\n\n    - `GtlGenRandom()` doesn't mention anything about its specific\n      failure mode.\n\n    - The fallback reads from \"/dev/urandom\", which also returns bytes in\n      case the entropy pool is drained in modern Linux systems.\n\nThat only leaves OpenSSL with `RAND_bytes()`, which returns an error in\ncase the returned data wouldn't be cryptographically safe. This function\nis replaced with a call to `RAND_pseudo_bytes()`, which can indicate\nwhether or not the returned data is cryptographically secure via its\nreturn value. If it is insecure, and if the `CSPRNG_BYTES_INSECURE` flag\nis set, then we ignore the insecurity and return the data regardless.\n\nIt is somewhat questionable whether we really need the flag in the first\nplace, or whether we wouldn't just ignore the potentially-insecure data.\nBut the risk of doing that is that we might have or grow callsites that\naren't aware of the potential insecureness of the data in places where\nit really matters. So using a flag to opt-in to that behaviour feels\nlike the more secure choice.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/gc.c                        |  2 +-\n reftable/stack.c                    |  2 +-\n t/helper/test-csprng.c              |  2 +-\n t/unit-tests/t-reftable-readwrite.c |  6 +++---\n wrapper.c                           | 24 ++++++++++++++----------\n wrapper.h                           | 16 ++++++++++++----\n 6 files changed, 32 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex a9b1c36de27da2ffb16aaa45e979cbaeec18bfa1..3e754f25bba2aa16fbae9b2b6229961a53409395 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1909,7 +1909,7 @@ static int get_random_minute(void)\n \tif (getenv(\"GIT_TEST_MAINT_SCHEDULER\"))\n \t\treturn 13;\n \n-\treturn git_rand() % 60;\n+\treturn git_rand(0) % 60;\n }\n \n static int is_launchctl_available(void)\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 531660a49f0948c33041831ee0d740feacb22b2f..6d0aa774e7e29d5366ed55df19725944f8eef792 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -659,7 +659,7 @@ int reftable_stack_add(struct reftable_stack *st,\n static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n {\n \tchar buf[100];\n-\tuint32_t rnd = (uint32_t)git_rand();\n+\tuint32_t rnd = (uint32_t)git_rand(0);\n \tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n \t\t min, max, rnd);\n \treftable_buf_reset(dest);\ndiff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c\nindex a4a0aca61773b0b30de618955e5a5b61bba2d0cc..c86dcc4870fed8ab81aa7c6c17d77c0b32101cf8 100644\n--- a/t/helper/test-csprng.c\n+++ b/t/helper/test-csprng.c\n@@ -15,7 +15,7 @@ int cmd__csprng(int argc, const char **argv)\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\tif (csprng_bytes(buf, chunk, 0) < 0) {\n \t\t\tperror(\"failed to read\");\n \t\t\treturn 5;\n \t\t}\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 6b75a419b9d3bbfb720a3c39f741f940dfcd56aa..f22b9775639e3ec9167336100753d5736e5a2957 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -108,8 +108,8 @@ static void t_log_buffer_size(void)\n \t   hash, to ensure that the compressed part is larger than the original.\n \t*/\n \tfor (i = 0; i < REFTABLE_HASH_SIZE_SHA1; i++) {\n-\t\tlog.value.update.old_hash[i] = (uint8_t)(git_rand() % 256);\n-\t\tlog.value.update.new_hash[i] = (uint8_t)(git_rand() % 256);\n+\t\tlog.value.update.old_hash[i] = (uint8_t)(git_rand(0) % 256);\n+\t\tlog.value.update.new_hash[i] = (uint8_t)(git_rand(0) % 256);\n \t}\n \treftable_writer_set_limits(w, update_index, update_index);\n \terr = reftable_writer_add_log(w, &log);\n@@ -325,7 +325,7 @@ static void t_log_zlib_corruption(void)\n \t};\n \n \tfor (i = 0; i < sizeof(message) - 1; i++)\n-\t\tmessage[i] = (uint8_t)(git_rand() % 64 + ' ');\n+\t\tmessage[i] = (uint8_t)(git_rand(0) % 64 + ' ');\n \n \treftable_writer_set_limits(w, 1, 1);\n \ndiff --git a/wrapper.c b/wrapper.c\nindex fa79fd6ec9e144509f34e007bb2ffa458d69e695..8b985931490d62acb2030c147b8728b34917df8a 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -479,7 +479,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \tfor (count = 0; count < TMP_MAX; ++count) {\n \t\tint i;\n \t\tuint64_t v;\n-\t\tif (csprng_bytes(&v, sizeof(v)) < 0)\n+\t\tif (csprng_bytes(&v, sizeof(v), 0) < 0)\n \t\t\treturn error_errno(\"unable to get random bytes for temporary file\");\n \n \t\t/* Fill in the random bits. */\n@@ -750,7 +750,7 @@ int open_nofollow(const char *path, int flags)\n #endif\n }\n \n-int csprng_bytes(void *buf, size_t len)\n+int csprng_bytes(void *buf, size_t len, MAYBE_UNUSED unsigned flags)\n {\n #if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD)\n \t/* This function never returns an error. */\n@@ -785,14 +785,18 @@ int csprng_bytes(void *buf, size_t len)\n \t\treturn -1;\n \treturn 0;\n #elif defined(HAVE_OPENSSL_CSPRNG)\n-\tint res = RAND_bytes(buf, len);\n-\tif (res == 1)\n+\tswitch (RAND_pseudo_bytes(buf, len)) {\n+\tcase 1:\n \t\treturn 0;\n-\tif (res == -1)\n-\t\terrno = ENOTSUP;\n-\telse\n+\tcase 0:\n+\t\tif (flags & CSPRNG_BYTES_INSECURE)\n+\t\t\treturn 0;\n \t\terrno = EIO;\n-\treturn -1;\n+\t\treturn -1;\n+\tdefault:\n+\t\terrno = ENOTSUP;\n+\t\treturn -1;\n+\t}\n #else\n \tssize_t res;\n \tchar *p = buf;\n@@ -816,11 +820,11 @@ int csprng_bytes(void *buf, size_t len)\n #endif\n }\n \n-uint32_t git_rand(void)\n+uint32_t git_rand(unsigned flags)\n {\n \tuint32_t result;\n \n-\tif (csprng_bytes(&result, sizeof(result)) < 0)\n+\tif (csprng_bytes(&result, sizeof(result), flags) < 0)\n \t\tdie(_(\"unable to get random bytes\"));\n \n \treturn result;\ndiff --git a/wrapper.h b/wrapper.h\nindex a6b3e1f09ecdef72fdcc4a234df7a176c4daded6..7df824e34a906e9f822ad3bee27b8b1b0000621b 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -127,18 +127,26 @@ int open_nofollow(const char *path, int flags);\n \n void sleep_millisec(int millisec);\n \n+enum {\n+\t/*\n+\t * Accept insecure bytes, which some CSPRNG implementations may return\n+\t * in case the entropy pool has been exhausted.\n+\t */\n+\tCSPRNG_BYTES_INSECURE = (1 << 0),\n+};\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+ * satisfy the full request is an error. Accepts CSPRNG flags.\n  */\n-int csprng_bytes(void *buf, size_t len);\n+int csprng_bytes(void *buf, size_t len, unsigned flags);\n \n /*\n  * Returns a random uint32_t, uniformly distributed across all possible\n- * values.\n+ * values. Accepts CSPRNG flags.\n  */\n-uint32_t git_rand(void);\n+uint32_t git_rand(unsigned flags);\n \n /* Provide log2 of the given `size_t`. */\n static inline unsigned log2u(uintmax_t sz)\n\n-- \n2.48.0.rc1.245.gb3e6e7acbc.dirty\n\n"},{"id":"510085","messageId":"20250107-b4-pks-reftable-csprng-v1-2-6109a54a8756@pks.im","threadId":"62759","inReplyTo":"20250107-b4-pks-reftable-csprng-v1-0-6109a54a8756@pks.im","subject":"[PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-07T15:27:00Z","receivedAt":"2025-01-07T15:27:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reftable library uses randomness in two call paths:\n\n  - When reading a stack in case some of the referenced tables\n    disappears. The randomness is used to delay the next read by a\n    couple of milliseconds.\n\n  - When writing a new table, where the randomness gets appended to the\n    table name (e.g. \"0x000000000001-0x000000000002-0b1d8ddf.ref\").\n\nIn neither of these cases do we need strong randomness.\n\nUnfortunately though, we have observed test failures caused by the\nformer case. In t0610 we have a test that spawns a 100 processes at\nonce, all of which try to write a new table to the stack. And given that\nall of the processes will require randomness, it can happen that these\nprocesses make the entropy pool run dry, which will then cause us to\ndie:\n\n    + test_seq 100\n    + printf %s commit\\trefs/heads/branch-%s\\n\n    68d032e9edd3481ac96382786ececc37ec28709e 1\n    + printf %s commit\\trefs/heads/branch-%s\\n\n    68d032e9edd3481ac96382786ececc37ec28709e 2\n    ...\n    + git update-ref refs/heads/branch-98 HEAD\n    + git update-ref refs/heads/branch-97 HEAD\n    + git update-ref refs/heads/branch-99 HEAD\n    + git update-ref refs/heads/branch-100 HEAD\n    fatal: unable to get random bytes\n    fatal: unable to get random bytes\n    fatal: unable to get random bytes\n    fatal: unable to get random bytes\n    fatal: unable to get random bytes\n    fatal: unable to get random bytes\n    fatal: unable to get random bytes\n\nThe report was for NonStop, which uses OpenSSL as the backend for\nrandomness. In the preceding commit we have adapted that backend to also\nreturn randomness in case the entropy pool is empty and the caller\npasses the `CSPRNG_BYTES_INSECURE` flag. Do so to fix the issue.\n\nReported-by: Randall S. Becker <rsbecker@nexbridge.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 6d0aa774e7e29d5366ed55df19725944f8eef792..572a74e00f9ed6040534e060652e72c26641749d 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -493,7 +493,7 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\tclose(fd);\n \t\tfd = -1;\n \n-\t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n+\t\tdelay = delay + (delay * git_rand(CSPRNG_BYTES_INSECURE)) / UINT32_MAX + 1;\n \t\tsleep_millisec(delay);\n \t}\n \n@@ -659,7 +659,7 @@ int reftable_stack_add(struct reftable_stack *st,\n static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n {\n \tchar buf[100];\n-\tuint32_t rnd = (uint32_t)git_rand(0);\n+\tuint32_t rnd = git_rand(CSPRNG_BYTES_INSECURE);\n \tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n \t\t min, max, rnd);\n \treftable_buf_reset(dest);\n\n-- \n2.48.0.rc1.245.gb3e6e7acbc.dirty\n\n"},{"id":"510088","messageId":"005d01db611a$19ed55f0$4dc801d0$@nexbridge.com","threadId":"62759","inReplyTo":"20250107-b4-pks-reftable-csprng-v1-2-6109a54a8756@pks.im","subject":"RE: [PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2025-01-07T15:37:41Z","receivedAt":"2025-01-07T15:37:52Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On January 7, 2025 10:27 AM, Patrick Steinhardt wrote:\n>The reftable library uses randomness in two call paths:\n>\n>  - When reading a stack in case some of the referenced tables\n>    disappears. The randomness is used to delay the next read by a\n>    couple of milliseconds.\n>\n>  - When writing a new table, where the randomness gets appended to the\n>    table name (e.g. \"0x000000000001-0x000000000002-0b1d8ddf.ref\").\n>\n>In neither of these cases do we need strong randomness.\n>\n>Unfortunately though, we have observed test failures caused by the former case. In\n>t0610 we have a test that spawns a 100 processes at once, all of which try to write\n>a new table to the stack. And given that all of the processes will require\n>randomness, it can happen that these processes make the entropy pool run dry,\n>which will then cause us to\n>die:\n>\n>    + test_seq 100\n>    + printf %s commit\\trefs/heads/branch-%s\\n\n>    68d032e9edd3481ac96382786ececc37ec28709e 1\n>    + printf %s commit\\trefs/heads/branch-%s\\n\n>    68d032e9edd3481ac96382786ececc37ec28709e 2\n>    ...\n>    + git update-ref refs/heads/branch-98 HEAD\n>    + git update-ref refs/heads/branch-97 HEAD\n>    + git update-ref refs/heads/branch-99 HEAD\n>    + git update-ref refs/heads/branch-100 HEAD\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>\n>The report was for NonStop, which uses OpenSSL as the backend for randomness.\n>In the preceding commit we have adapted that backend to also return randomness\n>in case the entropy pool is empty and the caller passes the\n>`CSPRNG_BYTES_INSECURE` flag. Do so to fix the issue.\n>\n>Reported-by: Randall S. Becker <rsbecker@nexbridge.com>\n>Signed-off-by: Patrick Steinhardt <ps@pks.im>\n>---\n> reftable/stack.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n>diff --git a/reftable/stack.c b/reftable/stack.c index\n>6d0aa774e7e29d5366ed55df19725944f8eef792..572a74e00f9ed6040534e060\n>652e72c26641749d 100644\n>--- a/reftable/stack.c\n>+++ b/reftable/stack.c\n>@@ -493,7 +493,7 @@ static int reftable_stack_reload_maybe_reuse(struct\n>reftable_stack *st,\n> \t\tclose(fd);\n> \t\tfd = -1;\n>\n>-\t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n>+\t\tdelay = delay + (delay * git_rand(CSPRNG_BYTES_INSECURE)) /\n>+UINT32_MAX + 1;\n> \t\tsleep_millisec(delay);\n> \t}\n>\n>@@ -659,7 +659,7 @@ int reftable_stack_add(struct reftable_stack *st,  static int\n>format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)  {\n> \tchar buf[100];\n>-\tuint32_t rnd = (uint32_t)git_rand(0);\n>+\tuint32_t rnd = git_rand(CSPRNG_BYTES_INSECURE);\n> \tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n> \t\t min, max, rnd);\n> \treftable_buf_reset(dest);\n\nI think this is a good catch. We are seeing other tests fail similarly through this path. I will retest rc3\non both platforms since we cannot exhaust randomness on x86 using the hardware randomizer\ninside OpenSSL.\n\n"},{"id":"510114","messageId":"xmqqzfk2qr62.fsf@gitster.g","threadId":"62759","inReplyTo":"20250107-b4-pks-reftable-csprng-v1-2-6109a54a8756@pks.im","subject":"Re: [PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-07T20:56:21Z","receivedAt":"2025-01-07T20:56:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The report was for NonStop, which uses OpenSSL as the backend for\n> randomness. In the preceding commit we have adapted that backend to also\n> return randomness in case the entropy pool is empty and the caller\n> passes the `CSPRNG_BYTES_INSECURE` flag. Do so to fix the issue.\n\nNo kidding.\n\nThis is calling rand(3).  The use of the resulting value is to fuzz\nthe delay before retrying, which wants NO CRYPTOGRAPHIC randomness.\nThis callsite does not require it, but rand(3) is perfect for ensuring\npredictability/repeatability as well (via srand(3)).\n\nAnd it is not allowed to fail.\n\nYet a platform replaces it with a function that returns an error or\naborts?  What kind of nonsense is that?  Do we really need to cater\nto such an insanity?\n\nUse of git_rand() here goes backwards against the more recent trend\nin reftable/ directory to wean the code off of the rest of Git by\ngetting rid of unnecessary dependency, doesn't it?\n\nI think [PATCH 1/2] makes sense regardless, though.  But shouldn't\nwe be pushing back this step, with \"fix your rand(3)\"?\n\nThanks.\n\n> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  reftable/stack.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 6d0aa774e7e29d5366ed55df19725944f8eef792..572a74e00f9ed6040534e060652e72c26641749d 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -493,7 +493,7 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n>  \t\tclose(fd);\n>  \t\tfd = -1;\n>  \n> -\t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n> +\t\tdelay = delay + (delay * git_rand(CSPRNG_BYTES_INSECURE)) / UINT32_MAX + 1;\n>  \t\tsleep_millisec(delay);\n>  \t}\n>  \n> @@ -659,7 +659,7 @@ int reftable_stack_add(struct reftable_stack *st,\n>  static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n>  {\n>  \tchar buf[100];\n> -\tuint32_t rnd = (uint32_t)git_rand(0);\n> +\tuint32_t rnd = git_rand(CSPRNG_BYTES_INSECURE);\n>  \tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n>  \t\t min, max, rnd);\n>  \treftable_buf_reset(dest);\n"},{"id":"510115","messageId":"007f01db6147$90a24ed0$b1e6ec70$@nexbridge.com","threadId":"62759","inReplyTo":"xmqqzfk2qr62.fsf@gitster.g","subject":"RE: [PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2025-01-07T21:03:07Z","receivedAt":"2025-01-07T21:03:24Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On January 7, 2025 3:56 PM, Junio C Hamano wrote:\n>Patrick Steinhardt <ps@pks.im> writes:\n>\n>> The report was for NonStop, which uses OpenSSL as the backend for\n>> randomness. In the preceding commit we have adapted that backend to\n>> also return randomness in case the entropy pool is empty and the\n>> caller passes the `CSPRNG_BYTES_INSECURE` flag. Do so to fix the issue.\n>\n>No kidding.\n>\n>This is calling rand(3).  The use of the resulting value is to fuzz the\ndelay before\n>retrying, which wants NO CRYPTOGRAPHIC randomness.\n>This callsite does not require it, but rand(3) is perfect for ensuring\n>predictability/repeatability as well (via srand(3)).\n>\n>And it is not allowed to fail.\n\nI don't think rand() is what is failing here. The call into OpenSSL is using\nrandomness\nfrom PRNGD. A poorly configured, or overloaded instance of it will cause\nthis situation.\nThe ia64 platform for NonStop is using a standard PRNGD but it is being\noverloaded\nwith a large volume of uses hitting that server. I think this is more of a\ncanary-in-the-coalmine situation, where we have just brought something to\nlight\nthat otherwise would be hard to recreate.\n\n>Yet a platform replaces it with a function that returns an error or aborts?\nWhat kind\n>of nonsense is that?  Do we really need to cater to such an insanity?\n\nNonStop has not replaced this call. What we did in OpenSSL is to use the x86\nhardware randomizer, which is highly reliable and does not show this\nproblem. In\nia64, in OpenSSL, the PRNGD is generating randomness and that seems to be\nnot\nreliable enough for git or OpenSSL when under significant load.\n\n>Use of git_rand() here goes backwards against the more recent trend in\nreftable/\n>directory to wean the code off of the rest of Git by getting rid of\nunnecessary\n>dependency, doesn't it?\n>\n>I think [PATCH 1/2] makes sense regardless, though.  But shouldn't we be\npushing\n>back this step, with \"fix your rand(3)\"?\n>\n>Thanks.\n>\n>> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>\n>> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n>> ---\n>>  reftable/stack.c | 4 ++--\n>>  1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/reftable/stack.c b/reftable/stack.c index\n>>\n>6d0aa774e7e29d5366ed55df19725944f8eef792..572a74e00f9ed6040534e060\n>652e\n>> 72c26641749d 100644\n>> --- a/reftable/stack.c\n>> +++ b/reftable/stack.c\n>> @@ -493,7 +493,7 @@ static int reftable_stack_reload_maybe_reuse(struct\n>reftable_stack *st,\n>>  \t\tclose(fd);\n>>  \t\tfd = -1;\n>>\n>> -\t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n>> +\t\tdelay = delay + (delay * git_rand(CSPRNG_BYTES_INSECURE)) /\n>> +UINT32_MAX + 1;\n>>  \t\tsleep_millisec(delay);\n>>  \t}\n>>\n>> @@ -659,7 +659,7 @@ int reftable_stack_add(struct reftable_stack *st,\n>> static int format_name(struct reftable_buf *dest, uint64_t min,\n>> uint64_t max)  {\n>>  \tchar buf[100];\n>> -\tuint32_t rnd = (uint32_t)git_rand(0);\n>> +\tuint32_t rnd = git_rand(CSPRNG_BYTES_INSECURE);\n>>  \tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n>>  \t\t min, max, rnd);\n>>  \treftable_buf_reset(dest);\n\n--Randall\n\n"},{"id":"510116","messageId":"xmqqv7uqqqu9.fsf@gitster.g","threadId":"62759","inReplyTo":"xmqqzfk2qr62.fsf@gitster.g","subject":"Re: [PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-07T21:03:26Z","receivedAt":"2025-01-07T21:03:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Yet a platform replaces it with a function that returns an error or\n> aborts?  What kind of nonsense is that?  Do we really need to cater\n> to such an insanity?\n>\n> Use of git_rand() here goes backwards against the more recent trend\n> in reftable/ directory to wean the code off of the rest of Git by\n> getting rid of unnecessary dependency, doesn't it?\n>\n> I think [PATCH 1/2] makes sense regardless, though.  But shouldn't\n> we be pushing back this step, with \"fix your rand(3)\"?\n>\n> Thanks.\n\nAh, I misread the patch.  It has two hunks, and what I said applies\nto the earlier one, but the later one is already contaminated with\ngit_rand(), and that is what is failing, i.e. it is not a nonsense\nplatform replacing rand() with something that can fail.\n\nIt may still make sense to drop the first hunk, and consider how to\nproceed when you further want to reduce the unnecessary dependencies\nfor external users of the reftable library, though.  Are there\ncorrectness implications if git_rand() in format_name() yields non\nrandom results (like, always using \"rnd = 0\" instead of calling\ngit_rand())?  I seriously hope not.  And if there is no correctness\nimplications, perhaps we can replace it with rand() or even constant\n\"0\"?\n\nThanks.\n\n>> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>\n>> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n>> ---\n>>  reftable/stack.c | 4 ++--\n>>  1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/reftable/stack.c b/reftable/stack.c\n>> index 6d0aa774e7e29d5366ed55df19725944f8eef792..572a74e00f9ed6040534e060652e72c26641749d 100644\n>> --- a/reftable/stack.c\n>> +++ b/reftable/stack.c\n>> @@ -493,7 +493,7 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n>>  \t\tclose(fd);\n>>  \t\tfd = -1;\n>>  \n>> -\t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n>> +\t\tdelay = delay + (delay * git_rand(CSPRNG_BYTES_INSECURE)) / UINT32_MAX + 1;\n>>  \t\tsleep_millisec(delay);\n>>  \t}\n>>  \n>> @@ -659,7 +659,7 @@ int reftable_stack_add(struct reftable_stack *st,\n>>  static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n>>  {\n>>  \tchar buf[100];\n>> -\tuint32_t rnd = (uint32_t)git_rand(0);\n>> +\tuint32_t rnd = git_rand(CSPRNG_BYTES_INSECURE);\n>>  \tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n>>  \t\t min, max, rnd);\n>>  \treftable_buf_reset(dest);\n"},{"id":"510118","messageId":"xmqqmsg2qqjn.fsf@gitster.g","threadId":"62759","inReplyTo":"007f01db6147$90a24ed0$b1e6ec70$@nexbridge.com","subject":"Re: [PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-07T21:09:48Z","receivedAt":"2025-01-07T21:09:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"<rsbecker@nexbridge.com> writes:\n\n> I don't think rand() is what is failing here.\n\nYeah, sorry, I misead the patch.  The problematic one is git_rand()\nin the latter hunk, and I was commenting on the other, former, hunk.\n"},{"id":"510132","messageId":"Z323CLrRsnOko1gB@tapette.crustytoothpaste.net","threadId":"62759","inReplyTo":"20250107-b4-pks-reftable-csprng-v1-0-6109a54a8756@pks.im","subject":"Re: [PATCH 0/2] reftable/stack: stop dying on exhausted entropy pool","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-01-07T23:21:44Z","receivedAt":"2025-01-07T23:21:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-01-07 at 15:26:58, Patrick Steinhardt wrote:\n> Hi,\n> \n> this small patch series fixes the issue reported by Randall [1], where\n> an exhausted entropy pool can cause us to die when writing a new table\n> to the reftable stack. I _think_ that this is only an issue with the\n> OpenSSL backend of `csprng_bytes()`:\n> \n>   - `arc4random_buf()` never returns an error.\n> \n>   - `getrandom()` pulls from \"/dev/urandom\" by default.\n> \n>   - `getentropy()` seems to block when there is not enough randomness\n>     available.\n> \n>   - `GtlGenRandom()` I cannot really tell.\n> \n>   - The fallback reads from \"/dev/urandom\", which also returns bytes in\n>     case the entropy pool is drained.\n> \n> So OpenSSL's `RAND_bytes()` seems to be the only one that returns an\n> error when the entropy pool is empty. I did wonder whether we even need\n> to introduce the new flag in the first place, or whether we cannot just\n> use `RAND_pseudo_bytes()` unconditionally. But I'm a bit uneasy about it\n> given that OpenSSL has this doc:\n> \n>     RAND_pseudo_bytes() puts num pseudo-random bytes into buf.\n>     Pseudo-random byte sequences generated by RAND_pseudo_bytes() will\n>     be unique if they are of sufficient length, but are not necessarily\n>     unpredictable. They can be used for non-cryptographic purposes and\n>     for certain purposes in cryptographic protocols, but usually not for\n>     key generation etc.\n> \n> It might be too easy to accidentally rely on `csprng_bytes()` where it\n> actually requires strong cryptographic data, so I was erring on the side\n> of caution.\n\nThe reason I didn't use RAND_pseudo_bytes is because it's been\ndeprecated since OpenSSL 1.1.0 and RAND_bytes uses a CSPRNG just like\nRAND_pseudo_bytes as of that version.  Once it's seeded, it should be\nable to generate plenty of bytes, because I believe it uses a CTR-DRBG,\nwhich only needs to be reseeded after 2^48 bytes (which is far more than\nwe should be using).\n\nWe can full well use RAND_pseudo_bytes, but all operating systems should\nprovide an appropriate entropy source that can provide 256 bits of\nentropy on startup.  arc4random will just kill the process if it can't\nseed itself, so your changes won't actually prevent dying on a lack of\nentropy.\n\nI don't want an option that chooses \"insecure\" bytes.  My preference is\nthat we require people use a different backend or an up-to-date OpenSSL\nversion that shouldn't have this problem.  We can use RAND_pseudo_bytes\nif we really need to support older versions, but there are also no major\noperating systems which require that old of a version (CentOS 7, which\nis dead, used OpenSSL 1.0.2, and CentOS 8 uses 1.1.1k), so it's probably\nnot within our support policy to do that.\n\nNote also that if OpenSSL is being used for TLS, a lack of entropy will\nresult in TLS not working, which means that Git will be randomly broken\non that system, which is not really an experience that we want to\nencourage, so that should be taken into account.\n\nCan we get some more information about what version of OpenSSL is being\nused and what the system entropy source is?\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"510136","messageId":"00ac01db615f$70b72600$52257200$@nexbridge.com","threadId":"62759","inReplyTo":"Z323CLrRsnOko1gB@tapette.crustytoothpaste.net","subject":"RE: [PATCH 0/2] reftable/stack: stop dying on exhausted entropy pool","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2025-01-07T23:54:02Z","receivedAt":"2025-01-07T23:54:19Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On January 7, 2025 6:22 PM, brian m. carlson wrote:\n>On 2025-01-07 at 15:26:58, Patrick Steinhardt wrote:\n>> Hi,\n>>\n>> this small patch series fixes the issue reported by Randall [1], where\n>> an exhausted entropy pool can cause us to die when writing a new table\n>> to the reftable stack. I _think_ that this is only an issue with the\n>> OpenSSL backend of `csprng_bytes()`:\n>>\n>>   - `arc4random_buf()` never returns an error.\n>>\n>>   - `getrandom()` pulls from \"/dev/urandom\" by default.\n>>\n>>   - `getentropy()` seems to block when there is not enough randomness\n>>     available.\n>>\n>>   - `GtlGenRandom()` I cannot really tell.\n>>\n>>   - The fallback reads from \"/dev/urandom\", which also returns bytes in\n>>     case the entropy pool is drained.\n>>\n>> So OpenSSL's `RAND_bytes()` seems to be the only one that returns an\n>> error when the entropy pool is empty. I did wonder whether we even\n>> need to introduce the new flag in the first place, or whether we\n>> cannot just use `RAND_pseudo_bytes()` unconditionally. But I'm a bit\n>> uneasy about it given that OpenSSL has this doc:\n>>\n>>     RAND_pseudo_bytes() puts num pseudo-random bytes into buf.\n>>     Pseudo-random byte sequences generated by RAND_pseudo_bytes() will\n>>     be unique if they are of sufficient length, but are not necessarily\n>>     unpredictable. They can be used for non-cryptographic purposes and\n>>     for certain purposes in cryptographic protocols, but usually not for\n>>     key generation etc.\n>>\n>> It might be too easy to accidentally rely on `csprng_bytes()` where it\n>> actually requires strong cryptographic data, so I was erring on the\n>> side of caution.\n>\n>The reason I didn't use RAND_pseudo_bytes is because it's been deprecated since\n>OpenSSL 1.1.0 and RAND_bytes uses a CSPRNG just like RAND_pseudo_bytes as of\n>that version.  Once it's seeded, it should be able to generate plenty of bytes,\n>because I believe it uses a CTR-DRBG, which only needs to be reseeded after 2^48\n>bytes (which is far more than we should be using).\n>\n>We can full well use RAND_pseudo_bytes, but all operating systems should provide\n>an appropriate entropy source that can provide 256 bits of entropy on startup.\n>arc4random will just kill the process if it can't seed itself, so your changes won't\n>actually prevent dying on a lack of entropy.\n>\n>I don't want an option that chooses \"insecure\" bytes.  My preference is that we\n>require people use a different backend or an up-to-date OpenSSL version that\n>shouldn't have this problem.  We can use RAND_pseudo_bytes if we really need to\n>support older versions, but there are also no major operating systems which\n>require that old of a version (CentOS 7, which is dead, used OpenSSL 1.0.2, and\n>CentOS 8 uses 1.1.1k), so it's probably not within our support policy to do that.\n>\n>Note also that if OpenSSL is being used for TLS, a lack of entropy will result in TLS\n>not working, which means that Git will be randomly broken on that system, which is\n>not really an experience that we want to encourage, so that should be taken into\n>account.\n>\n>Can we get some more information about what version of OpenSSL is being used\n>and what the system entropy source is?\n\nIn my situation, OpenSSL 3.0.11 on ia64. 3.4.1 and 3.0.13 on x86 (x86 works fine\nBecause OpenSSL uses hardware. On ia64, we end up on PRNGD, which does fail.\n\n"},{"id":"510137","messageId":"00ad01db615f$ce9b6740$6bd235c0$@nexbridge.com","threadId":"62759","inReplyTo":"20250107-b4-pks-reftable-csprng-v1-2-6109a54a8756@pks.im","subject":"RE: [PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2025-01-07T23:56:38Z","receivedAt":"2025-01-07T23:56:48Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"\n\n>-----Original Message-----\n>From: Patrick Steinhardt <ps@pks.im>\n>Sent: January 7, 2025 10:27 AM\n>To: git@vger.kernel.org\n>Cc: Randall S. Becker <randall.becker@nexbridge.ca>\n>Subject: [PATCH 2/2] reftable/stack: accept insecure random bytes\n>\n>The reftable library uses randomness in two call paths:\n>\n>  - When reading a stack in case some of the referenced tables\n>    disappears. The randomness is used to delay the next read by a\n>    couple of milliseconds.\n>\n>  - When writing a new table, where the randomness gets appended to the\n>    table name (e.g. \"0x000000000001-0x000000000002-0b1d8ddf.ref\").\n>\n>In neither of these cases do we need strong randomness.\n>\n>Unfortunately though, we have observed test failures caused by the former case. In\n>t0610 we have a test that spawns a 100 processes at once, all of which try to write\n>a new table to the stack. And given that all of the processes will require\n>randomness, it can happen that these processes make the entropy pool run dry,\n>which will then cause us to\n>die:\n>\n>    + test_seq 100\n>    + printf %s commit\\trefs/heads/branch-%s\\n\n>    68d032e9edd3481ac96382786ececc37ec28709e 1\n>    + printf %s commit\\trefs/heads/branch-%s\\n\n>    68d032e9edd3481ac96382786ececc37ec28709e 2\n>    ...\n>    + git update-ref refs/heads/branch-98 HEAD\n>    + git update-ref refs/heads/branch-97 HEAD\n>    + git update-ref refs/heads/branch-99 HEAD\n>    + git update-ref refs/heads/branch-100 HEAD\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>    fatal: unable to get random bytes\n>\n>The report was for NonStop, which uses OpenSSL as the backend for randomness.\n>In the preceding commit we have adapted that backend to also return randomness\n>in case the entropy pool is empty and the caller passes the\n>`CSPRNG_BYTES_INSECURE` flag. Do so to fix the issue.\n>\n>Reported-by: Randall S. Becker <rsbecker@nexbridge.com>\n>Signed-off-by: Patrick Steinhardt <ps@pks.im>\n>---\n> reftable/stack.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n>diff --git a/reftable/stack.c b/reftable/stack.c index\n>6d0aa774e7e29d5366ed55df19725944f8eef792..572a74e00f9ed6040534e060\n>652e72c26641749d 100644\n>--- a/reftable/stack.c\n>+++ b/reftable/stack.c\n>@@ -493,7 +493,7 @@ static int reftable_stack_reload_maybe_reuse(struct\n>reftable_stack *st,\n> \t\tclose(fd);\n> \t\tfd = -1;\n>\n>-\t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n>+\t\tdelay = delay + (delay * git_rand(CSPRNG_BYTES_INSECURE)) /\n>+UINT32_MAX + 1;\n> \t\tsleep_millisec(delay);\n> \t}\n>\n>@@ -659,7 +659,7 @@ int reftable_stack_add(struct reftable_stack *st,  static int\n>format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)  {\n> \tchar buf[100];\n>-\tuint32_t rnd = (uint32_t)git_rand(0);\n>+\tuint32_t rnd = git_rand(CSPRNG_BYTES_INSECURE);\n> \tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n> \t\t min, max, rnd);\n> \treftable_buf_reset(dest);\n>\n>--\n>2.48.0.rc1.245.gb3e6e7acbc.dirty\n\nThe tests that are all impacted on ia64 (using PRNGD) are:\n\nt0610-reftable-basics\nt1092-sparse-checkout-compatibility\nt1300-config\nt1404-update-ref-errors\nt1410-reflog\nt1414-reflog-walk\nt1415-worktree-refs\nt1450-fsck\nt1460-refs-migrate\nt1500-rev-parse\nt2006-checkout-index-basic\nt2009-checkout-statinfo\nt2013-checkout-submodule\nt2017-checkout-orphan\nt2030-unresolve-info\nt2400-worktree-add\n\nEverything passes on x86.\n\n"},{"id":"510147","messageId":"Z34gfa-_dSbWD19h@pks.im","threadId":"62759","inReplyTo":"xmqqv7uqqqu9.fsf@gitster.g","subject":"Re: [PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-08T06:51:41Z","receivedAt":"2025-01-08T06:51:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 07, 2025 at 01:03:26PM -0800, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Yet a platform replaces it with a function that returns an error or\n> > aborts?  What kind of nonsense is that?  Do we really need to cater\n> > to such an insanity?\n> >\n> > Use of git_rand() here goes backwards against the more recent trend\n> > in reftable/ directory to wean the code off of the rest of Git by\n> > getting rid of unnecessary dependency, doesn't it?\n\nIt certainly does, yes, but unifying those two callsites to do the same\nis also something I do in a patch series that gets rid of the last deps\non the Git codebase. This then allows me to move `git_rand()` into\n\"reftable/system.h\" and make it a shim provided by the respective code\nbase that it's part of. So it's not a step backwards.\n\n> > I think [PATCH 1/2] makes sense regardless, though.  But shouldn't\n> > we be pushing back this step, with \"fix your rand(3)\"?\n> >\n> > Thanks.\n> \n> Ah, I misread the patch.  It has two hunks, and what I said applies\n> to the earlier one, but the later one is already contaminated with\n> git_rand(), and that is what is failing, i.e. it is not a nonsense\n> platform replacing rand() with something that can fail.\n> \n> It may still make sense to drop the first hunk, and consider how to\n> proceed when you further want to reduce the unnecessary dependencies\n> for external users of the reftable library, though.  Are there\n> correctness implications if git_rand() in format_name() yields non\n> random results (like, always using \"rnd = 0\" instead of calling\n> git_rand())?  I seriously hope not.  And if there is no correctness\n> implications, perhaps we can replace it with rand() or even constant\n> \"0\"?\n\nNo, there aren't any implications on correctness in that case. Sure, the\nrandomized delays not being randomized can lead to more contention. But\neven when the randomized suffix for tables is deterministic we wouldn't\nhave an issue as the files are still distinguished by their update\nindices.\n\nPatrick\n"},{"id":"510148","messageId":"Z34m1HRLAeEszXDG@pks.im","threadId":"62759","inReplyTo":"00ac01db615f$70b72600$52257200$@nexbridge.com","subject":"Re: [PATCH 0/2] reftable/stack: stop dying on exhausted entropy pool","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-08T07:18:52Z","receivedAt":"2025-01-08T07:18:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 07, 2025 at 06:54:02PM -0500, rsbecker@nexbridge.com wrote:\n> On January 7, 2025 6:22 PM, brian m. carlson wrote:\n> >On 2025-01-07 at 15:26:58, Patrick Steinhardt wrote:\n> >> Hi,\n> >>\n> >> this small patch series fixes the issue reported by Randall [1], where\n> >> an exhausted entropy pool can cause us to die when writing a new table\n> >> to the reftable stack. I _think_ that this is only an issue with the\n> >> OpenSSL backend of `csprng_bytes()`:\n> >>\n> >>   - `arc4random_buf()` never returns an error.\n> >>\n> >>   - `getrandom()` pulls from \"/dev/urandom\" by default.\n> >>\n> >>   - `getentropy()` seems to block when there is not enough randomness\n> >>     available.\n> >>\n> >>   - `GtlGenRandom()` I cannot really tell.\n> >>\n> >>   - The fallback reads from \"/dev/urandom\", which also returns bytes in\n> >>     case the entropy pool is drained.\n> >>\n> >> So OpenSSL's `RAND_bytes()` seems to be the only one that returns an\n> >> error when the entropy pool is empty. I did wonder whether we even\n> >> need to introduce the new flag in the first place, or whether we\n> >> cannot just use `RAND_pseudo_bytes()` unconditionally. But I'm a bit\n> >> uneasy about it given that OpenSSL has this doc:\n> >>\n> >>     RAND_pseudo_bytes() puts num pseudo-random bytes into buf.\n> >>     Pseudo-random byte sequences generated by RAND_pseudo_bytes() will\n> >>     be unique if they are of sufficient length, but are not necessarily\n> >>     unpredictable. They can be used for non-cryptographic purposes and\n> >>     for certain purposes in cryptographic protocols, but usually not for\n> >>     key generation etc.\n> >>\n> >> It might be too easy to accidentally rely on `csprng_bytes()` where it\n> >> actually requires strong cryptographic data, so I was erring on the\n> >> side of caution.\n> >\n> >The reason I didn't use RAND_pseudo_bytes is because it's been deprecated since\n> >OpenSSL 1.1.0 and RAND_bytes uses a CSPRNG just like RAND_pseudo_bytes as of\n> >that version.  Once it's seeded, it should be able to generate plenty of bytes,\n> >because I believe it uses a CTR-DRBG, which only needs to be reseeded after 2^48\n> >bytes (which is far more than we should be using).\n> >\n> >We can full well use RAND_pseudo_bytes, but all operating systems should provide\n> >an appropriate entropy source that can provide 256 bits of entropy on startup.\n> >arc4random will just kill the process if it can't seed itself, so your changes won't\n> >actually prevent dying on a lack of entropy.\n> >\n> >I don't want an option that chooses \"insecure\" bytes.  My preference is that we\n> >require people use a different backend or an up-to-date OpenSSL version that\n> >shouldn't have this problem.  We can use RAND_pseudo_bytes if we really need to\n> >support older versions, but there are also no major operating systems which\n> >require that old of a version (CentOS 7, which is dead, used OpenSSL 1.0.2, and\n> >CentOS 8 uses 1.1.1k), so it's probably not within our support policy to do that.\n> >\n> >Note also that if OpenSSL is being used for TLS, a lack of entropy will result in TLS\n> >not working, which means that Git will be randomly broken on that system, which is\n> >not really an experience that we want to encourage, so that should be taken into\n> >account.\n> >\n> >Can we get some more information about what version of OpenSSL is being used\n> >and what the system entropy source is?\n> \n> In my situation, OpenSSL 3.0.11 on ia64. 3.4.1 and 3.0.13 on x86 (x86 works fine\n> Because OpenSSL uses hardware. On ia64, we end up on PRNGD, which does fail.\n\nYou reported in [1] that a couple more tests are indeed failing, not\nonly t0610. That changes things in my opinion as it shows that this is\nnot a localized issue in the reftable library, but likely in multiple\ncallsites where we use randomness. So my current patch series is not\nsufficient as it only fixes up the reftable codebase. But in the case\nwhere it's a general issue I tend to agree with brian, because I don't\nwant to play whack-a-mole with all the callsites of `git_rand()` where\nwe can indeed use insecure bytes.\n\nHonestly, this rather makes me want to remove the OpenSSL backend for\nour CSRNG completely. NonStop is the only platform that uses it right\nnow, and it seems to be easy to misconfigure. All the other backends we\nhave don't have the same issue as explained further up in my message. So\ndoes NonStop support any of the alternative backends that Git has, like\n`arc4random_buf()`, `getrandom()`, `getentropy()` or reading from\n\"/dev/urandom\"?\n\nMight be I'm coming to conclusions too fast, so if I'm missing obvious\nusecases then please stop me :)\n\nRandall, you mentioned that your platform had a maintenance window right\nduring the release of v2.48.0-rc2 [2]. You never mentioned issues with\nrandomness before that maintenance window, and after it you hit them in\nmany tests without any changes to the CSPRNG between v2.48.0-rc1 and\n-rc2. Could it be that something broke on your end?\n\nPatrick\n\n[1]: https://lore.kernel.org/git/00ad01db615f$ce9b6740$6bd235c0$@nexbridge.com/\n[2]: https://lore.kernel.org/git/000501db607c$40c009a0$c2401ce0$@nexbridge.com/\n"},{"id":"510159","messageId":"00dc01db61d4$48bba660$da32f320$@nexbridge.com","threadId":"62759","inReplyTo":"Z34m1HRLAeEszXDG@pks.im","subject":"RE: [PATCH 0/2] reftable/stack: stop dying on exhausted entropy pool","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2025-01-08T13:50:26Z","receivedAt":"2025-01-08T13:50:41Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On January 8, 2025 2:19 AM, Patrick Steinhardt wrote:\n>On Tue, Jan 07, 2025 at 06:54:02PM -0500, rsbecker@nexbridge.com wrote:\n>> On January 7, 2025 6:22 PM, brian m. carlson wrote:\n>> >On 2025-01-07 at 15:26:58, Patrick Steinhardt wrote:\n>> >> Hi,\n>> >>\n>> >> this small patch series fixes the issue reported by Randall [1],\n>> >> where an exhausted entropy pool can cause us to die when writing a\n>> >> new table to the reftable stack. I _think_ that this is only an\n>> >> issue with the OpenSSL backend of `csprng_bytes()`:\n>> >>\n>> >>   - `arc4random_buf()` never returns an error.\n>> >>\n>> >>   - `getrandom()` pulls from \"/dev/urandom\" by default.\n>> >>\n>> >>   - `getentropy()` seems to block when there is not enough randomness\n>> >>     available.\n>> >>\n>> >>   - `GtlGenRandom()` I cannot really tell.\n>> >>\n>> >>   - The fallback reads from \"/dev/urandom\", which also returns bytes\nin\n>> >>     case the entropy pool is drained.\n>> >>\n>> >> So OpenSSL's `RAND_bytes()` seems to be the only one that returns\n>> >> an error when the entropy pool is empty. I did wonder whether we\n>> >> even need to introduce the new flag in the first place, or whether\n>> >> we cannot just use `RAND_pseudo_bytes()` unconditionally. But I'm a\n>> >> bit uneasy about it given that OpenSSL has this doc:\n>> >>\n>> >>     RAND_pseudo_bytes() puts num pseudo-random bytes into buf.\n>> >>     Pseudo-random byte sequences generated by RAND_pseudo_bytes() will\n>> >>     be unique if they are of sufficient length, but are not\nnecessarily\n>> >>     unpredictable. They can be used for non-cryptographic purposes and\n>> >>     for certain purposes in cryptographic protocols, but usually not\nfor\n>> >>     key generation etc.\n>> >>\n>> >> It might be too easy to accidentally rely on `csprng_bytes()` where\n>> >> it actually requires strong cryptographic data, so I was erring on\n>> >> the side of caution.\n>> >\n>> >The reason I didn't use RAND_pseudo_bytes is because it's been\n>> >deprecated since OpenSSL 1.1.0 and RAND_bytes uses a CSPRNG just like\n>> >RAND_pseudo_bytes as of that version.  Once it's seeded, it should be\n>> >able to generate plenty of bytes, because I believe it uses a\n>> >CTR-DRBG, which only needs to be reseeded after 2^48 bytes (which is far\nmore\n>than we should be using).\n>> >\n>> >We can full well use RAND_pseudo_bytes, but all operating systems\n>> >should provide an appropriate entropy source that can provide 256 bits\nof\n>entropy on startup.\n>> >arc4random will just kill the process if it can't seed itself, so\n>> >your changes won't actually prevent dying on a lack of entropy.\n>> >\n>> >I don't want an option that chooses \"insecure\" bytes.  My preference\n>> >is that we require people use a different backend or an up-to-date\n>> >OpenSSL version that shouldn't have this problem.  We can use\n>> >RAND_pseudo_bytes if we really need to support older versions, but\n>> >there are also no major operating systems which require that old of a\n>> >version (CentOS 7, which is dead, used OpenSSL 1.0.2, and CentOS 8 uses\n>1.1.1k), so it's probably not within our support policy to do that.\n>> >\n>> >Note also that if OpenSSL is being used for TLS, a lack of entropy\n>> >will result in TLS not working, which means that Git will be randomly\n>> >broken on that system, which is not really an experience that we want\n>> >to encourage, so that should be taken into account.\n>> >\n>> >Can we get some more information about what version of OpenSSL is\n>> >being used and what the system entropy source is?\n>>\n>> In my situation, OpenSSL 3.0.11 on ia64. 3.4.1 and 3.0.13 on x86 (x86\n>> works fine Because OpenSSL uses hardware. On ia64, we end up on PRNGD,\n>which does fail.\n>\n>You reported in [1] that a couple more tests are indeed failing, not only\nt0610. That\n>changes things in my opinion as it shows that this is not a localized issue\nin the\n>reftable library, but likely in multiple callsites where we use randomness.\nSo my\n>current patch series is not sufficient as it only fixes up the reftable\ncodebase. But in\n>the case where it's a general issue I tend to agree with brian, because I\ndon't want to\n>play whack-a-mole with all the callsites of `git_rand()` where we can\nindeed use\n>insecure bytes.\n>\n>Honestly, this rather makes me want to remove the OpenSSL backend for our\n>CSRNG completely. NonStop is the only platform that uses it right now, and\nit seems\n>to be easy to misconfigure. All the other backends we have don't have the\nsame\n>issue as explained further up in my message. So does NonStop support any of\nthe\n>alternative backends that Git has, like `arc4random_buf()`, `getrandom()`,\n>`getentropy()` or reading from \"/dev/urandom\"?\n>\n>Might be I'm coming to conclusions too fast, so if I'm missing obvious\nusecases then\n>please stop me :)\n>\n>Randall, you mentioned that your platform had a maintenance window right\nduring\n>the release of v2.48.0-rc2 [2]. You never mentioned issues with randomness\nbefore\n>that maintenance window, and after it you hit them in many tests without\nany\n>changes to the CSPRNG between v2.48.0-rc1 and -rc2. Could it be that\nsomething\n>broke on your end?\n\nUnfortunately, ia64 is not a great platform for randomness. There are no\nalternates\navailable. We have a case open on PRNGD, but it is unlikely to be fixed any\ntime soon.\nThe ia64 platform goes off support at the end of 2025, so we will stop\nbuilding git\nfor that platform when that happens. If there is some stopgap solution we\ncan use,\neven PRNGD, but warn about reducing randomness load, it might work. For x86,\nthe hardware randomizer used in OpenSSL is fine.\n\n"},{"id":"510165","messageId":"xmqqr05dnwli.fsf@gitster.g","threadId":"62759","inReplyTo":"Z34gfa-_dSbWD19h@pks.im","subject":"Re: [PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-08T15:39:37Z","receivedAt":"2025-01-08T15:39:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> It may still make sense to drop the first hunk, and consider how to\n>> proceed when you further want to reduce the unnecessary dependencies\n>> for external users of the reftable library, though.  Are there\n>> correctness implications if git_rand() in format_name() yields non\n>> random results (like, always using \"rnd = 0\" instead of calling\n>> git_rand())?  I seriously hope not.  And if there is no correctness\n>> implications, perhaps we can replace it with rand() or even constant\n>> \"0\"?\n>\n> No, there aren't any implications on correctness in that case. Sure, the\n> randomized delays not being randomized can lead to more contention. But\n> even when the randomized suffix for tables is deterministic we wouldn't\n> have an issue as the files are still distinguished by their update\n> indices.\n\nOK, so they both can be turned into a simple rand() that is expected\nto work more reliably especially on more exotic systems (meaning:\nthe ability the system providers can test their rand() is much\nbetter than our ability to test our git_rand() there)?  It would\nhelp us solve the immediate issue reported, while removing one git\nspecific function from the reftable library?\n\nThanks.\n"},{"id":"510173","messageId":"Z36l--QUjaYYb6Uf@pks.im","threadId":"62759","inReplyTo":"xmqqr05dnwli.fsf@gitster.g","subject":"Re: [PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-08T16:21:15Z","receivedAt":"2025-01-08T16:21:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 08, 2025 at 07:39:37AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> >> It may still make sense to drop the first hunk, and consider how to\n> >> proceed when you further want to reduce the unnecessary dependencies\n> >> for external users of the reftable library, though.  Are there\n> >> correctness implications if git_rand() in format_name() yields non\n> >> random results (like, always using \"rnd = 0\" instead of calling\n> >> git_rand())?  I seriously hope not.  And if there is no correctness\n> >> implications, perhaps we can replace it with rand() or even constant\n> >> \"0\"?\n> >\n> > No, there aren't any implications on correctness in that case. Sure, the\n> > randomized delays not being randomized can lead to more contention. But\n> > even when the randomized suffix for tables is deterministic we wouldn't\n> > have an issue as the files are still distinguished by their update\n> > indices.\n> \n> OK, so they both can be turned into a simple rand() that is expected\n> to work more reliably especially on more exotic systems (meaning:\n> the ability the system providers can test their rand() is much\n> better than our ability to test our git_rand() there)?  It would\n> help us solve the immediate issue reported, while removing one git\n> specific function from the reftable library?\n\nHm. The problem is when Git dies in the middle of a transaction:\n\n  1. We write the temporary table.\n  2. We compute the not-so-random suffix.\n  3. We write the temporary \"tables.list\" file.\n  4. We move the temporary table into place using the not-so-random\n     suffix.\n  5. Git dies before updating \"tables.list\".\n\nNow we have the temporary table moved into place, but \"tables.list\"\nhasn't been updated yet. When the next Git process comes along and wants\nto update the table it would result in an error if it computed the same\nsuffix.\n\nThe reftable library knows to clean up such stale tables when not\nreferenced by the \"tables.list\" file, but it doesn't do so on every\nwrite. So this would likely still cause issues in practice.\n\nI already though about this scenario when writing my mail, but didn't\nreally think about it as \"correctness\". But I guess it is.\n\nAlso, based on the feedback from Randall it's not only the reftable\nbackend that has issues. It's a more general problem on ia64, where many\ntests are failing. So even if we fixed this one case, it's likely that\nother cases would still die when running low on entropy.\n\nI dunno. It feels like a platform issue, not like a Git issue, when the\nRNG cannot provide us a couple of integers. The OpenSSL backend seems\nunfit for use to me as none of the other backends have the same issue.\n\nPatrick\n"},{"id":"510177","messageId":"xmqqo70hmcet.fsf@gitster.g","threadId":"62759","inReplyTo":"Z36l--QUjaYYb6Uf@pks.im","subject":"Re: [PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-08T17:40:58Z","receivedAt":"2025-01-08T17:41:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Hm. The problem is when Git dies in the middle of a transaction:\n>\n>   1. We write the temporary table.\n>   2. We compute the not-so-random suffix.\n>   3. We write the temporary \"tables.list\" file.\n>   4. We move the temporary table into place using the not-so-random\n>      suffix.\n>   5. Git dies before updating \"tables.list\".\n>\n> Now we have the temporary table moved into place, but \"tables.list\"\n> hasn't been updated yet. When the next Git process comes along and wants\n> to update the table it would result in an error if it computed the same\n> suffix.\n\nHere, I hear that we _do_ depend on the suffix being relatively\nunique.  Once our random number generator decides to give the same\nnumber twice to cause collision, the reftable data gets corrupt?\n\n> The reftable library knows to clean up such stale tables when not\n> referenced by the \"tables.list\" file, but it doesn't do so on every\n> write. So this would likely still cause issues in practice.\n>\n> I already though about this scenario when writing my mail, but didn't\n> really think about it as \"correctness\". But I guess it is.\n\nHmph.  I am not sure how I should feel about this.  Our reliance on\nhash functions (which can be made to collide) not colliding is one\nthing, but is it sensibly safe to rely on a cryptographically\nunpredictable random generator not to yield the same suffix twice\nduring the lifetime of an previous invocation for correctness?\n"},{"id":"510179","messageId":"Z37A_d9mAnKtGNcU@pks.im","threadId":"62759","inReplyTo":"xmqqo70hmcet.fsf@gitster.g","subject":"Re: [PATCH 2/2] reftable/stack: accept insecure random bytes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-08T18:16:35Z","receivedAt":"2025-01-08T18:16:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 08, 2025 at 09:40:58AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Hm. The problem is when Git dies in the middle of a transaction:\n> >\n> >   1. We write the temporary table.\n> >   2. We compute the not-so-random suffix.\n> >   3. We write the temporary \"tables.list\" file.\n> >   4. We move the temporary table into place using the not-so-random\n> >      suffix.\n> >   5. Git dies before updating \"tables.list\".\n> >\n> > Now we have the temporary table moved into place, but \"tables.list\"\n> > hasn't been updated yet. When the next Git process comes along and wants\n> > to update the table it would result in an error if it computed the same\n> > suffix.\n> \n> Here, I hear that we _do_ depend on the suffix being relatively\n> unique.  Once our random number generator decides to give the same\n> number twice to cause collision, the reftable data gets corrupt?\n\nNo, there is no corruption. We may fail to update the stack when there\nare colliding files, but that's it.\n\n> > The reftable library knows to clean up such stale tables when not\n> > referenced by the \"tables.list\" file, but it doesn't do so on every\n> > write. So this would likely still cause issues in practice.\n> >\n> > I already though about this scenario when writing my mail, but didn't\n> > really think about it as \"correctness\". But I guess it is.\n> \n> Hmph.  I am not sure how I should feel about this.  Our reliance on\n> hash functions (which can be made to collide) not colliding is one\n> thing, but is it sensibly safe to rely on a cryptographically\n> unpredictable random generator not to yield the same suffix twice\n> during the lifetime of an previous invocation for correctness?\n\nThis is why I've been hesistant to call it a \"correctness\" issue, as\nthere is no corruption involved here. It's more of a denial of service\nas you may not be able to update the stack anymore until you remove the\noccluding file.\n\nBut turns out I misremembered from 9abda98149 (reftable/stack: fix use\nof unseeded randomness, 2023-12-11): things indeed work alright. To\ndemonstrate, let's update `format_name()` like this:\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 531660a49f..b7422679df 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -659,7 +659,7 @@ int reftable_stack_add(struct reftable_stack *st,\n static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)\n {\n \tchar buf[100];\n-\tuint32_t rnd = (uint32_t)git_rand();\n+\tuint32_t rnd = 123;\n \tsnprintf(buf, sizeof(buf), \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x\",\n \t\t min, max, rnd);\n \treftable_buf_reset(dest);\n\nAnd then we create an occluding file and try to update:\n\n    $ ~/Development/git/build/bin-wrappers/git init repo --ref-format=reftable\n    Initialized empty Git repository in /tmp/repo/.git/\n    $ cd repo/\n    $ ls .git/reftable/\n    0x000000000001-0x000000000001-0000007b.ref  tables.list\n    $ touch .git/reftable/0x000000000002-0x000000000002-0000007b.ref\n    $ ~/Development/git/build/git commit --allow-empty -mx\n    [main (root-commit) 08d02ef] x\n    $ ls .git/reftable/\n    0x000000000001-0x000000000002-0000007b.ref  tables.list\n    $ git show\n    commit 08d02efa88f085f02c31285bf909d8d3a25c70dd (HEAD -> main)\n    Author: Patrick Steinhardt <ps@pks.im>\n    Date:   Wed Jan 8 19:03:46 2025 +0100\n\n    x\n\nSo the stack gets updated as expected, no corruption there.\n\nBut this _can_ be a problem on Windows, where the file cannot be deleted\nin case it was still open. This is also documented as the reason in\n\"Documentation/techincal/reftable.txt\". That should've gotten better\nthough now with the improvements I made to our rename-emulation, as it\nnow uses POSIX semantics.\n\nThat being said, I still don't think that swapping out `git_rand()` for\n`rand()` is the right thing to do. It does not solve the issue, but only\na symptom thereof. Git can still die whenever the OpenSSL CSPRNG fails\nas there are other uses of `git_rand()` or `csprng_bytes()` in the\ncodebase.\n\nSo this change would regress something that works everywhere but on\nNonStop and make the suffixes predictable. The ia64 machine in question\nis being EOLd end of 2025. And the fact that this has never been a\nproblem before v2.48.0-rc2, and that the machine was rebooted for\nmaintenance immediately before running the tests, indicates to me that\nsomething fishy is going on on that platform.\n\nPatrick\n"},{"id":"510207","messageId":"Z37_4hoUgeVd3p9x@tapette.crustytoothpaste.net","threadId":"62759","inReplyTo":"Z34m1HRLAeEszXDG@pks.im","subject":"Re: [PATCH 0/2] reftable/stack: stop dying on exhausted entropy pool","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-01-08T22:44:50Z","receivedAt":"2025-01-08T22:44:56Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-01-08 at 07:18:52, Patrick Steinhardt wrote:\n> You reported in [1] that a couple more tests are indeed failing, not\n> only t0610. That changes things in my opinion as it shows that this is\n> not a localized issue in the reftable library, but likely in multiple\n> callsites where we use randomness. So my current patch series is not\n> sufficient as it only fixes up the reftable codebase. But in the case\n> where it's a general issue I tend to agree with brian, because I don't\n> want to play whack-a-mole with all the callsites of `git_rand()` where\n> we can indeed use insecure bytes.\n> \n> Honestly, this rather makes me want to remove the OpenSSL backend for\n> our CSRNG completely. NonStop is the only platform that uses it right\n> now, and it seems to be easy to misconfigure. All the other backends we\n> have don't have the same issue as explained further up in my message. So\n> does NonStop support any of the alternative backends that Git has, like\n> `arc4random_buf()`, `getrandom()`, `getentropy()` or reading from\n> \"/dev/urandom\"?\n\nOpenSSL's backend is only as good as the system entropy source, which,\napparently in the case of PRNGD, is not very good.  The last release of\nPRNGD was in 2007 apparently, so I don't think we should hold our breath\nfor a fix.\n\nOr, of course, it could be simply that prngd works just fine and there\naren't enough sources for it.  If the machine has an analog microphone\ninput that isn't plugged in, streaming some data from that might be a\ngood source, since that will be noisy.  A second of recording signed\n16-bit PCM data as 48 kHz might provide at least 64 bits of entropy[0].\n\nI will say that libbsd provides a fallback implementation for its\ngetentropy code, which would allow the use of arc4random as a backend.\nI know there were some portability problems with getting that to run on\nNonStop, and of course I provide no guarantees about its suitability or\nsecurity, but it does appear that there is some alternative if the\nporting problems can be overcome.  We explicitly have support for libbsd\nin the Makefile already.\n\nThat doesn't avoid the problem of TLS and SSH not working, but it may\nget the tests passing.\n\n[0] That's 750 samples per bit of entropy, which I think should be\nreasonably conservative.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"}]}