/dev/urandom fd leaks to subprocesses

[martinetd]

Problem:

Using https://github.com/awslabs/aws-crt-python /dev/urandom apparently stays open through the lifetime of the application.
Executing subprocesses (or re-execing) leaves the fd open.

Solution:

urandom should be opened with O_CLOEXEC:

diff --git a/utils/s2n_random.c b/utils/s2n_random.c
index 295c87006f39..9ba0f83233bf 100755
--- a/utils/s2n_random.c
+++ b/utils/s2n_random.c
@@ -327,7 +327,7 @@ RAND_METHOD s2n_openssl_rand_method = {
 static int s2n_rand_init_impl(void)
 {
   OPEN:
-    entropy_fd = open(ENTROPY_SOURCE, O_RDONLY);
+    entropy_fd = open(ENTROPY_SOURCE, O_RDONLY|O_CLOXEC);
     if (entropy_fd == S2N_FAILURE) {
         if (errno == EINTR) {
             goto OPEN;

• Does this change what S2N sends over the wire? No
• Does this change any public APIs? No
• Which versions of TLS will this impact? None

Requirements / Acceptance Criteria:

Check that the fd is closed after exec?

• RFC links: N/A
• Related Issues: N/A
• Will the Usage Guide or other documentation need to be updated? No
• Testing: Probably does not require a new test... Existing tests should be enough to make sure O_CLOEXEC is supported on all platforms s2n-tls supports (I didn't take the time to check, sorry); if that is the case the rest should be robust enough.
  • Will this change trigger SAW changes? No
  • Should this change be fuzz tested? No

Thanks!

[dougch]

Thanks for the issue! It looks like valgrind can spot these, looking to see if that's a quick update. Curious how you found this?

[dougch]

working up a PR, adding --track-fds to valgrind does find this

Running s2n_fork_generation_number_test.c...
==5763== FILE DESCRIPTORS: 4 open at exit.
==5763== Open file descriptor 3: /dev/urandom
==5763==    at 0x5387E1B: open (open64.c:47)
==5763==    by 0x15B9ED: open (fcntl2.h:53)
==5763==    by 0x15B9ED: s2n_rand_init_impl (s2n_random.c:378)
==5763==    by 0x15BE8E: s2n_rand_init (s2n_random.c:395)
==5763==    by 0x12411A: s2n_init (s2n_init.c:71)
==5763==    by 0x12282F: s2n_test_case_madv_wipeonfork_cb (s2n_fork_generation_number_test.c:295)
==5763==    by 0x12282F: s2n_test_case_madv_wipeonfork_cb (s2n_fork_generation_number_test.c:287)
==5763==    by 0x11EA90: main (s2n_fork_generation_number_test.c:360)
==5763==

[martinetd]

Thanks for the issue! It looks like valgrind can spot these, looking to see if that's a quick update.

Nice, I didn't know valgrind also has a fd leak check! :)

Curious how you found this?

We had an app using s2n-tls that would re-exec itself periodically on errors, and everytime it re-exec'd an extra /dev/urandom fd showed up (I was explicitly checking for fd leaks as that is a common problem for apps re-execing themselves; memory etc is cleaned up by the kernel but fd need CLOEXEC or they'll be left behind)
Turns out we stopped doing re-exec for other reasons (actually not sure what-- not my team), but it's a low hanging fruit so might as well fix :)

[toidiu]

Thanks for reporting! Fixed in #3989

[martinetd]

Thanks for the follow-up & fix!