Skip to content

Commit 1568d15

Browse files
pks-tgitster
authored andcommitted
wrapper: allow generating insecure random bytes
The `csprng_bytes()` function generates randomness and writes it into a caller-provided buffer. It abstracts over a couple of implementations, where the exact one that is used depends on the platform. These implementations have different guarantees: while some guarantee to never fail (arc4random(3)), others may fail. There are two significant failures to distinguish from one another: - Systemic failure, where e.g. opening "/dev/urandom" fails or when OpenSSL doesn't have a provider configured. - Entropy failure, where the entropy pool is exhausted, and thus the function cannot guarantee strong cryptographic randomness. While we cannot do anything about the former, the latter failure can be acceptable in some situations where we don't care whether or not the randomness can be predicted. Introduce a new `CSPRNG_BYTES_INSECURE` flag that allows callers to opt into weak cryptographic randomness. The exact behaviour of the flag depends on the underlying implementation: - `arc4random_buf()` never returns an error, so it doesn't change. - `getrandom()` pulls from "/dev/urandom" by default, which never blocks on modern systems even when the entropy pool is empty. - `getentropy()` seems to block when there is not enough randomness available, and there is no way of changing that behaviour. - `GtlGenRandom()` doesn't mention anything about its specific failure mode. - The fallback reads from "/dev/urandom", which also returns bytes in case the entropy pool is drained in modern Linux systems. That only leaves OpenSSL with `RAND_bytes()`, which returns an error in case the returned data wouldn't be cryptographically safe. This function is replaced with a call to `RAND_pseudo_bytes()`, which can indicate whether or not the returned data is cryptographically secure via its return value. If it is insecure, and if the `CSPRNG_BYTES_INSECURE` flag is set, then we ignore the insecurity and return the data regardless. It is somewhat questionable whether we really need the flag in the first place, or whether we wouldn't just ignore the potentially-insecure data. But the risk of doing that is that we might have or grow callsites that aren't aware of the potential insecureness of the data in places where it really matters. So using a flag to opt-in to that behaviour feels like the more secure choice. Signed-off-by: Patrick Steinhardt <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent b74ff38 commit 1568d15

File tree

6 files changed

+32
-20
lines changed

6 files changed

+32
-20
lines changed

builtin/gc.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1909,7 +1909,7 @@ static int get_random_minute(void)
19091909
if (getenv("GIT_TEST_MAINT_SCHEDULER"))
19101910
return 13;
19111911

1912-
return git_rand() % 60;
1912+
return git_rand(0) % 60;
19131913
}
19141914

19151915
static int is_launchctl_available(void)

reftable/stack.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -659,7 +659,7 @@ int reftable_stack_add(struct reftable_stack *st,
659659
static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)
660660
{
661661
char buf[100];
662-
uint32_t rnd = (uint32_t)git_rand();
662+
uint32_t rnd = (uint32_t)git_rand(0);
663663
snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
664664
min, max, rnd);
665665
reftable_buf_reset(dest);

t/helper/test-csprng.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ int cmd__csprng(int argc, const char **argv)
1515

1616
while (count) {
1717
unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf);
18-
if (csprng_bytes(buf, chunk) < 0) {
18+
if (csprng_bytes(buf, chunk, 0) < 0) {
1919
perror("failed to read");
2020
return 5;
2121
}

t/unit-tests/t-reftable-readwrite.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,8 @@ static void t_log_buffer_size(void)
108108
hash, to ensure that the compressed part is larger than the original.
109109
*/
110110
for (i = 0; i < REFTABLE_HASH_SIZE_SHA1; i++) {
111-
log.value.update.old_hash[i] = (uint8_t)(git_rand() % 256);
112-
log.value.update.new_hash[i] = (uint8_t)(git_rand() % 256);
111+
log.value.update.old_hash[i] = (uint8_t)(git_rand(0) % 256);
112+
log.value.update.new_hash[i] = (uint8_t)(git_rand(0) % 256);
113113
}
114114
reftable_writer_set_limits(w, update_index, update_index);
115115
err = reftable_writer_add_log(w, &log);
@@ -325,7 +325,7 @@ static void t_log_zlib_corruption(void)
325325
};
326326

