NetBSD Problem Report #59124

From www@netbsd.org  Mon Mar  3 21:02:23 2025
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 919D21A923A
	for <gnats-bugs@gnats.NetBSD.org>; Mon,  3 Mar 2025 21:02:23 +0000 (UTC)
Message-Id: <20250303210222.688CD1A923D@mollari.NetBSD.org>
Date: Mon,  3 Mar 2025 21:02:22 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: arc4random(3): first call in process races with concurrent fork
X-Send-Pr-Version: www-1.0

>Number:         59124
>Category:       lib
>Synopsis:       arc4random(3): first call in process races with concurrent fork
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    riastradh
>State:          pending-pullups
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Mar 03 21:05:00 +0000 2025
>Closed-Date:    
>Last-Modified:  Fri Oct 23 19:10:03 +0000 2026
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, ...
>Organization:
The NetBSD Fork4randamnation
>Environment:
>Description:
Suppose thread A calls arc4random for the first time, and thread B calls fork at the same time.

arc4random will (a) take the global arc4random lock and (b) call pthread_atfork to make itself fork-safe.

But if the fork happens between (a) and (b), the child will see the lock held with no threads running to release it.
>How-To-Repeat:
#include <sys/wait.h>

#include <err.h>
#include <pthread.h>
#include <stdlib.h>
#include <unistd.h>

static void *
forkit(void *cookie)
{
	pthread_barrier_t *bar = cookie;
	pid_t pid;
	int status;

	(void)pthread_barrier_wait(bar);
	if ((pid = fork()) == -1)
		err(1, "fork");
	if (pid == 0) {
		(void)alarm(1);
		(void)arc4random();
		_exit(0);
	}
	if (waitpid(pid, &status, 0) == -1)
		err(1, "waitpid");
	if (WIFSIGNALED(status)) {
		errx(1, "child exited on signal %d (%s)", WTERMSIG(status),
		    strsignal(WTERMSIG(status)));
	}
	if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
		errx(1, "child exited 0x%x", status);
	return NULL;
}

static void
test(void)
{
	pthread_barrier_t bar;
	pthread_t t;
	int error;

	error = pthread_barrier_init(&bar, NULL, 2);
	if (error)
		errc(1, error, "pthread_barrier_init");
	error = pthread_create(&t, NULL, &forkit, &bar);
	if (error)
		errc(1, error, "pthread_create");
	(void)pthread_barrier_wait(&bar);
	(void)arc4random();
	error = pthread_join(t, NULL);
	if (error)
		errc(1, error, "pthread_join");
}

int
main(void)
{

	for (unsigned long long n = 1;; n++) {
		pid_t pid;
		int status;

		if ((pid = fork()) == -1)
			err(1, "fork");
		if (pid == 0) {
			test();
			_exit(0);
		}
		if (waitpid(pid, &status, 0) == -1)
			err(1, "waitpid");
		if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
			errx(1, "test exited 0x%x after %zu trial%s", status,
			    n, n == 1 ? "" : "s");
		}
	}
	return 0;
}
>Fix:
Yes, please!

This is an instance of a more general problem.  One way we could address this is by creating a variant of pthread_atfork with caller-supplied storage, and using it in an ELF constructor routine which runs before any threads can be started.  Applications, of course, can avoid the problem if they make a single call to arc4random before fork.  But this has never been part of the contract of arc4random which is generally to be usable in any contexts without having to think about anything.

>Release-Note:

