Check fd status before using urandom

[goatgoose]

Resolved issues:

Resolves #4324.

Description of changes:

When s2n-tls is initialized, /dev/urandom is opened, which is later used retrieve entropy when generating random bytes. However, if this file descriptor is closed after initialization but before it's actually used, s2n-tls will currently loop forever trying to read from the closed file descriptor:

s2n-tls/utils/s2n_random.c

Lines 344 to 347 in edebdae

	while (n) {
	errno = 0;
	int r = read(entropy_fd, data, n);
	if (r <= 0) {

This PR implements a similar check to OpenSSL, where information about /dev/urandom is cached on initialization to be checked before actually retrieving entropy. If the current status of the file descriptor is different from when it was opened, /dev/urandom is re-opened.

Call-outs:

The fstat syscall is used to cache and check information about the file descriptor, which adds latency when generating random data. I ran performance tests with 500k handshakes on common cipher suites to determine the impact, and found that this change adds an average of 0.40% of latency to handshakes. The maximum latency added was 0.72% for AES128-GCM-SHA256. See the following performance data for details:

Performance data

cipher	duration (s)	duration with fstat (s)	% longer	duration with opening every invocation (s)	% longer
TLS_AES_128_GCM_SHA256	1535.92422	1542.00578	0.40%	1555.63746	1.28%
TLS_AES_256_GCM_SHA384	1546.50541	1550.69969	0.27%	1560.36224	0.90%
TLS_CHACHA20_POLY1305_SHA256	1538.59447	1543.15091	0.30%	1557.0791	1.20%
ECDHE-ECDSA-AES128-GCM-SHA256	1492.22279	1493.00317	0.05%	1508.55081	1.09%
ECDHE-RSA-AES128-GCM-SHA256	1519.45908	1522.37856	0.19%	1530.76816	0.74%
ECDHE-ECDSA-AES256-GCM-SHA384	1497.09047	1497.51423	0.03%	1513.45966	1.09%
ECDHE-RSA-AES256-GCM-SHA384	1525.16886	1527.90403	0.18%	1535.30494	0.66%
ECDHE-ECDSA-CHACHA20-POLY1305	1490.34668	1495.62126	0.35%	1503.83686	0.91%
ECDHE-RSA-CHACHA20-POLY1305	1516.00897	1523.96872	0.53%	1528.24489	0.81%
ECDHE-ECDSA-AES128-SHA	1489.28442	1493.72832	0.30%	1516.57031	1.83%
ECDHE-RSA-AES128-SHA	1514.6712	1522.00039	0.48%	1534.47172	1.31%
ECDHE-ECDSA-AES128-SHA256	1488.11378	1493.31757	0.35%	1509.76294	1.45%
ECDHE-RSA-AES128-SHA256	1512.44169	1522.69062	0.68%	1533.64569	1.40%
ECDHE-ECDSA-AES256-SHA	1485.98543	1493.17439	0.48%	1517.655	2.13%
ECDHE-RSA-AES256-SHA	1514.09495	1521.87596	0.51%	1538.35928	1.60%
AES128-GCM-SHA256	1390.37696	1400.42623	0.72%
AES128-SHA256	1393.46719	1402.81752	0.67%
AES128-SHA	1395.12007	1404.40474	0.67%

Testing:

I added a new random test that closes the urandom file descriptor before generating random data. Without the fix, this test loops forever in s2n_rand_urandom_impl(). I also added a test that swaps the file descriptor to ensure that /dev/urandom is re-opened.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[camshaft]

While you're doing performance benchmarks... I would be curious what it would look like if we opened and closed /dev/urandom on every call. If it's not that far off from what we're doing with the fstat in terms of syscall counts either. It would be much simpler to reason about and would let us get rid of more globals.

[goatgoose]

While you're doing performance benchmarks... I would be curious what it would look like if we opened and closed /dev/urandom on every call. If it's not that far off from what we're doing with the fstat in terms of syscall counts either. It would be much simpler to reason about and would let us get rid of more globals.

I updated the performance data table with data for this. It looks like opening and closing the file descriptor on every call is about 3x slower than opening once and using fstat to verify. The average latency increase for opening on every call is 1.23% vs 0.40% for fstat. The code is definitely cleaner without the global file descriptor though.

[lrstewart]

Nice test :)

[camshaft]

looks good! nice work