From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by sourceware.org (Postfix) with ESMTPS id 480433858CDA for ; Mon, 25 Jul 2022 23:59:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 480433858CDA Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 42237B8110D; Mon, 25 Jul 2022 23:59:21 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C2730C341C6; Mon, 25 Jul 2022 23:59:19 +0000 (UTC) Date: Mon, 25 Jul 2022 16:59:17 -0700 From: Eric Biggers To: "Jason A. Donenfeld" Cc: libc-alpha@sourceware.org, Adhemerval Zanella Netto , Florian Weimer , Cristian =?iso-8859-1?Q?Rodr=EDguez?= , Paul Eggert , linux-crypto@vger.kernel.org Subject: Re: [PATCH v2] arc4random: simplify design for better safety Message-ID: References: <20220725225728.824128-1-Jason@zx2c4.com> <20220725232810.843433-1-Jason@zx2c4.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220725232810.843433-1-Jason@zx2c4.com> X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Jul 2022 23:59:23 -0000 On Tue, Jul 26, 2022 at 01:28:10AM +0200, Jason A. Donenfeld wrote: > Rather than buffering 16 MiB of entropy in userspace (by way of > chacha20), simply call getrandom() every time. > > This approach is doubtlessly slower, for now, but trying to prematurely > optimize arc4random appears to be leading toward all sorts of nasty > properties and gotchas. Instead, this patch takes a much more > conservative approach. The interface is added as a basic loop wrapper > around getrandom(), and then later, the kernel and libc together can > work together on optimizing that. > > This prevents numerous issues in which userspace is unaware of when it > really must throw away its buffer, since we avoid buffering all > together. Future improvements may include userspace learning more from > the kernel about when to do that, which might make these sorts of > chacha20-based optimizations more possible. The current heuristic of 16 > MiB is meaningless garbage that doesn't correspond to anything the > kernel might know about. So for now, let's just do something > conservative that we know is correct and won't lead to cryptographic > issues for users of this function. > > This patch might be considered along the lines of, "optimization is the > root of all evil," in that the much more complex implementation it > replaces moves too fast without considering security implications, > whereas the incremental approach done here is a much safer way of going > about things. Once this lands, we can take our time in optimizing this > properly using new interplay between the kernel and userspace. > > getrandom(0) is used, since that's the one that ensures the bytes > returned are cryptographically secure. But on systems without it, we > fallback to using /dev/urandom. This is unfortunate because it means > opening a file descriptor, but there's not much of a choice. Secondly, > as part of the fallback, in order to get more or less the same > properties of getrandom(0), we poll on /dev/random, and if the poll > succeeds at least once, then we assume the RNG is initialized. This is a > rough approximation, as the ancient "non-blocking pool" initialized > after the "blocking pool", not before, but it's the best approximation > we can do. > > The motivation for including arc4random, in the first place, is to have > source-level compatibility with existing code. That means this patch > doesn't attempt to litigate the interface itself. It does, however, > choose a conservative approach for implementing it. > > Cc: Adhemerval Zanella Netto > Cc: Florian Weimer > Cc: Cristian Rodríguez > Cc: Paul Eggert > Cc: linux-crypto@vger.kernel.org > Signed-off-by: Jason A. Donenfeld This looks good to me. There are still a few bits that need to be removed/updated. With a quick grep, I found: sysdeps/generic/tls-internal-struct.h: struct arc4random_state_t *rand_state; sysdeps/unix/sysv/linux/tls-internal.h:/* Reset the arc4random TCB state on fork. * NEWS: ... The functions use a pseudo-random number generator along with NEWS: entropy from the kernel. Also, the documentation in manual/math.texi should say that the randomness is cryptographically secure. - Eric