>Audit-Trail:
From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/59124: arc4random(3): first call in process races with concurrent fork
Date: Tue, 04 Mar 2025 10:29:36 +0700

     Date:        Mon,  3 Mar 2025 21:05:00 +0000 (UTC)
     From:        campbell+netbsd@mumble.net
     Message-ID:  <20250303210500.63C301A923F@mollari.NetBSD.org>

   | arc4random will (a) take the global arc4random lock and (b) call pthre=
 ad_atfork to make itself fork-safe.
   |
   | But if the fork happens between (a) and (b), the child will see the lo=
 ck held with no threads running to release it.


   | One way we could address this is by creating a variant of pthread_atfo=
 rk
   | with caller-supplied storage, and using it in an ELF constructor routi=
 ne

 Or, arc4random() could just call pthread_atfork() before it takes its own
 private lock.   That has to be simpler.   Just reverse (a) and (b).

 All that's needed is for the callback functions it supplies to able to tel=
 l
 if the lock is held or not, before attempting to release it - which is nee=
 ded
 anyway, as a more likely case is some thread forking, sometime after a cal=
 l
 to arc4random, when no call to that is active, and no lock is held.

 kre

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59124 CVS commit: src/lib/libc
Date: Thu, 6 Mar 2025 00:53:27 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Thu Mar  6 00:53:27 UTC 2025

 Modified Files:
 	src/lib/libc/gen: arc4random.c
 	src/lib/libc/include: arc4random.h

 Log Message:
 arc4random(3): Switch to use thr_once (libc pthread_once symbol).

 This way, we reduce the problem of arc4random initialization
 fork-safety to the problem of pthread_once fork-safety -- and
 anything else to do with one-time lazy initialization.

 PR lib/59124: arc4random(3): first call in process races with
 concurrent fork


 To generate a diff of this commit:
 cvs rdiff -u -r1.43 -r1.44 src/lib/libc/gen/arc4random.c
 cvs rdiff -u -r1.2 -r1.3 src/lib/libc/include/arc4random.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Responsible-Changed-From-To: lib-bug-people->riastradh
Responsible-Changed-By: riastradh@NetBSD.org
Responsible-Changed-When: Thu, 13 Mar 2025 10:22:30 +0000
Responsible-Changed-Why:
reduced to a separately solvable problem in HEAD
needs pullup-10, pullup-9


State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Thu, 13 Mar 2025 10:22:30 +0000
State-Changed-Why:
mine