327327
for (i = 0; i < sizeof(message) - 1; i++)
328-
message[i] = (uint8_t)(git_rand() % 64 + ' ');
328+
message[i] = (uint8_t)(git_rand(0) % 64 + ' ');
329329

330330
reftable_writer_set_limits(w, 1, 1);
331331

wrapper.c

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
479479
for (count = 0; count < TMP_MAX; ++count) {
480480
int i;
481481
uint64_t v;
482-
if (csprng_bytes(&v, sizeof(v)) < 0)
482+
if (csprng_bytes(&v, sizeof(v), 0) < 0)
483483
return error_errno("unable to get random bytes for temporary file");
484484

485485
/* Fill in the random bits. */
@@ -750,7 +750,7 @@ int open_nofollow(const char *path, int flags)
750750
#endif
751751
}
752752

753-
int csprng_bytes(void *buf, size_t len)
753+
int csprng_bytes(void *buf, size_t len, MAYBE_UNUSED unsigned flags)
754754
{
755755
#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD)
756756
/* This function never returns an error. */
@@ -785,14 +785,18 @@ int csprng_bytes(void *buf, size_t len)
785785
return -1;
786786
return 0;
787787
#elif defined(HAVE_OPENSSL_CSPRNG)
788-
int res = RAND_bytes(buf, len);
789-
if (res == 1)
788+
switch (RAND_pseudo_bytes(buf, len)) {
789+
case 1:
790790
return 0;
791-
if (res == -1)
792-
errno = ENOTSUP;
793-
else
791+
case 0:
792+
if (flags & CSPRNG_BYTES_INSECURE)
793+
return 0;
794794
errno = EIO;
795-
return -1;
795+
return -1;
796+
default:
797+
errno = ENOTSUP;
798+
return -1;
799+
}
796800
#else
797801
ssize_t res;
798802
char *p = buf;
@@ -816,11 +820,11 @@ int csprng_bytes(void *buf, size_t len)
816820
#endif
817821
}
818822

819-
uint32_t git_rand(void)
823+
uint32_t git_rand(unsigned flags)
820824
{
821825
uint32_t result;
822826

823-
if (csprng_bytes(&result, sizeof(result)) < 0)
827+
if (csprng_bytes(&result, sizeof(result), flags) < 0)
824828
die(_("unable to get random bytes"));
825829

826830
return result;

wrapper.h

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,18 +127,26 @@ int open_nofollow(const char *path, int flags);
127127

128128
void sleep_millisec(int millisec);
129129

130+
enum {
131+
/*
132+
* Accept insecure bytes, which some CSPRNG implementations may return
133+
* in case the entropy pool has been exhausted.
134+
*/
135+
CSPRNG_BYTES_INSECURE = (1 << 0),
136+
};
137+
130138
/*
131139
* Generate len bytes from the system cryptographically secure PRNG.
132140
* Returns 0 on success and -1 on error, setting errno. The inability to
133-
* satisfy the full request is an error.
141+
* satisfy the full request is an error. Accepts CSPRNG flags.
134142
*/
135-
int csprng_bytes(void *buf, size_t len);
143+
int csprng_bytes(void *buf, size_t len, unsigned flags);
136144

137145
/*
138146
* Returns a random uint32_t, uniformly distributed across all possible
139-
* values.
147+
* values. Accepts CSPRNG flags.
140148
*/
141-
uint32_t git_rand(void);
149+
uint32_t git_rand(unsigned flags);
142150

143151
/* Provide log2 of the given `size_t`. */
144152
static inline unsigned log2u(uintmax_t sz)

0 commit comments

Comments
 (0)