Use the Windows CSPRNG to get a random seed for Stdlib.Random

[MisterDA]

Windows (since Vista SP2 / Server 2008) provides a CSPRNG as the BCryptGenRandom function. Use it to generate Stdlib.Random default seed, mimicking the use of getentropy or /dev/urandom on Unix-like systems. See the Unix counter-part caml_unix_random_seed:

ocaml/runtime/sys.c

Lines 604 to 644 in 9caaced

	int caml_unix_random_seed(intnat data[16])
	{
	int n = 0;
	unsigned char buffer[12];
	int nread = 0;
	/* Try kernel entropy first */
	#ifdef HAS_GETENTROPY
	if (getentropy(buffer, 12) != -1) {
	nread = 12;
	} else
	#endif
	{ int fd = open("/dev/urandom", O_RDONLY, 0);
	if (fd != -1) {
	nread = read(fd, buffer, 12);
	close(fd);
	}
	}
	while (nread > 0) data[n++] = buffer[--nread];
	/* If the kernel provided enough entropy, we now have 96 bits
	of good random data and can stop here. */
	if (n >= 12) return n;
	/* Otherwise, complement whatever we got (probably nothing)
	with some not-very-random data. */
	{
	#ifdef HAS_GETTIMEOFDAY
	struct timeval tv;
	gettimeofday(&tv, NULL);
	if (n < 16) data[n++] = tv.tv_usec;
	if (n < 16) data[n++] = tv.tv_sec;
	#else
	if (n < 16) data[n++] = time(NULL);
	#endif
	#ifdef HAS_UNISTD
	if (n < 16) data[n++] = getpid();
	if (n < 16) data[n++] = getppid();
	#endif
	return n;
	}
	}

[nojb]

Why not memcpy(data, buffer, sizeof(buffer))?

Also, this loop can be put under the if above, and be made to follow by the "return" below (always unde the if).

[MisterDA]

Why not memcpy(data, buffer, sizeof(buffer))?

The function doesn't return the number of random bytes written to data, it returns the number of intnats that have random unsigned char in their low-byte. If I do a memcpy and then return 12, the final OCaml seed array will have 12 intnats, of which the first 12 bytes will be random data, and the rest (I guess) the stack values that were there (or zeroes?).

[nojb]

n is always zero if you reach here, or am I missing something?

[MisterDA]

Yes, you're right. BCryptGenRandom either writes 12 bytes and succeeds; or fails and n will be zero.

[MisterDA]

I agree with your comments @nojb. The idea behind the change was to copy the existing caml_unix_random_seed function as much as possible, which is why it looks a bit odd here (it's possible to get a partial read from /dev/urandom but not from getentropy or BCryptGenRandom). I guess I can simplify the code further.

ocaml/runtime/sys.c

Lines 628 to 668 in 6e80aa0

	int caml_unix_random_seed(intnat data[16])
	{
	int n = 0;
	unsigned char buffer[12];
	int nread = 0;
	/* Try kernel entropy first */
	#ifdef HAS_GETENTROPY
	if (getentropy(buffer, 12) != -1) {
	nread = 12;
	} else
	#endif
	{ int fd = open("/dev/urandom", O_RDONLY, 0);
	if (fd != -1) {
	nread = read(fd, buffer, 12);
	close(fd);
	}
	}
	while (nread > 0) data[n++] = buffer[--nread];
	/* If the kernel provided enough entropy, we now have 96 bits
	of good random data and can stop here. */
	if (n >= 12) return n;
	/* Otherwise, complement whatever we got (probably nothing)
	with some not-very-random data. */
	{
	#ifdef HAS_GETTIMEOFDAY
	struct timeval tv;
	gettimeofday(&tv, NULL);
	if (n < 16) data[n++] = tv.tv_usec;
	if (n < 16) data[n++] = tv.tv_sec;
	#else
	if (n < 16) data[n++] = time(NULL);
	#endif
	#ifndef _WIN32
	if (n < 16) data[n++] = getpid();
	if (n < 16) data[n++] = getppid();
	#endif
	return n;
	}
	}

One thing that surprised me with this code is that we don't fill the intnat data[16] buffer continuously, but sometimes only the low bytes of each intnat. I suppose it doesn't matter for the PRNG if the seed has zero bytes as long as some of its bytes are random.

In the end this function is a bit of a headache... so I went with the easy way, which was to copy caml_unix_random_seed as much as possible.

[nojb]

As far as I can see, the patch is correct. The changes to sys.c could have been omitted or put in a separate PR, but am not strongly opposed to them.

I am "informally" approving this PR, as the code is tricky, so a second pair of eyes would be appreciated before merging.

LGTM

[xavierleroy]

I remember that using bcrypt.lib was considered a long time ago, but rejected because it's a big library and linking with it would penalize all OCaml programs. (Not sure I remember correctly.)

Stdlib.Random is not cryptographically strong; do we really need to link with a whole crypto library to implement it?

[MisterDA]

I remember that using bcrypt.lib was considered a long time ago, but rejected because it's a big library and linking with it would penalize all OCaml programs. (Not sure I remember correctly.)

I see an increase of 1-2KB on the ocaml*.exe files when building this PR.

Stdlib.Random is not cryptographically strong; do we really need to link with a whole crypto library to implement it?

I think not, my intent was to have better parity with Unix which does use a CSPRNG to seed Stdlib.Random, and the PR was also prompted by the comments suggesting better ways to gather random bytes from Windows.

A 'wontfix' is a reasonable answer here, in which case I'll just update the comments.

[xavierleroy]

Thanks for the measurement. Searching the Web a bit, I see that Microsoft's crypto API and its implementation changed several times in the past, so my vague recollection of a possible code size problem with a crypto PRNG is certainly outdated, if not plain wrong.

I also see that there's a rand_s function in the CRT that draws from the same randomness source as the crypto API, but doesn't require to link with bcrypt. Three calls to rand_s would provide 96 bits of entropy, which is plenty. What do you think about this?

[MisterDA]

I also see that there's a rand_s function in the CRT

Not sure why or how I missed this one. I've changed my patch to use rand_s.

[nojb]

LGTM

[xavierleroy]

Nitpicking: it's OK to spread the 3 4-byte random integers over the first 12 integers of data, one byte at a time. But you could just as well write the return values of rand_s into data[0], data[1] and data[2], and stop there if the 3 calls to rand_s succeeded. In both cases you have 96 bits of entropy, and the Random module will make good use of these bits, no matter how they are spread over the elements of the data array.

[MisterDA]

I've implemented your suggestion, I hope correctly.

[xavierleroy]

You didn't have to change the code, but thanks for having changed it, the result is short and sweet.

I ran a chi^2 test on the output of caml_sys_random_seed. It passes with the new code using rand_s , and fails on the old code, which is as expected, and a confirmation that the new code is good.

Now merging...