| Message ID | 20211116033542.3247094-2-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generate temporary files using a CSPRNG | expand |
On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote: > The order of options is also important here. On systems with > arc4random, which is most of the BSDs, we use that, since, except on > MirBSD, it uses ChaCha20, which is extremely fast, and sits entirely in > userspace, avoiding a system call. We then prefer getrandom over > getentropy, because the former has been available longer on Linux, and > finally, if none of those are available, we use /dev/urandom, because > most Unix-like operating systems provide that API. We prefer options > that don't involve device files when possible because those work in some > restricted environments where device files may not be available. I wonder if we'll need a low-quality fallback for older systems which don't even have /dev/urandom. Because it's going to be used in such a core part of the system (tempfiles), this basically becomes a hard requirement for using Git at all. I can't say I'm excited in general to be introducing a dependency like this, just because of the portability headaches. But it may be the least bad thing (especially if we can fall back to the existing behavior). One alternative would be to build on top of the system mkstemp(), which makes it libc's problem. I'm not sure if we'd run into problems there, though. > diff --git a/Makefile b/Makefile > index 12be39ac49..1d17021f59 100644 > --- a/Makefile > +++ b/Makefile > @@ -234,6 +234,14 @@ all:: > # Define NO_TRUSTABLE_FILEMODE if your filesystem may claim to support > # the executable mode bit, but doesn't really do so. > # > +# Define HAVE_ARC4RANDOM if your system has arc4random and arc4random_buf. > +# > +# Define HAVE_GETRANDOM if your system has getrandom. > +# > +# Define HAVE_GETENTROPY if your system has getentropy. > +# > +# Define HAVE_RTLGENRANDOM if your system has RtlGenRandom (Windows only). It seems like these will be mutually exclusive (and indeed, the #ifdef in the code ends up defining a particular precedence). Would we be better off exposing that to the user with a single CSPRNG_METHOD set to arc4random, getrandom, getentropy, etc? > diff --git a/config.mak.uname b/config.mak.uname > index 3236a4918a..5030d3c70b 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -257,6 +257,9 @@ ifeq ($(uname_S),FreeBSD) > HAVE_PATHS_H = YesPlease > HAVE_BSD_SYSCTL = YesPlease > HAVE_BSD_KERN_PROC_SYSCTL = YesPlease > + HAVE_ARC4RANDOM = YesPlease > + HAVE_GETRANDOM = YesPlease > + HAVE_GETENTROPY = YesPlease So here we claim to support a whole bunch of methods, but in practice, we only use arc4random, because these are all in an #elif chain: > +int csprng_bytes(void *buf, size_t len) > +{ > +#if defined(HAVE_ARC4RANDOM) > + arc4random_buf(buf, len); > + return 0; > +#elif defined(HAVE_GETRANDOM) though we still respect the others in other places, like including headers that we don't end up using: > +#ifdef HAVE_GETRANDOM > +#include <sys/random.h> > +#endif If csprng_bytes() could fallback between methods based on runtime errors, it would make sense to me to allow support for multiple methods to be declared. But without that, it just seems to invite confusion (and I am not sure runtime fallbacks are really worth the trouble). > +int csprng_bytes(void *buf, size_t len) > +{ > +#if defined(HAVE_ARC4RANDOM) > + arc4random_buf(buf, len); > + return 0; OK, presumably this one can't return an error, which is nice. > +#elif defined(HAVE_GETRANDOM) > + > + ssize_t res; > + char *p = buf; > + while (len) { > + res = getrandom(p, len, 0); > + if (res < 0) > + return -1; > + len -= res; > + p += res; > + } > + return 0; Do we ever have to worry about a "0" return from getrandom()? I'd expect it to block rather than return 0, but what I'm wondering is if we could ever be in a situation where we fail to make progress and loop infinitely. The manpage says that reads up to 256 bytes will always return the full output and never be interrupted. So for the caller you add in patch 2, we wouldn't need this loop. However, since cspring_bytes() is generic, being defensive makes sense. But in that case, do we need to handle EINTR when it returns -1? > +#elif defined(HAVE_GETENTROPY) > + int res; > + char *p = buf; > + while (len) { > + /* getentropy has a maximum size of 256 bytes. */ > + size_t chunk = len < 256 ? len : 256; > + res = getentropy(p, chunk); > + if (res < 0) > + return -1; > + len -= chunk; > + p += chunk; > + } > + return 0; Heh, I see that getentropy() punted on all of those questions above by just insisting you ask for 256 bytes at a time. Cute solution. ;) > +#elif defined(HAVE_RTLGENRANDOM) > + if (!RtlGenRandom(buf, len)) > + return -1; > + return 0; I have no comment on this one. :) > +#else > + ssize_t res; > + char *p = buf; > + int fd, err; > + fd = open("/dev/urandom", O_RDONLY); > + if (fd < 0) > + return -1; > + while (len) { > + res = xread(fd, p, len); > + if (res < 0) { > + err = errno; > + close(fd); > + errno = err; > + return -1; > + } > + len -= res; > + p += res; > + } > + close(fd); > + return 0; > +#endif > +} This loop is basically read_in_full(), except that it doesn't treat a "0" return as an EOF. I'm not sure if that's intentional (because we want to keep trying on a 0 return, though I'd expect the read to block in such a case), or if it would be an improvement (because it would prevent us from infinite looping if /dev/urandom wanted to signal EOF). -Peff
On November 16, 2021 10:31 AM, Jeff King wrote: > On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote: > > > The order of options is also important here. On systems with > > arc4random, which is most of the BSDs, we use that, since, except on > > MirBSD, it uses ChaCha20, which is extremely fast, and sits entirely > > in userspace, avoiding a system call. We then prefer getrandom over > > getentropy, because the former has been available longer on Linux, and > > finally, if none of those are available, we use /dev/urandom, because > > most Unix-like operating systems provide that API. We prefer options > > that don't involve device files when possible because those work in > > some restricted environments where device files may not be available. > > I wonder if we'll need a low-quality fallback for older systems which don't > even have /dev/urandom. Because it's going to be used in such a core part of > the system (tempfiles), this basically becomes a hard requirement for using > Git at all. > > I can't say I'm excited in general to be introducing a dependency like this, just > because of the portability headaches. But it may be the least bad thing > (especially if we can fall back to the existing behavior). > One alternative would be to build on top of the system mkstemp(), which > makes it libc's problem. I'm not sure if we'd run into problems there, though. None 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. -Randall
On Tue, Nov 16, 2021 at 11:01:20AM -0500, rsbecker@nexbridge.com wrote: > On November 16, 2021 10:31 AM, Jeff King wrote: > > On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote: > > > > > The order of options is also important here. On systems with > > > arc4random, which is most of the BSDs, we use that, since, except on > > > MirBSD, it uses ChaCha20, which is extremely fast, and sits entirely > > > in userspace, avoiding a system call. We then prefer getrandom over > > > getentropy, because the former has been available longer on Linux, and > > > finally, if none of those are available, we use /dev/urandom, because > > > most Unix-like operating systems provide that API. We prefer options > > > that don't involve device files when possible because those work in > > > some restricted environments where device files may not be available. > > > > I wonder if we'll need a low-quality fallback for older systems which don't > > even have /dev/urandom. Because it's going to be used in such a core part of > > the system (tempfiles), this basically becomes a hard requirement for using > > Git at all. > > > > I can't say I'm excited in general to be introducing a dependency like this, just > > because of the portability headaches. But it may be the least bad thing > > (especially if we can fall back to the existing behavior). > > One alternative would be to build on top of the system mkstemp(), which > > makes it libc's problem. I'm not sure if we'd run into problems there, though. > > None 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. I share Peff's lack of enthusiasm about the dependency situation. But making Git depend on having /dev/urandom available is simply not feasible, as you point out. I wonder if the suitable fall-back should be the existing behavior of git_mkstemps_mode()? That leaves us in a somewhat-disappointing situation of not having fully resolved the DOS attack on all platforms. But it makes our dependency situation less complicated, and leaves things no worse off than the were before on platforms like NonStop. Thanks, Taylor
On November 16, 2021 1:23 PM, Taylor Blau wrote: > On Tue, Nov 16, 2021 at 11:01:20AM -0500, rsbecker@nexbridge.com wrote: > > On November 16, 2021 10:31 AM, Jeff King wrote: > > > On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote: > > > > > > > The order of options is also important here. On systems with > > > > arc4random, which is most of the BSDs, we use that, since, except > > > > on MirBSD, it uses ChaCha20, which is extremely fast, and sits > > > > entirely in userspace, avoiding a system call. We then prefer > > > > getrandom over getentropy, because the former has been available > > > > longer on Linux, and finally, if none of those are available, we > > > > use /dev/urandom, because most Unix-like operating systems provide > > > > that API. We prefer options that don't involve device files when > > > > possible because those work in some restricted environments where > device files may not be available. > > > > > > I wonder if we'll need a low-quality fallback for older systems > > > which don't even have /dev/urandom. Because it's going to be used in > > > such a core part of the system (tempfiles), this basically becomes a > > > hard requirement for using Git at all. > > > > > > I can't say I'm excited in general to be introducing a dependency > > > like this, just because of the portability headaches. But it may be > > > the least bad thing (especially if we can fall back to the existing behavior). > > > One alternative would be to build on top of the system mkstemp(), > > > which makes it libc's problem. I'm not sure if we'd run into problems > there, though. > > > > None 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. > > I share Peff's lack of enthusiasm about the dependency situation. But making > Git depend on having /dev/urandom available is simply not feasible, as you > point out. > > I wonder if the suitable fall-back should be the existing behavior of > git_mkstemps_mode()? That leaves us in a somewhat-disappointing > situation of not having fully resolved the DOS attack on all platforms. > But it makes our dependency situation less complicated, and leaves things no > worse off than the were before on platforms like NonStop. The 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. -Randall
On 2021-11-16 at 16:01:20, rsbecker@nexbridge.com wrote: > On November 16, 2021 10:31 AM, Jeff King wrote: > > On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote: > > > > > The order of options is also important here. On systems with > > > arc4random, which is most of the BSDs, we use that, since, except on > > > MirBSD, it uses ChaCha20, which is extremely fast, and sits entirely > > > in userspace, avoiding a system call. We then prefer getrandom over > > > getentropy, because the former has been available longer on Linux, and > > > finally, if none of those are available, we use /dev/urandom, because > > > most Unix-like operating systems provide that API. We prefer options > > > that don't involve device files when possible because those work in > > > some restricted environments where device files may not be available. > > > > I wonder if we'll need a low-quality fallback for older systems which don't > > even have /dev/urandom. Because it's going to be used in such a core part of > > the system (tempfiles), this basically becomes a hard requirement for using > > Git at all. > > > > I can't say I'm excited in general to be introducing a dependency like this, just > > because of the portability headaches. But it may be the least bad thing > > (especially if we can fall back to the existing behavior). > > One alternative would be to build on top of the system mkstemp(), which > > makes it libc's problem. I'm not sure if we'd run into problems there, though. > > None 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. I'm really not excited about a fallback here, and I specifically did not include one for that reason. I'm happy to add an appropriate dependency on an OpenSSL or libgcrypt PRNG if you're linking against that already (e.g., for libcurl) or support for libbsd's arc4random or getentropy if that will work on your system. For example, how are you dealing with TLS connections over HTTPS? That library will almost certainly provide the required primitives in a straightforward and portable way. I do fundamentally believe every operating system and language environment need to provide a readily available CSPRNG in 2021, especially because in the vast majority of cases, hash tables must be randomized to avoid hash DoS attacks on untrusted input. I'm planning to look into our hash tables in the future to see if they are vulnerable to that kind of attack, and if so, we'll need to have a CSPRNG for basic security reasons, and platforms that can't provide one would be subject to a CVE. If we really can't find a solution, I won't object to a patch on top that adds an insecure fallback, but I don't want to put my name or sign-off on such a patch because I think it's a mistake. But I think we almost certainly can, though.
On November 16, 2021 5:42 PM, brian m. carlson > On 2021-11-16 at 16:01:20, rsbecker@nexbridge.com wrote: > > On November 16, 2021 10:31 AM, Jeff King wrote: > > > On Tue, Nov 16, 2021 at 03:35:41AM +0000, brian m. carlson wrote: > > > > > > > The order of options is also important here. On systems with > > > > arc4random, which is most of the BSDs, we use that, since, except > > > > on MirBSD, it uses ChaCha20, which is extremely fast, and sits > > > > entirely in userspace, avoiding a system call. We then prefer > > > > getrandom over getentropy, because the former has been available > > > > longer on Linux, and finally, if none of those are available, we > > > > use /dev/urandom, because most Unix-like operating systems provide > > > > that API. We prefer options that don't involve device files when > > > > possible because those work in some restricted environments where > device files may not be available. > > > > > > I wonder if we'll need a low-quality fallback for older systems > > > which don't even have /dev/urandom. Because it's going to be used in > > > such a core part of the system (tempfiles), this basically becomes a > > > hard requirement for using Git at all. > > > > > > I can't say I'm excited in general to be introducing a dependency > > > like this, just because of the portability headaches. But it may be > > > the least bad thing (especially if we can fall back to the existing behavior). > > > One alternative would be to build on top of the system mkstemp(), > > > which makes it libc's problem. I'm not sure if we'd run into problems > there, though. > > > > None 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. > > I'm really not excited about a fallback here, and I specifically did not include > one for that reason. I'm happy to add an appropriate dependency on an > OpenSSL or libgcrypt PRNG if you're linking against that already (e.g., for > libcurl) or support for libbsd's arc4random or getentropy if that will work on > your system. For example, how are you dealing with TLS connections over > HTTPS? That library will almost certainly provide the required primitives in a > straightforward and portable way. > > I do fundamentally believe every operating system and language > environment need to provide a readily available CSPRNG in 2021, especially > because in the vast majority of cases, hash tables must be randomized to > avoid hash DoS attacks on untrusted input. I'm planning to look into our hash > tables in the future to see if they are vulnerable to that kind of attack, and if > so, we'll need to have a CSPRNG for basic security reasons, and platforms > that can't provide one would be subject to a CVE. > > If we really can't find a solution, I won't object to a patch on top that adds an > insecure fallback, but I don't want to put my name or sign-off on such a patch > because I think it's a mistake. But I think we almost certainly can, though. 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. -- Randall Becker Also from the GTA
On Tue, Nov 16, 2021 at 4:01 PM <rsbecker@nexbridge.com> wrote: > > 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. Older versions of OpenSSL exported (AFAIK) a usable version of arc4random_buf() that could have helped here; it seems to still be there in libressl[1] which is mostly API compatible and might be worth looking into IMHO even if as you pointed out will need an implementation similar to what OpenSSL does internally. [1] https://cvsweb.openbsd.org/src/lib/libcrypto/arc4random/
On 2021-11-16 at 23:20:45, rsbecker@nexbridge.com wrote: > 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. Great, as long as you don't define NO_OPENSSL, I think I can make this work with OpenSSL by calling RAND_bytes, which will use whatever OpenSSL uses. I'll work on that for a v2 to see if that will meet the needs for your platform, and if not, I'll try something else. That should also have the pleasant side effect of making this more portable even for those people who do have less common platforms, since OpenSSL will likely be an option there.
On Tue, Nov 16, 2021 at 5:04 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > On 2021-11-16 at 23:20:45, rsbecker@nexbridge.com wrote: > > 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. > > Great, as long as you don't define NO_OPENSSL, I think I can make this > work with OpenSSL by calling RAND_bytes, which will use whatever OpenSSL > uses. not that RAND_bytes return high entropy bytes (like /dev/random) and is therefore limited and prone to draining, blocking and erroring when drained, so if we are going this route, will most likely need a second layer on top that doesn't block (like arc4random does), and at that point I would think we would rather use something battle tested than our own. for the little amount of random data we need, it might be wiser to fallback to something POSIX like lrand48 which is most likely to be available, but of course your tests that consume lots of random data will need to change. Carlo PS. Probably missing context as I don't know what was discussed previously, but indeed making this the libc problem by using mkstemp (plus some compatibility on top), like Peff mentioned seems like a more straightforward "fix" I'll work on that for a v2 to see if that will meet the needs for > your platform, and if not, I'll try something else. > > That should also have the pleasant side effect of making this more > portable even for those people who do have less common platforms, since > OpenSSL will likely be an option there. > -- > brian m. carlson (he/him or they/them) > Toronto, Ontario, CA
On November 16, 2021 8:03 PM, brian m. carlson wrote: > On 2021-11-16 at 23:20:45, rsbecker@nexbridge.com wrote: > > 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. > > Great, as long as you don't define NO_OPENSSL, I think I can make this work > with OpenSSL by calling RAND_bytes, which will use whatever OpenSSL uses. > I'll work on that for a v2 to see if that will meet the needs for your platform, > and if not, I'll try something else. > > That should also have the pleasant side effect of making this more portable > even for those people who do have less common platforms, since OpenSSL > will likely be an option there. I checked config.mak.uname. We should be fine with that qualification. Regards, Randall
On Tue, Nov 16, 2021 at 05:50:44PM -0800, Carlo Arenas wrote: > for the little amount of random data we need, it might be wiser to > fallback to something POSIX like lrand48 which is most likely to be > available, but of course your tests that consume lots of random data > will need to change. Unfortunately that won't help. You have to seed lrand48 with something, which usually means pid and/or timestamp. Which are predictable to an attacker, which was the start of the whole conversation. You really need _some_ source of entropy, and only the OS can provide that. > PS. Probably missing context as I don't know what was discussed > previously, but indeed making this the libc problem by using mkstemp > (plus some compatibility on top), like Peff mentioned seems like a > more straightforward "fix" It might be nice if it works. I don't recall all of the reasons that led us to implement our own mkstemp in the first place. So the first step would probably be digging in the history and the archive to find that out, and whether it still applies. -Peff
On November 16, 2021 7:48 PM, Carlo Arenas wrote: > On Tue, Nov 16, 2021 at 4:01 PM <rsbecker@nexbridge.com> wrote: > > > > 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. > > Older versions of OpenSSL exported (AFAIK) a usable version of > arc4random_buf() that could have helped here; it seems to still be there in > libressl[1] which is mostly API compatible and might be worth looking into > IMHO even if as you pointed out will need an implementation similar to what > OpenSSL does internally. > > [1] https://cvsweb.openbsd.org/src/lib/libcrypto/arc4random/ I 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.
On November 16, 2021 10:04 PM, Jeff King wrote: > On Tue, Nov 16, 2021 at 05:50:44PM -0800, Carlo Arenas wrote: > > > for the little amount of random data we need, it might be wiser to > > fallback to something POSIX like lrand48 which is most likely to be > > available, but of course your tests that consume lots of random data > > will need to change. > > Unfortunately that won't help. You have to seed lrand48 with something, > which usually means pid and/or timestamp. Which are predictable to an > attacker, which was the start of the whole conversation. You really need > _some_ source of entropy, and only the OS can provide that. > > > PS. Probably missing context as I don't know what was discussed > > previously, but indeed making this the libc problem by using mkstemp > > (plus some compatibility on top), like Peff mentioned seems like a > > more straightforward "fix" > > It might be nice if it works. I don't recall all of the reasons that led us to > implement our own mkstemp in the first place. So the first step would > probably be digging in the history and the archive to find that out, and > whether it still applies. mkstemp 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. --Randall
On Tue, Nov 16, 2021 at 7:04 PM Jeff King <peff@peff.net> wrote: > > On Tue, Nov 16, 2021 at 05:50:44PM -0800, Carlo Arenas wrote: > > > for the little amount of random data we need, it might be wiser to > > fallback to something POSIX like lrand48 which is most likely to be > > available, but of course your tests that consume lots of random data > > will need to change. > > Unfortunately that won't help. You have to seed lrand48 with something, > which usually means pid and/or timestamp. Which are predictable to an > attacker, which was the start of the whole conversation. You really need > _some_ source of entropy, and only the OS can provide that. again, showing my ignorance here; but that "something" doesn't need to be guessable externally; ex: git add could use as seed contents from the file that is adding, or even better mix it up with the other sources as a poor man's /dev/urandom I agree though that having a true random source will require the OS, but isn't it about generating 6 random letters? Carlo
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > Finally, add a self-test option here to make sure that our buffer > handling is correct and we aren't truncating data. We simply read 64 > KiB and then make sure we've seen each byte. The probability of this > test failing spuriously is less than 10^-100. I saw that 10^-100 math in the other message, and have no problem with that, but I am not sure how such a test makes "sure that our buffer handling is correct and we aren't truncating data." If you thought you are generate 64kiB of random bytes but a bug caused you to actually use 32kiB of random bytes with 32kiB of other garbage, wouldn't you still have enough entropy left that you would be likely to paint all 256 buckets? I also agree with Peff's comment about making these look as if many of them can be specified at once, when only one of them would actually be in effect. Giving one Makefile macro that the builder can set to a single value would be much less confusing. Thanks.
On Tue, Nov 16, 2021 at 07:36:51PM -0800, Carlo Arenas wrote: > > > for the little amount of random data we need, it might be wiser to > > > fallback to something POSIX like lrand48 which is most likely to be > > > available, but of course your tests that consume lots of random data > > > will need to change. > > > > Unfortunately that won't help. You have to seed lrand48 with something, > > which usually means pid and/or timestamp. Which are predictable to an > > attacker, which was the start of the whole conversation. You really need > > _some_ source of entropy, and only the OS can provide that. > > again, showing my ignorance here; but that "something" doesn't need to > be guessable externally; ex: git add could use as seed contents from > the file that is adding, or even better mix it up with the other > sources as a poor man's /dev/urandom Those contents are still predictable. So you've made the attacker's job a little harder (now they have to block tempfiles for, say, each tag you're going to verify), but haven't changed the fundamental problem. It definitely would help in _some_ threat models, but I think we should strive for a solution that can be explained clearly as "nobody can DoS your tempfiles" without complicated qualifications. -Peff
On November 17, 2021 3:02 PM, Jeff King wrote: > On Tue, Nov 16, 2021 at 07:36:51PM -0800, Carlo Arenas wrote: > > > > > for the little amount of random data we need, it might be wiser to > > > > fallback to something POSIX like lrand48 which is most likely to > > > > be available, but of course your tests that consume lots of random > > > > data will need to change. > > > > > > Unfortunately that won't help. You have to seed lrand48 with > > > something, which usually means pid and/or timestamp. Which are > > > predictable to an attacker, which was the start of the whole > > > conversation. You really need _some_ source of entropy, and only the OS > can provide that. > > > > again, showing my ignorance here; but that "something" doesn't need to > > be guessable externally; ex: git add could use as seed contents from > > the file that is adding, or even better mix it up with the other > > sources as a poor man's /dev/urandom > > Those contents are still predictable. So you've made the attacker's job a little > harder (now they have to block tempfiles for, say, each tag you're going to > verify), but haven't changed the fundamental problem. > > It definitely would help in _some_ threat models, but I think we should strive > for a solution that can be explained clearly as "nobody can DoS your > tempfiles" without complicated qualifications. I missed this one... lrand48 is also not generally available. I don’t think it is even available on Windows. 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. Randall
On 2021-11-17 at 07:39:08, Junio C Hamano wrote: > "brian m. carlson" <sandals@crustytoothpaste.net> writes: > > > Finally, add a self-test option here to make sure that our buffer > > handling is correct and we aren't truncating data. We simply read 64 > > KiB and then make sure we've seen each byte. The probability of this > > test failing spuriously is less than 10^-100. > > I saw that 10^-100 math in the other message, and have no problem > with that, but I am not sure how such a test makes "sure that our > buffer handling is correct and we aren't truncating data." If you > thought you are generate 64kiB of random bytes but a bug caused you > to actually use 32kiB of random bytes with 32kiB of other garbage, > wouldn't you still have enough entropy left that you would be likely > to paint all 256 buckets? True, but our code processes smaller chunks at a time, which means that theoretically we'd notice before then. For example, getentropy(2) won't process chunks larger than 256 bytes. If we don't think there's value, I can just remove it. > I also agree with Peff's comment about making these look as if many > of them can be specified at once, when only one of them would > actually be in effect. Giving one Makefile macro that the builder > can set to a single value would be much less confusing. I can use one Makefile macro, sure. I think we'll still need multiple macros for the actual C code because we can't really do a string comparison in the C preprocessor.
On 2021-11-17 at 20:19:49, rsbecker@nexbridge.com wrote: > I missed this one... lrand48 is also not generally available. I don’t think it is even available on Windows. > > 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. I think in this case I'd like to try to stick with OpenSSL or other standard interfaces if that's going to meet folks' needs. I can write an HMAC-DRBG, but getting entropy is the tricky part, and jitterentropy approaches are controversial because it's not clear how unpredictable they are. I'm also specifically trying to avoid anything that's architecture specific like RDRAND, since that means we have to carry assembly code, and on some systems RDRAND is broken, which means that you have to test for that and then pass the output into another CSPRNG. I'm also not sure how maintainable such code is, since I don't think there are many people on the list who would be familiar enough with those algorithms to maintain it. Plus there's always the rule, "Don't write your own crypto." Using OpenSSL or system-provided interfaces is much, much easier, it means users can use Git in FIPS-certified environments, and it avoids us ending up with subtly broken code in the future.
On November 17, 2021 6:31 PM, brian m. carlson wrote: > To: rsbecker@nexbridge.com > Cc: 'Jeff King' <peff@peff.net>; 'Carlo Arenas' <carenas@gmail.com>; > git@vger.kernel.org > Subject: Re: [PATCH 1/2] wrapper: add a helper to generate numbers from a > CSPRNG > > On 2021-11-17 at 20:19:49, rsbecker@nexbridge.com wrote: > > I missed this one... lrand48 is also not generally available. I don’t think it is > even available on Windows. > > > > 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. > > I think in this case I'd like to try to stick with OpenSSL or other standard > interfaces if that's going to meet folks' needs. I can write an HMAC-DRBG, > but getting entropy is the tricky part, and jitterentropy approaches are > controversial because it's not clear how unpredictable they are. I'm also > specifically trying to avoid anything that's architecture specific like RDRAND, > since that means we have to carry assembly code, and on some systems > RDRAND is broken, which means that you have to test for that and then pass > the output into another CSPRNG. > I'm also not sure how maintainable such code is, since I don't think there are > many people on the list who would be familiar enough with those algorithms > to maintain it. Plus there's always the rule, "Don't write your own crypto." > > Using OpenSSL or system-provided interfaces is much, much easier, it means > users can use Git in FIPS-certified environments, and it avoids us ending up > with subtly broken code in the future. I 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. Regards, Randall
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2021-11-17 at 07:39:08, Junio C Hamano wrote: >> "brian m. carlson" <sandals@crustytoothpaste.net> writes: >> >> > Finally, add a self-test option here to make sure that our buffer >> > handling is correct and we aren't truncating data. We simply read 64 >> > KiB and then make sure we've seen each byte. The probability of this >> > test failing spuriously is less than 10^-100. >> >> I saw that 10^-100 math in the other message, and have no problem >> with that, but I am not sure how such a test makes "sure that our >> buffer handling is correct and we aren't truncating data." If you >> thought you are generate 64kiB of random bytes but a bug caused you >> to actually use 32kiB of random bytes with 32kiB of other garbage, >> wouldn't you still have enough entropy left that you would be likely >> to paint all 256 buckets? > > True, but our code processes smaller chunks at a time, which means that > theoretically we'd notice before then. For example, getentropy(2) won't > process chunks larger than 256 bytes. Sorry, you lost me. > If we don't think there's value, I can just remove it. It is not that I do not think there is value. I am not sure where this code is getting its value from. We grab 1k at a time and repeat that 64 times. Presumably csprn_bytes() grabs bytes from underlying mechanism in smaller chunk, but would not return until it fills the buffer---ah, your "make sure our buffer handling is correct" is primarily about the check that we get full 1k bytes in the loop? We ask 1k chunk 64 times and we must get full 1k chunk every time? What I was wondering about was the other half of the check, ensuring all buckets[] are painted that gave us the cute 10^-100 math. + int buckets[256] = { 0 }; + unsigned char buf[1024]; + unsigned long count = 64 * 1024; + int i; + + while (count) { + if (csprng_bytes(buf, sizeof(buf)) < 0) { + perror("failed to read"); + return 3; + } + for (i = 0; i < sizeof(buf); i++) + buckets[buf[i]]++; + count -= sizeof(buf); + }
On 2021-11-18 at 07:19:08, Junio C Hamano wrote: > Presumably csprn_bytes() grabs bytes from underlying mechanism in > smaller chunk, but would not return until it fills the buffer---ah, > your "make sure our buffer handling is correct" is primarily about > the check that we get full 1k bytes in the loop? We ask 1k chunk 64 > times and we must get full 1k chunk every time? Yes, that's what we'd expect to happen. > What I was wondering about was the other half of the check, ensuring > all buckets[] are painted that gave us the cute 10^-100 math. Say the buffer handling is incorrect and we read only a few bytes instead of the full 1 KiB. Then we'll end up filling only some of the buckets, and the check will fail much of the time, because we won't get sufficient number of random bytes to fill all the buckets. The check is that we got enough data that looks like random bytes over the course of our requests.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2021-11-18 at 07:19:08, Junio C Hamano wrote: >> Presumably csprn_bytes() grabs bytes from underlying mechanism in >> smaller chunk, but would not return until it fills the buffer---ah, >> your "make sure our buffer handling is correct" is primarily about >> the check that we get full 1k bytes in the loop? We ask 1k chunk 64 >> times and we must get full 1k chunk every time? > > Yes, that's what we'd expect to happen. > >> What I was wondering about was the other half of the check, ensuring >> all buckets[] are painted that gave us the cute 10^-100 math. > > Say the buffer handling is incorrect and we read only a few bytes > instead of the full 1 KiB. Then we'll end up filling only some of the > buckets, and the check will fail much of the time, because we won't get > sufficient number of random bytes to fill all the buckets. ... meaning (64 * a few bytes) is small enough such that some slots in buckets[] will be left untouched (and the remainder of 1kB is untouched --- but the buffer[] is not initialized in any way, so it's not like such an "oops, we only fed a few bytes" bug would leave the rest to NUL or anything)? > The check is that we got enough data that looks like random bytes over > the course of our requests. If the check were doing so, yes, I would have understood (whether I agreed with it or not), but the check is "if we taint each and every bucket[] even once, we are OK", not "bucket[] should be more or less evenly touched", and that is why I do/did not understand the test.
diff --git a/Makefile b/Makefile index 12be39ac49..1d17021f59 100644 --- a/Makefile +++ b/Makefile @@ -234,6 +234,14 @@ all:: # Define NO_TRUSTABLE_FILEMODE if your filesystem may claim to support # the executable mode bit, but doesn't really do so. # +# Define HAVE_ARC4RANDOM if your system has arc4random and arc4random_buf. +# +# Define HAVE_GETRANDOM if your system has getrandom. +# +# Define HAVE_GETENTROPY if your system has getentropy. +# +# Define HAVE_RTLGENRANDOM if your system has RtlGenRandom (Windows only). +# # Define NEEDS_MODE_TRANSLATION if your OS strays from the typical file type # bits in mode values (e.g. z/OS defines I_SFMT to 0xFF000000 as opposed to the # usual 0xF000). @@ -694,6 +702,7 @@ TEST_BUILTINS_OBJS += test-bloom.o TEST_BUILTINS_OBJS += test-chmtime.o TEST_BUILTINS_OBJS += test-config.o TEST_BUILTINS_OBJS += test-crontab.o +TEST_BUILTINS_OBJS += test-csprng.o TEST_BUILTINS_OBJS += test-ctype.o TEST_BUILTINS_OBJS += test-date.o TEST_BUILTINS_OBJS += test-delta.o @@ -1900,6 +1909,22 @@ ifdef HAVE_GETDELIM BASIC_CFLAGS += -DHAVE_GETDELIM endif +ifdef HAVE_ARC4RANDOM + BASIC_CFLAGS += -DHAVE_ARC4RANDOM +endif + +ifdef HAVE_GETRANDOM + BASIC_CFLAGS += -DHAVE_GETRANDOM +endif + +ifdef HAVE_GETENTROPY + BASIC_CFLAGS += -DHAVE_GETENTROPY +endif + +ifdef HAVE_RTLGENRANDOM + BASIC_CFLAGS += -DHAVE_RTLGENRANDOM +endif + ifneq ($(PROCFS_EXECUTABLE_PATH),) procfs_executable_path_SQ = $(subst ','\'',$(PROCFS_EXECUTABLE_PATH)) BASIC_CFLAGS += '-DPROCFS_EXECUTABLE_PATH="$(procfs_executable_path_SQ)"' diff --git a/compat/winansi.c b/compat/winansi.c index c27b20a79d..0e5a9cc82e 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -3,6 +3,12 @@ */ #undef NOGDI + +/* + * Including the appropriate header file for RtlGenRandom causes MSVC to see a + * redefinition of types in an incompatible way when including headers below. + */ +#undef HAVE_RTLGENRANDOM #include "../git-compat-util.h" #include <wingdi.h> #include <winreg.h> diff --git a/config.mak.uname b/config.mak.uname index 3236a4918a..5030d3c70b 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -257,6 +257,9 @@ ifeq ($(uname_S),FreeBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + HAVE_ARC4RANDOM = YesPlease + HAVE_GETRANDOM = YesPlease + HAVE_GETENTROPY = YesPlease PAGER_ENV = LESS=FRX LV=-c MORE=FRX FREAD_READS_DIRECTORIES = UnfortunatelyYes FILENO_IS_A_MACRO = UnfortunatelyYes @@ -271,6 +274,8 @@ ifeq ($(uname_S),OpenBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + HAVE_ARC4RANDOM = YesPlease + HAVE_GETENTROPY = YesPlease PROCFS_EXECUTABLE_PATH = /proc/curproc/file FREAD_READS_DIRECTORIES = UnfortunatelyYes FILENO_IS_A_MACRO = UnfortunatelyYes @@ -282,6 +287,7 @@ ifeq ($(uname_S),MirBSD) NEEDS_LIBICONV = YesPlease HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease + HAVE_ARC4RANDOM = YesPlease endif ifeq ($(uname_S),NetBSD) ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2) @@ -293,6 +299,7 @@ ifeq ($(uname_S),NetBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + HAVE_ARC4RANDOM = YesPlease PROCFS_EXECUTABLE_PATH = /proc/curproc/exe endif ifeq ($(uname_S),AIX) @@ -422,6 +429,7 @@ ifeq ($(uname_S),Windows) NO_STRTOUMAX = YesPlease NO_MKDTEMP = YesPlease NO_INTTYPES_H = YesPlease + HAVE_RTLGENRANDOM = YesPlease # VS2015 with UCRT claims that snprintf and friends are C99 compliant, # so we don't need this: # @@ -624,6 +632,7 @@ ifeq ($(uname_S),MINGW) NO_POSIX_GOODIES = UnfortunatelyYes DEFAULT_HELP_FORMAT = html HAVE_PLATFORM_PROCINFO = YesPlease + HAVE_RTLGENRANDOM = YesPlease BASIC_LDFLAGS += -municode COMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32 COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index fd1399c440..134e00bde3 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -260,7 +260,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows") _CONSOLE DETECT_MSYS_TTY STRIP_EXTENSION=".exe" NO_SYMLINK_HEAD UNRELIABLE_FSTAT NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 USE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP - UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET) + UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET HAVE_RTLGENRANDOM) list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c compat/win32/path-utils.c compat/win32/pthread.c compat/win32mmap.c compat/win32/syslog.c compat/win32/trace2_win32_process_info.c compat/win32/dirent.c diff --git a/git-compat-util.h b/git-compat-util.h index d70ce14286..f2cff656e7 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -165,6 +165,12 @@ #endif #include <windows.h> #define GIT_WINDOWS_NATIVE +#ifdef HAVE_RTLGENRANDOM +/* This is required to get access to RtlGenRandom. */ +#define SystemFunction036 NTAPI SystemFunction036 +#include <NTSecAPI.h> +#undef SystemFunction036 +#endif #endif #include <unistd.h> @@ -235,6 +241,9 @@ #else #include <stdint.h> #endif +#ifdef HAVE_GETRANDOM +#include <sys/random.h> +#endif #ifdef NO_INTPTR_T /* * On I16LP32, ILP32 and LP64 "long" is the safe bet, however @@ -1381,4 +1390,11 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset) void sleep_millisec(int millisec); +/* + * Generate len bytes from the system cryptographically secure PRNG. + * Returns 0 on success and -1 on error, setting errno. The inability to + * satisfy the full request is an error. + */ +int csprng_bytes(void *buf, size_t len); + #endif diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c new file mode 100644 index 0000000000..196c14e44f --- /dev/null +++ b/t/helper/test-csprng.c @@ -0,0 +1,63 @@ +#include "test-tool.h" +#include "git-compat-util.h" + +/* + * Check that we read each byte value at least once when reading 64 KiB from the + * CSPRNG. This is not to test the quality of the CSPRNG, but to test our + * buffer handling of it. + * + * The probability of this failing by random is less than 10^-100. + */ +static int selftest(void) +{ + int buckets[256] = { 0 }; + unsigned char buf[1024]; + unsigned long count = 64 * 1024; + int i; + + while (count) { + if (csprng_bytes(buf, sizeof(buf)) < 0) { + perror("failed to read"); + return 3; + } + for (i = 0; i < sizeof(buf); i++) + buckets[buf[i]]++; + count -= sizeof(buf); + } + for (i = 0; i < ARRAY_SIZE(buckets); i++) + if (!buckets[i]) { + fprintf(stderr, "failed to find any bytes with value %02x\n", i); + return 4; + } + return 0; +} + +int cmd__csprng(int argc, const char **argv) +{ + unsigned long count; + unsigned char buf[1024]; + + if (argc > 2) { + fprintf(stderr, "usage: %s [--selftest | <size>]\n", argv[0]); + return 2; + } + + if (!strcmp(argv[1], "--selftest")) { + return selftest(); + } + + count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L; + + while (count) { + unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf); + if (csprng_bytes(buf, chunk) < 0) { + perror("failed to read"); + return 5; + } + if (fwrite(buf, chunk, 1, stdout) != chunk) + return 1; + count -= chunk; + } + + return 0; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 3ce5585e53..fc0fb86c1b 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -20,6 +20,7 @@ static struct test_cmd cmds[] = { { "chmtime", cmd__chmtime }, { "config", cmd__config }, { "crontab", cmd__crontab }, + { "csprng", cmd__csprng }, { "ctype", cmd__ctype }, { "date", cmd__date }, { "delta", cmd__delta }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 9f0f522850..077d9bfcca 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -10,6 +10,7 @@ int cmd__bloom(int argc, const char **argv); int cmd__chmtime(int argc, const char **argv); int cmd__config(int argc, const char **argv); int cmd__crontab(int argc, const char **argv); +int cmd__csprng(int argc, const char **argv); int cmd__ctype(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index b007f0efef..9647ec9629 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -1131,4 +1131,8 @@ test_expect_success 'test_must_fail rejects a non-git command with env' ' grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err ' +test_expect_success 'CSPRNG handling functions correctly' ' + test-tool csprng --selftest +' + test_done diff --git a/wrapper.c b/wrapper.c index 36e12119d7..0046f32e46 100644 --- a/wrapper.c +++ b/wrapper.c @@ -702,3 +702,59 @@ int open_nofollow(const char *path, int flags) return open(path, flags); #endif } + +int csprng_bytes(void *buf, size_t len) +{ +#if defined(HAVE_ARC4RANDOM) + arc4random_buf(buf, len); + return 0; +#elif defined(HAVE_GETRANDOM) + ssize_t res; + char *p = buf; + while (len) { + res = getrandom(p, len, 0); + if (res < 0) + return -1; + len -= res; + p += res; + } + return 0; +#elif defined(HAVE_GETENTROPY) + int res; + char *p = buf; + while (len) { + /* getentropy has a maximum size of 256 bytes. */ + size_t chunk = len < 256 ? len : 256; + res = getentropy(p, chunk); + if (res < 0) + return -1; + len -= chunk; + p += chunk; + } + return 0; +#elif defined(HAVE_RTLGENRANDOM) + if (!RtlGenRandom(buf, len)) + return -1; + return 0; +#else + ssize_t res; + char *p = buf; + int fd, err; + fd = open("/dev/urandom", O_RDONLY); + if (fd < 0) + return -1; + while (len) { + res = xread(fd, p, len); + if (res < 0) { + err = errno; + close(fd); + errno = err; + return -1; + } + len -= res; + p += res; + } + close(fd); + return 0; +#endif +}
There are many situations in which having access to a cryptographically secure pseudorandom number generator (CSPRNG) is helpful. In the future, we'll encounter one of these when dealing with temporary files. To make this possible, let's add a function which reads from a system CSPRNG and returns some bytes. Because this is a security sensitive interface, we take some precautions. We either succeed by filling the buffer completely as we requested, or we fail. We don't return partial data because the caller will almost never find that to be a useful behavior. The order of options is also important here. On systems with arc4random, which is most of the BSDs, we use that, since, except on MirBSD, it uses ChaCha20, which is extremely fast, and sits entirely in userspace, avoiding a system call. We then prefer getrandom over getentropy, because the former has been available longer on Linux, and finally, if none of those are available, we use /dev/urandom, because most Unix-like operating systems provide that API. We prefer options that don't involve device files when possible because those work in some restricted environments where device files may not be available. macOS appears to have arc4random but not the arc4random_buf function we want to use, so we let it use the fallback of /dev/urandom. Set the configuration variables appropriately for Linux and the other BSDs. We specifically only consider versions which receive publicly available security support; for example, getrandom(2) and getentropy(3) are only available in FreeBSD 12, which is the oldest version with current security support. For the same reason, we don't specify getrandom(2) on Linux, because CentOS 7 doesn't support it in glibc (although its kernel does) and we don't want to resort to making syscalls. Finally, add a self-test option here to make sure that our buffer handling is correct and we aren't truncating data. We simply read 64 KiB and then make sure we've seen each byte. The probability of this test failing spuriously is less than 10^-100. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- Makefile | 25 ++++++++++++ compat/winansi.c | 6 +++ config.mak.uname | 9 +++++ contrib/buildsystems/CMakeLists.txt | 2 +- git-compat-util.h | 16 ++++++++ t/helper/test-csprng.c | 63 +++++++++++++++++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0000-basic.sh | 4 ++ wrapper.c | 56 +++++++++++++++++++++++++ 10 files changed, 182 insertions(+), 1 deletion(-) create mode 100644 t/helper/test-csprng.c