State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Mon, 19 Oct 2026 21:19:06 +0000
State-Changed-Why:
fixed in HEAD before netbsd-11 branch    
pullup-10 #1381


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59124 CVS commit: [netbsd-10] src
Date: Fri, 23 Oct 2026 19:05:12 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Fri Oct 23 19:05:12 UTC 2026

 Modified Files:
 	src/lib/libc/gen [netbsd-10]: arc4random.c
 	src/lib/libc/include [netbsd-10]: arc4random.h
 	src/sys/kern [netbsd-10]: kern_entropy.c
 	src/sys/sys [netbsd-10]: sysctl.h
 	src/tests/lib/libc/gen [netbsd-10]: t_arc4random.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #1381):

 	lib/libc/gen/arc4random.c: revision 1.39
 	sys/kern/kern_entropy.c: revision 1.73
 	lib/libc/gen/arc4random.c: revision 1.50
 	lib/libc/include/arc4random.h: revision 1.2
 	lib/libc/include/arc4random.h: revision 1.3
 	lib/libc/include/arc4random.h: revision 1.4
 	lib/libc/gen/arc4random.c: revision 1.42
 	lib/libc/gen/arc4random.c: revision 1.44
 	lib/libc/gen/arc4random.c: revision 1.45
 	lib/libc/gen/arc4random.c: revision 1.46
 	lib/libc/gen/arc4random.c: revision 1.47
 	lib/libc/gen/arc4random.c: revision 1.48
 	lib/libc/gen/arc4random.c: revision 1.49
 	tests/lib/libc/gen/t_arc4random.c: revision 1.2
 	tests/lib/libc/gen/t_arc4random.c: revision 1.3
 	tests/lib/libc/gen/t_arc4random.c: revision 1.4
 	tests/lib/libc/gen/t_arc4random.c: revision 1.5
 	sys/sys/sysctl.h: revision 1.240
 	tests/lib/libc/gen/t_arc4random.c: revision 1.6
 	(all via patch)

 t_arc4random: Fix test to guarantee global state initialization.

 Previously, if pthread_atfork failed, arc4random_global_buf would
 fail to initialize the global state as intended.  (There are separate
 tests for intentional failure of global state initialization.)  It is
 unlikely for pthread_atfork to fail, but with RLIMIT_AS cranked down
 to zero, it may.  To ensure that the global state is successfully
 initialized for the tests that are trying to exercise it, call
 arc4random from another thread first before forcing failure of the
 local state initialization.

 PR lib/59117: arc4random has some failure modes it shouldn't

 arc4random(3): Avoid failure due to thread key limits.

 If thr_keycreate (a.k.a. pthread_key_create) fails, fall back to
 using globally serialized state instead of per-thread state.  This is
 unlikely to happen but arc4random(3) should work even if it does.

 New test case forces exercising this path (at least, simulating the
 effect of key creation failure).

 PR lib/59117: arc4random has some failure modes it shouldn't

 arc4random(3): Don't ifdef out the self-test.

 The runtime cost of this test is negligible (64 bytes of data and a
 handful of instructions for the subroutine) and the security value of
 verifying a known input/output for the deterministic cryptographic
 function on whose security the whole API relies is high.
 t_arc4random: Verify arc4random works without fds.

 It must work in an empty chroot where /dev/urandom doesn't exist, and

 it must work when the file descriptor resource limit is exceeded.

 Prompted by discussion around:
 PR lib/59117: arc4random has some failure modes it shouldn't
 Fortunately these are not failure modes of the current arc4random
 implementation!  But it is important to test them nevertheless, to
 forestall any temptation to invent new failure modes.

 arc4random(3): Switch to use thr_once (libc pthread_once symbol).

 This way, we reduce the problem of arc4random initialization
 fork-safety to the problem of pthread_once fork-safety -- and
 anything else to do with one-time lazy initialization.

 PR lib/59124: arc4random(3): first call in process races with
 concurrent fork

 t_arc4random: Test arc4random_global.per_thread, not .initialized.

 If arc4random_initialize has been called, and thr_keycreate failed,
 then .initialized will be true but .per_thread will be false -- and
 .thread_key will be garbage (some other thread key for another
 purpose, most likely).  This path was enabled by allowing
 thr_keycreate to fail instead of aborting the process.

 This hasn't caused trouble yet, mainly because we don't do anything
 to inject faults into thr_keycreate in these tests.  Tweak the
 global_threadkeylimit test while here to provoke a crash with the
 wrong conditional.

 Fix a similar edge case in the little test program embedded in
 arc4random.c (which should maybe just go away now that we have atf
 tests).

 PR lib/59117: arc4random has some failure modes it shouldn't

 arc4random(3): Provide a fallback in case pthread_atfork fails.

 This is considerably more work and burden on testing than simply
 statically preallocating a bit of storage for pthread_atfork to
 eliminate the failure mode altogether, but it is less work than
 arguing further over the atfork interface:
 https://mail-index.NetBSD.org/source-changes-d/2025/03/02/msg014387.html

 PR lib/59117: arc4random has some failure modes it shouldn't

 arc4random(3): Add self-tests for PRNG algorithm.

 Independently generated by running:

 head -c 64 </dev/zero | openssl enc -chacha -20   -K 000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f   -iv -iv 00000000000000000000000000000000 | hexdump -C

 and then repeating with the first 32 bytes of output as the updated
 key for -K, extracting bytes [32, 32 + n) for the n-byte output in
 each call.

 arc4random(4): Fix crypto_onetimestream_selftest on big-endian.

 Had computed the nonce increment the wrong way around when simulating
 a big-endian machine to generate this.

 arc4random(3): Isolate endian-dependence of crypto_onetimestream.

 Make it all happen in crypto_le32enc/dec (which in this file is
 actually just memcpy).  This way you can swap out crypto_le32enc/dec
 to test either byte order on any machine, e.g. for generating the
 test vectors.  And if we ever decide that it's not worthwhile to take
 advantage of the purely nondeterministic nature of the API to make
 this have potentially less performance penalty on big-endian systems
 (which I'm not sure has ever been measured anyway), it'll all
 continue to work under

         #define crypto_le32enc  le32enc
         #define crypto_le32dec  le32dec

 with all the _BYTE_ORDER == _BIG_ENDIAN conditionals removed from the
 self-tests.

 No change to the code of crypto_onetimstream on hppa under tools gcc.

 Assign static MIB numbers for kern.entropy.epoch.

 This sidesteps any need for dynamically sized memory in userland to
 resolve sysctl names to read it out, or for a new syscall interface
 to sysctl resolution by name.

 It would really be better to expose this through a page shared with
 userland, so querying it doesn't cost a syscall, but this will serve
 for now.

 PR lib/59148: arc4random calls malloc so it can't be used in an ELF
 constructor


 To generate a diff of this commit:
 cvs rdiff -u -r1.33.2.2 -r1.33.2.3 src/lib/libc/gen/arc4random.c
 cvs rdiff -u -r1.1.2.2 -r1.1.2.3 src/lib/libc/include/arc4random.h
 cvs rdiff -u -r1.57.4.6 -r1.57.4.7 src/sys/kern/kern_entropy.c
 cvs rdiff -u -r1.236 -r1.236.4.1 src/sys/sys/sysctl.h
 cvs rdiff -u -r1.1.2.2 -r1.1.2.3 src/tests/lib/libc/gen/t_arc4random.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

>Unformatted:

Home	PR Database Search