NetBSD Problem Report #56905

From gson@gson.org  Wed Jun 29 15:58:42 2022
Return-Path: <gson@gson.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))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id AE7831A921F
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 29 Jun 2022 15:58:42 +0000 (UTC)
Message-Id: <20220629155833.3D38C2547A7@guava.gson.org>
Date: Wed, 29 Jun 2022 18:58:33 +0300 (EEST)
From: gson@gson.org (Andreas Gustafsson)
Reply-To: gson@gson.org (Andreas Gustafsson)
To: gnats-bugs@NetBSD.org
Subject: getentropy() may return predictable data
X-Send-Pr-Version: 3.95

>Number:         56905
>Category:       lib
>Synopsis:       getentropy() may return predictable data
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Jun 29 16:00:00 +0000 2022
>Last-Modified:  Fri Jun 30 21:45:07 +0000 2023
>Originator:     Andreas Gustafsson
>Release:        NetBSD-current, source date >= 2022.05.31.13.42.59
>Organization:

>Environment:
System: NetBSD
Architecture: any
Machine: any
>Description:

On 2022-05-31, riastradh@ committed a change to re-enable the
getentropy(3) library call which nia@ had added on 2020-05-06 and
then disabled on 2020-09-23.

When the system lacks entropy, this implementation of getentropy()
will succeed and therefore return data known by the system to be
predictable, rather than failing or blocking until entropy becomes
available.  I consider this to be a security vulnerability, similar to
the one in ssh-keygen I reported in PR 55659 but this time affecting
any third-party application that use getentropy() for cryptographic
purposes.

I understand that not everyone agrees with this assessment, and as
the commit message says, "Discussion of details of the semantics, as
interpreted by NetBSD, is ongoing".  Nonetheless, when I see what
I believe is a security vulnerability, I feel it is my duty to report
it, and if possible, fix it.  And because I believe the discussion
about this issue should be public, I'm filing this in category lib
rather than category security, as PRs in category security and their
follow-up messages don't get publicly posted to netbsd-bugs even if
you explicitly set "Confidential: no".

>How-To-Repeat:

Boot a -current system with no entropy source.  Call getentropy().
Find that it succeeds.

>Fix:

The following patch fixes the issue by making getentropy() block
until entropy is available.  It passes all the tests in
/usr/src/tests/lib/libc/gen/t_getentropy.c.

Index: getentropy.c
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/getentropy.c,v
retrieving revision 1.3
diff -u -r1.3 getentropy.c
--- getentropy.c	31 May 2022 13:42:59 -0000	1.3
+++ getentropy.c	28 Jun 2022 08:33:27 -0000
@@ -35,8 +35,9 @@
 #include "namespace.h"

 #include <sys/param.h>
-#include <sys/sysctl.h>
+#include <sys/random.h>

+#include <assert.h>
 #include <errno.h>
 #include <limits.h>
 #include <unistd.h>
@@ -50,8 +51,7 @@
 int
 getentropy(void *buf, size_t buflen)
 {
-	size_t len = buflen;
-	int name[2] = { CTL_KERN, KERN_ARND };
+	ssize_t r;

 	if (buf == NULL && buflen > 0) {
 		errno = EFAULT;
@@ -63,5 +63,11 @@
 		return -1;
 	}

-	return sysctl(name, 2, buf, &len, NULL, 0);
+	do {
+		r = getrandom(buf, buflen, 0);
+	} while (r == -1 && errno == EINTR);
+
+	assert(r == -1 || r == (ssize_t)buflen);
+
+	return (int)r;
 }

>Audit-Trail:
From: Christos Zoulas <christos@zoulas.com>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
Subject: Re: lib/56905: getentropy() may return predictable data
Date: Wed, 29 Jun 2022 12:13:47 -0400

 But it is also supposed to return 0 on success, not the number of bytes.

 christos

From: Robert Elz <kre@munnari.OZ.AU>
To: Christos Zoulas <christos@zoulas.com>
Cc: gnats-bugs@netbsd.org, lib-bug-people@netbsd.org, gnats-admin@netbsd.org,
        netbsd-bugs@netbsd.org
