Feature #3129


rand macOs case handling update

Added by devnexen 15 days ago. Updated 14 days ago.

Target version:


- using new api instead of arc4random when possible (from Yosemite).
- fixing cmake build for other cases.


Actions #2

Updated by gstrauss 15 days ago

  • Category set to core
  • Priority changed from Normal to Low

Citations and references, please.

If lighttpd is linked against any crypto library, you can see that lighttpd prefers interfaces from those crypto libraries to obtain random bytes.

lighttpd uses random bytes in a very small number of places (besides the TLS modules), and if lighttpd is not built against any crypto libraries or with the Nettle library, then the remaining algorithms shipped with lighttpd for historical compatibility are weak (MD5 and SHA1).

I agree with the part of the patch which adds some missing defines to config.h.cmake, but please provide some justification about the benefits of adding your proposed code to src/rand.c. Improving arc4random is not much of an improvement if you were reaching the point in the code which falls back to use arc4random.

Actions #3

Updated by stbuehler 15 days ago

I think it falls back to random()...

The justification should include what the failure conditions of CCRandomGenerateBytes are - does it just return an error if it thinks the entropy was bad, but generated output anyway? Because falling back to random() sounds worse in that case.

Actions #5

Updated by devnexen 15 days ago

pb with arc4random you cannot know if it failed. otherwise we can always open /dev/urandom but might be heavyweight for this part of the code I think.

Actions #6

Updated by stbuehler 14 days ago

CCRandomGenerateBytes() seems to be based on ccrng (which is also used by the arc4random code):

It seems ccrng is basically a function pointer: int (*generate)(struct ccrng_state *rng, unsigned long outlen, void *out); although it returns an int the return value is unspecified in the header; can't blame anyone for not handling it - hopefully the function can't fail in the first place.

I think the only error CCRandomGenerateBytes() can reasonably return is kCCMemoryFailure - when the initial allocation of ccrng() fails (arc4_init() simply crashes in this case).

So... unless we care arc4random crashes the process on OOM I don't think there is anything to worry about; this crash should only happen in the initial call to li_rand_init(), not after fork().

(Also I wonder whether a workaround should use the underlying ccrng interface directly instead of CCRandomGenerateBytes)

Actions #7

Updated by devnexen 14 days ago

this function pointer returns 0 on success, now the vast majority of time it does not fail indeed and the buffers at play here are fairly small as well.

However if only the cmake fixes part is committed that would be already something, I will be fine if this is not entirely accepted.

About your last remark, I m not 100% sure if this is necessarily not risky (for ex they might deprecate it for a better api one day without notice as the ccrng api is found buggy or not up to the standards, as this api is not public exposed one (they are in private headers) generally it s for a reasonable intent).

Actions #8

Updated by gstrauss 14 days ago

I consider the following untested patch to be better encapsulated and easier to read. Only because it is encapsulated will I consider it as a marginal improvement over arc4random(), since the patch checks if CCRandomGenerateBytes() fails, which should not happen on a functioning system. With this patch, on systems with CCRandomGenerateBytes(), HAVE_ARC4RANDOM_BUF is manually undefined with #undef since arc4random() on those systems uses (the equivalent of) CCRandomGenerateBytes() under the hood, but without checking the function return status.

--- a/src/rand.c
+++ b/src/rand.c
@@ -81,6 +81,13 @@
 #include <sys/ioctl.h>
+#if defined(__APPLE__) && defined(__MACH__)
+#if __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 101000 /* OS X 10.10+ */
+#include <CommonCrypto/CommonRandom.h>

 /* Take some reasonable steps to attempt to *seed* random number generators with
  * cryptographically random data.  Some of these initialization routines may
@@ -239,6 +246,11 @@ static void li_rand_init (void)
     if (1 == li_rand_device_bytes((unsigned char *)xsubi, (int)sizeof(xsubi))) {
         u = ((unsigned int)xsubi[0] << 16) | xsubi[1];
+    else if (CCRandomGenerateBytes(xsubi, sizeof(xsubi)) == kCCSuccess
+             && CCRandomGenerateBytes(&u, sizeof(u)) == kCCSuccess) {
+    }
+  #endif
     else {
       #ifdef HAVE_ARC4RANDOM_BUF
         u = arc4random();
@@ -373,6 +385,11 @@ int li_rand_pseudo (void)
     if (SECSuccess == PK11_GenerateRandom((unsigned char *)&i, sizeof(i)))
         return i;
+    int i;
+    if (CCRandomGenerateBytes(&i, sizeof(i)) == kCCSuccess)
+        return i;
+  #endif
     return (int)arc4random();
   #elif defined(__COVERITY__)

The above patch does not address missing defines in src/config.h.cmake. That should be a separate patch.

Actions #9

Updated by devnexen 14 days ago

your patch works for me.

Actions #10

Updated by gstrauss 14 days ago

  • Status changed from New to Fixed
Actions #11

Updated by gstrauss 14 days ago

  • Target version changed from 1.4.xx to 1.4.64

Also available in: Atom