NetBSD Problem Report #59117

From www@netbsd.org  Sun Mar  2 13:53: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)
	 client-signature RSA-PSS (2048 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 1C6941A923A
	for <gnats-bugs@gnats.NetBSD.org>; Sun,  2 Mar 2025 13:53:23 +0000 (UTC)
Message-Id: <20250302135321.E9BA51A923D@mollari.NetBSD.org>
Date: Sun,  2 Mar 2025 13:53:21 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: arc4random has some failure modes it shouldn't
X-Send-Pr-Version: www-1.0

>Number:         59117
>Category:       lib
>Synopsis:       arc4random has some failure modes it shouldn't
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    riastradh
>State:          needs-pullups
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Mar 02 13:55:00 +0000 2025
>Closed-Date:    
>Last-Modified:  Thu Mar 13 10:21:40 +0000 2025
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The Arc4BSD Forkation
>Environment:
>Description:
arc4random can fail (and abort, because it has no return value indicator) for the following reasons:

1. sysctl kern.arandom fails
2. crypto self-test fails
3. pthread_atfork fails
4. thr_keycreate fails

Cases (1) and (2) are reasonable: if we have no entropy, or if the software is buggy, we can't do anything.

Cases (3) and (4) are not really reasonable:

(a) We should have a variant of pthread_atfork where the caller provides storage so it can't fail.  This is also useful for libpthread's pthread_tsd_init which runs in a constructor before malloc is safe to call.
(b) If thr_keycreate fails we can always just fall back to using global state.
>How-To-Repeat:
make pthread_atfork or thr_keycreate fail and try to use arc4random
>Fix:
Yes, please!

>Release-Note:

>Audit-Trail:
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59117 CVS commit: src
Date: Sun, 2 Mar 2025 21:35:59 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sun Mar  2 21:35:59 UTC 2025

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

 Log Message:
 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


 To generate a diff of this commit:
 cvs rdiff -u -r1.38 -r1.39 src/lib/libc/gen/arc4random.c
 cvs rdiff -u -r1.1 -r1.2 src/lib/libc/include/arc4random.h
 cvs rdiff -u -r1.1 -r1.2 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.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59117 CVS commit: src/lib/libc/gen
Date: Sun, 2 Mar 2025 22:46:24 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sun Mar  2 22:46:24 UTC 2025

 Modified Files:
 	src/lib/libc/gen: arc4random.c pthread_atfork.c

 Log Message:
 libc: New __libc_atfork.

 This uses caller-provided storage for the callback queues.

 Use it in arc4random(3) in order to avoid possible failure modes.

 This is a private symbol, not designed for use outside NetBSD, and
 the API is not intended to be stable (yet) -- I just took the
 existing purely internal structure (struct atfork_callback) and
 reused it for this API without changing any of the calling-side
 logic.  We could change it, e.g. to use a single structure per call,
 to make the API a little less unwieldy, at the cost of
 microscopically more storage and runtime for the users that don't use
 all three callbacks; to be considered in a future change.

 We might reasonably use __libc_atfork in libpthread for use in the
 pthread_tsd_init constructor, in order to be confident it never
 attempts malloc(3), but let's do that in a separate commit just in
 case anything goes awry with that plan.

 PR lib/59112: libpthread constructors use malloc
 PR lib/59117: arc4random has some failure modes it shouldn't


 To generate a diff of this commit:
 cvs rdiff -u -r1.39 -r1.40 src/lib/libc/gen/arc4random.c
 cvs rdiff -u -r1.23 -r1.24 src/lib/libc/gen/pthread_atfork.c

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

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59117 CVS commit: src/lib/libc/gen
Date: Sun, 2 Mar 2025 22:53:45 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sun Mar  2 22:53:45 UTC 2025

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

 Log Message:
 arc4random(3): Update comments to reflect removal of failure modes.

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


 To generate a diff of this commit:
 cvs rdiff -u -r1.40 -r1.41 src/lib/libc/gen/arc4random.c

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

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59117 CVS commit: src/lib/libc/include
Date: Mon, 3 Mar 2025 00:57:22 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Mon Mar  3 00:57:21 UTC 2025

 Added Files:
 	src/lib/libc/include: atfork.h

 Log Message:
 __libc_atfork: Add header file missed in previous commit.

 PR lib/59112: libpthread constructors use malloc
 PR lib/59117: arc4random has some failure modes it shouldn't


 To generate a diff of this commit:
 cvs rdiff -u -r0 -r1.1 src/lib/libc/include/atfork.h

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

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59117 CVS commit: src/tests/lib/libc/gen
Date: Wed, 5 Mar 2025 21:30:34 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Wed Mar  5 21:30:34 UTC 2025

 Modified Files:
 	src/tests/lib/libc/gen: t_arc4random.c

 Log Message:
 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.


 To generate a diff of this commit:
 cvs rdiff -u -r1.2 -r1.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.

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

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

 Modified Files:
 	src/lib/libc/gen: arc4random.c
 	src/tests/lib/libc/gen: t_arc4random.c

 Log Message:
 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


 To generate a diff of this commit:
 cvs rdiff -u -r1.44 -r1.45 src/lib/libc/gen/arc4random.c
 cvs rdiff -u -r1.3 -r1.4 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.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59117 CVS commit: src
Date: Sun, 9 Mar 2025 18:11:55 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sun Mar  9 18:11:55 UTC 2025

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

 Log Message:
 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


 To generate a diff of this commit:
 cvs rdiff -u -r1.45 -r1.46 src/lib/libc/gen/arc4random.c
 cvs rdiff -u -r1.3 -r1.4 src/lib/libc/include/arc4random.h
 cvs rdiff -u -r1.4 -r1.5 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.

Responsible-Changed-From-To: lib-bug-people->riastradh
Responsible-Changed-By: riastradh@NetBSD.org
Responsible-Changed-When: Thu, 13 Mar 2025 10:21:40 +0000
Responsible-Changed-Why:
fixed in HEAD, needs pullup-10, maybe pullup-9


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


>Unformatted:

Home	PR Database Search