Subject: Re: lib/56905: getentropy() may return predictable data
Date: Wed, 29 Jun 2022 23:59:21 +0700

 It is also deliberately intended to not block, that was the whole
 point I think.

 As I understand it, the idea is that if you care about security,
 you make sure sufficient entropy is available when the system boots,
 and there isn't an issue with it the way it is now.

 If you don't care (that much) about security then you might not do
 that, but you also are very unlikely to care that the initial entropy
 might not be as unpredictable as security conscious people might demand.

 But what no-one wants, is processes hanging forever because they're waiting
 on entropy arriving, from somewhere unknown, which no-one cares (or perhaps
 knows) enough about to create.   That's what all the complaints have been
 about.   Note "forever" in the hanging - when there's no initial entropy,
 and nothing providing any, the loop you're proposing adding never terminates.

 The plan, which probably has not yet been implemented (as it isn't an ABI
 altering change, and so isn't urgent before the branch) is to have rc.d
 scripts which check (if requested) if entropy is there or not, and if not,
 delay/abort the system startup until some is provided.   If you care about
 security, you'll have that turned on, just in case the entropy source isn't
 available, and needs fixing.   If you don't, you won't, and won't care.

 kre

From: nia <nia@NetBSD.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/56905: getentropy() may return predictable data
Date: Wed, 29 Jun 2022 17:23:55 +0000

 On Wed, Jun 29, 2022 at 05:00:03PM +0000, Robert Elz wrote:
 >  But what no-one wants, is processes hanging forever because they're waiting
 >  on entropy arriving, from somewhere unknown, which no-one cares (or perhaps
 >  knows) enough about to create.   That's what all the complaints have been
 >  about.   Note "forever" in the hanging - when there's no initial entropy,
 >  and nothing providing any, the loop you're proposing adding never terminates.

 Forever hanging also very much goes against the specification
 accepted by the Austin Group, if you want to indicate failure
 it should return -1 and set errno instead.

 However that should also not be the default behavior for compatibility
 raisins.

From: Andreas Gustafsson <gson@gson.org>
To: Robert Elz <kre@munnari.OZ.AU>
Cc: Christos Zoulas <christos@zoulas.com>,
    gnats-bugs@netbsd.org
Subject: Re: lib/56905: getentropy() may return predictable data
Date: Thu, 30 Jun 2022 15:24:41 +0300

 Robert Elz wrote:
 > It is also deliberately intended to not block, that was the whole
 > point I think.

 The getentropy() call originated in OpenBSD, where the question of
 blocking doesn't apply because OpenBSD claims to always have
 sufficient entropy available.  On systems that do not make that claim,
 such as NetBSD or Linux, it must have the option of blocking (or
 failing) to guarantee unpredictability, and on Linux, it can block.

 The text in https://www.opengroup.org/austin/docs/austin_1110.pdf does
 not require non-blocking, but it does require unpredictability.

 > As I understand it, the idea is that if you care about security,
 > you make sure sufficient entropy is available when the system boots,
 > and there isn't an issue with it the way it is now.

 This idea of the user having to "care" and "make sure" does not seem
 compatible with NetBSD's long-standing tradition of being secure by
 default.

 > If you don't care (that much) about security then you might not do
 > that, but you also are very unlikely to care that the initial entropy
 > might not be as unpredictable as security conscious people might demand.
 > 
 > But what no-one wants, is processes hanging forever because they're waiting
 > on entropy arriving, from somewhere unknown, which no-one cares (or perhaps
 > knows) enough about to create.   That's what all the complaints have been
 > about.   Note "forever" in the hanging - when there's no initial entropy,
 > and nothing providing any, the loop you're proposing adding never terminates.

 Yes, this is a real problem, but it is not specific to getentropy(),
 and it should really have its own PR.  We need to address the root
 cause of entropy not being available, or being available but not being
 counted as such, not sweep the problem under the rug one API at a
 time.

 One way to fix it would be to go back to recognizing interrupt timing
 entropy sources as contributing a nonzero amount of entropy, as most
 operating systems do, including NetBSD 9.  While I sympathize in
 principle with the notion that we should only count entropy from
 "certifiable" sources, if that results in us having to disable
 blocking, we have ended up worse off than we were in NetBSD 9, not
 better.  Blocking until we have met at least _some_ criterion for
 sufficient entropy, even one lacking theoretical rigor, is always
 better than not blocking at all.

 > The plan, which probably has not yet been implemented (as it isn't an ABI
 > altering change, and so isn't urgent before the branch) is to have rc.d
 > scripts which check (if requested) if entropy is there or not, and if not,
 > delay/abort the system startup until some is provided.   If you care about
 > security, you'll have that turned on, just in case the entropy source isn't
 > available, and needs fixing.   If you don't, you won't, and won't care.

 The "if requested" part does not sound like it's secure by default.
 And until or unless the plan is implemented, -current remains vulnerable.
 -- 
 Andreas Gustafsson, gson@gson.org

From: Andreas Gustafsson <gson@gson.org>
To: Christos Zoulas <christos@zoulas.com>
Cc: gnats-bugs@netbsd.org
Subject: Re: lib/56905: getentropy() may return predictable data
Date: Thu, 30 Jun 2022 15:31:15 +0300

 Christos Zoulas wrote:
 > But it is also supposed to return 0 on success, not the number of bytes.

 Good catch, thanks.  Revised patch:

 Index: getentropy.c
 ===================================================================
 RCS file: /cvsroot/src/lib/libc/gen/getentropy.c,v
 retrieving revision 1.3
 diff -u -r1.3 getentropy.c
 --- getentropy.c	31 May 2022 13:42:59 -0000	1.3
 +++ getentropy.c	30 Jun 2022 12:32:06 -0000
 @@ -35,8 +35,9 @@
  #include "namespace.h"

  #include <sys/param.h>
 -#include <sys/sysctl.h>
 +#include <sys/random.h>

 +#include <assert.h>
  #include <errno.h>
  #include <limits.h>
  #include <unistd.h>
 @@ -50,8 +51,7 @@
  int
  getentropy(void *buf, size_t buflen)
  {
 -	size_t len = buflen;
 -	int name[2] = { CTL_KERN, KERN_ARND };
 +	ssize_t r;

  	if (buf == NULL && buflen > 0) {
  		errno = EFAULT;
 @@ -63,5 +63,14 @@
  		return -1;
  	}

 -	return sysctl(name, 2, buf, &len, NULL, 0);
 +	do {
 +		r = getrandom(buf, buflen, 0);
 +	} while (r == -1 && errno == EINTR);
 +
 +	if (r == -1)
 +		return r;
 +
 +	assert(r == (ssize_t)buflen);
 +
 +	return 0;
  }

From: Andreas Gustafsson <gson@gson.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/56905: getentropy() may return predictable data
Date: Fri, 22 Jul 2022 14:22:12 +0300

 The situation is even more complicated that I thought.

 I thought adding getentropy() as a new libc function would only affect
 third-party software because nothing in the base system would be using
 a function that did not yet exist, but it turns out I was wrong.  The
 OpenSSL library in base actually detects the presence of getentropy()
 at run time, and now ends up calling getentropy() rather than
 sysctl(KERN_ARND).

 Therefore, changing getentropy() to have the proper blocking semantics
 would have the side effect of causing many programs in the NetBSD base
 system that rely on OpenSSL, such as ssh-keygen(1), syslogd(8), and
 login(1), to also block until entropy is available.  That would be a
 good thing from a security perspective, and might have worked in -9
 where processes blocking for entropy eventually get unblocked, but
 regrettably, in -current they no longer do on some systems.  Since
 login(1) blocking forever would render the system unusable and not
 easily recoverable, making OpenSSL block is not a realistic option
 until the root cause of the blocking problems in -current, the
 persistent lack of entropy (or lack of belief in the entropy that is
 present), is fixed.

 If the intent when adding getentropy() to libc really was just to
 introduce the symbol into libc for reasons of ABI compatibility, and
 not to switch OpenSSL over to it, perhaps OpenSSL could be patched to
 ignore it, as in the patch below.  But this would just be a stopgap
 measure that effectively amounts to perpetuating a long-standing
 vulnerability in OpenSSL so that a new one in getentropy() can be
 fixed.

 Index: src/crypto/external/bsd/openssl/dist/crypto/rand/rand_unix.c
 ===================================================================
 RCS file: /cvsroot/src/crypto/external/bsd/openssl/dist/crypto/rand/rand_unix.c,v
 retrieving revision 1.18
 diff -u -r1.18 rand_unix.c
 --- src/crypto/external/bsd/openssl/dist/crypto/rand/rand_unix.c	7 Jan 2022 15:50:11 -0000	1.18
 +++ src/crypto/external/bsd/openssl/dist/crypto/rand/rand_unix.c	21 Jul 2022 12:16:43 -0000
 @@ -370,7 +370,9 @@
       * Note: Sometimes getentropy() can be provided but not implemented
       * internally. So we need to check errno for ENOSYS
       */
 -#  if defined(__GNUC__) && __GNUC__>=2 && defined(__ELF__) && !defined(__hpux)
 +#  if defined(__NetBSD__)
 +    /* Avoid getentropy() while its semantics are still being debated */
 +#  elif defined(__GNUC__) && __GNUC__>=2 && defined(__ELF__) && !defined(__hpux)
      extern int getentropy(void *buffer, size_t length) __attribute__((weak));

      if (getentropy != NULL) {

 -- 
 Andreas Gustafsson, gson@gson.org

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/56905: getentropy() may return predictable data
Date: Fri, 22 Jul 2022 21:37:43 +0700

 getentropy() is being added to POSIX in the next version of the standard.

 The description doesn't explicitly say it is not allowed to block,
 but it does say (explicitly) that it is not a thread cancellation
 point.

 In general, everything which can block is either a cancellation point,
 or is allowed to be (the latter for things like putc() which might call
 a function which is a cancellation point, or might not).

 Not being a cancellation point tells me that getentropy() is not
 expected to block (ever).

 It is allowed to fail - but the description of its one possible relevant
 failure mode (it is also possible for it to return an error if the user
 passes invalid params - asks for too many bits, for example - that's not
 relevant here) is:

     [ENOSYS]  The system does not provide the necessary source of entropy.

 which seems to me to be more a permanent failure, that is, it isn't
 saying "no entropy is currently available", than one which can
 ever be rectified (eg: by pausing and trying later - similating blocking
 in the caller).

 Blocking in getentropy() won't happen, "the proper semantics" are not that.

 kre

 ps: there's no getrandom() call in POSIX however.


From: Andreas Gustafsson <gson@gson.org>
To: gnats-bugs@netbsd.org
Cc: Robert Elz <kre@munnari.OZ.AU>
Subject: Re: lib/56905: getentropy() may return predictable data
Date: Fri, 22 Jul 2022 21:03:36 +0300

 Robert Elz wrote:
 >  getentropy() is being added to POSIX in the next version of the standard.
 >  
 >  The description doesn't explicitly say it is not allowed to block,
 >  but it does say (explicitly) that it is not a thread cancellation
 >  point.
 >  
 >  In general, everything which can block is either a cancellation point,
 >  or is allowed to be (the latter for things like putc() which might call
 >  a function which is a cancellation point, or might not).
 >  
 >  Not being a cancellation point tells me that getentropy() is not
 >  expected to block (ever).

 Yet in FreeBSD and Linux, it does block:

   https://www.freebsd.org/cgi/man.cgi?query=getentropy&sektion=3&format=html
   https://man7.org/linux/man-pages/man3/getentropy.3.html

 On operating systems where the kernel CSPRNG is not guaranteed to be
 seeded by the time a user process runs, such as FreeBSD, Linux, and
 NetBSD, getentropy() needs to block until it is seeded in order to
 meet the POSIX requirement of returning unpredictable data.  If POSIX
 has not anticipated this, that seems like a serious flaw in the
 standard.

 >  It is allowed to fail - but the description of its one possible relevant
 >  failure mode (it is also possible for it to return an error if the user
 >  passes invalid params - asks for too many bits, for example - that's not
 >  relevant here) is:
 >  
 >      [ENOSYS]  The system does not provide the necessary source of entropy.
 >  
 >  which seems to me to be more a permanent failure, that is, it isn't
 >  saying "no entropy is currently available", than one which can
 >  ever be rectified (eg: by pausing and trying later - similating blocking
 >  in the caller).

 I do agree that ENOSYS seems to be intended for more permanent
 failures.

 >  Blocking in getentropy() won't happen, "the proper semantics" are not that.

 And I won't be using an operating system where it doesn't block (or
 possibly fail) when the system knows it is lacking entropy, even if
 that means the end of my 27 years as a NetBSD user.  Since that is not
 a decision to be made lightly, I'm sure you understand that I'm not
 taking your word for this but asking core to make a formal decision
 one way the other.
 -- 
 Andreas Gustafsson, gson@gson.org

From: Andreas Gustafsson <gson@gson.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/56905: getentropy() may return predictable data
Date: Thu, 23 Mar 2023 14:39:32 +0200

 For the record, I recently received the following statement from core:

 "We consider the behavior in PR 56905 a bug and we are working towards a fix."
 -- 
 Andreas Gustafsson, gson@gson.org

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56905 CVS commit: src
Date: Fri, 30 Jun 2023 21:44:09 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Jun 30 21:44:09 UTC 2023

 Modified Files:
 	src/etc/defaults: rc.conf
 	src/share/man/man5: rc.conf.5

 Log Message:
 rc.conf(5): Set entropy=wait by default.

 We no longer block indefinitely -- if nothing else, the hardclock
 timer should yield enough samples to unblock /dev/random on all but
 the most severely deterministic machines -- so it should be generally
 safe for availability to set entropy=wait.

 This doesn't guarantee that HWRNG/seed has been provided before you
 run ssh-keygen or call getentropy(3) in a user application, but it
 does raise the security above netbsd<=9.

 PR security/55659
 PR lib/56905

 XXX pullup-10


 To generate a diff of this commit:
 cvs rdiff -u -r1.163 -r1.164 src/etc/defaults/rc.conf
 cvs rdiff -u -r1.192 -r1.193 src/share/man/man5/rc.conf.5

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

Home	PR Database Search