Re: [PATCH, RFC] random: introduce getrandom(2) system call

Re: [PATCH, RFC] random: introduce getrandom(2) system call

[Theodore Ts'o]

On Thu, Jul 17, 2014 at 11:50:58AM -0600, Bob Beck wrote:
> >
> > int getentropy(void *buf, size_t buflen)
> > {
> >         int     ret;
> >
> >         ret = getentropy(buf, buflen, 0);
> >         return (ret > 0) ? 0 : ret;
> > }
> >
> > The reason for the additional flags is that I'm trying to solve more
> > problems than just getentropy()'s raison d'etre.  The discussion of
> > this is in the commit description; let me know if there bits that I
> > could make clearer.
> 
> Ted is that the right flags with the new call so it doesn't block?

Yes, it's currently the correct flags.  There are some people who are
arguing for GRND_NONBLOCK instead of GRND_BLOCK.  The original idea
was that default should always be non-blocking, and you want 0 to be
the default.

On Thu, Jul 17, 2014 at 12:48:12PM -0700, Zach Brown wrote:
> Do we want it to block by default and have the flag be _NONBLOCK?  Feels
> more.. familiar.

Yes, I'm starting to think so.  In the default case where we are using
the urandom pool, we *do* want the default to block until the pool is
initialized.  At least, the FreeBSD people have been whining forever
that Linux is horribly insecure because we don't block until we are
sure that we know that we have good entropy in the pool.

The git description explains why I haven't changed this to date; I
didn't want to break userspace.  In addition, I've been working
awfully hard to make sure that least on x86 systems, that we gather
enough system noise that despite the fact that we are being very
conservative in our entropy estimation/accounting), the pool gets
initialized very quickly.

My goal was that on x86 laptops and severs, that the pool would be
fully initialized before the openssh scripts has a chance to run.  We
have a printk that triggers if something tries to read from
/dev/urandom before it is fully initialized, and we seem to be doing
pretty well.  Systemd seems to want to read from /dev/urandom in very
early boot:

Jul 11 21:14:10 closure kernel: [    2.941902] random: systemd urandom read with 109 bits of entropy available

.... but if you use the old-fashioned System V init scripts, it seems
to work fairly well.  :-)

So in practice, the fact that we block at system init time shouldn't
be a hardship for LibreSSL in most cases --- and in the case where you
are running on an embedded system where there are barely any devices,
no cycle counter, and nothing that produces enough interrupts to
initialize the pool, what would you prefer that we do?  Return data
that might not be fully "seed grade entropy"?

If you are determined to get data from a not a fully initialized
entropy pool, then you can open /dev/urandom and get it via the old
interface.  In actual practice, this shouldn't happen except in very
early boot, when any sane system wouldn't be trying to create
long-term public keys anyway.  (The fact that most systems try to
create OpenSSH's host keys as the first thing after an out-of-the-Box
first boot situation is something I've always considered Crazy-Eddie
Bat-Shit Insane....)  And that early in the boot sequence, it's highly
unlikely that an attacker would be carrying out a fd exhaustion
attack, since the network won't have been started.  And if you are
determined to get data even from an potentially insecure pool, and you
can't open /dev/urandom due to a fd exhaustion attack, the right
answer is to SIGKILL yourself anyway....

						- Ted

Re: [PATCH, RFC] random: introduce getrandom(2) system call

[Theodore Ts'o]

On Thu, Jul 17, 2014 at 12:48:12PM -0700, Zach Brown wrote:
> 
> > +	return urandom_read(NULL, buf, count, NULL);
> 
> I wonder if we want to refactor the entry points a bit more instead of
> directly calling the device read functions.  get_random_bytes() and
> urandom_read() both have their own uninitialied use warning message and
> tracing.  Does the syscall want its own little extraction function as
> well?

I'm not sure what warning you are worried about?  urandom_read() never
uses file or ppos, so passing in NULL works just fine as near as I can
tell.

I could refactor the entropy point, but it probably wouldn't add any
extra bloat, since the compiler would hopefully compile it away, but
adding the extra static function would seem to make things less
readable at least in my opinion.

					- Ted

Re: [PATCH, RFC] random: introduce getrandom(2) system call

[Theodore Ts'o]

On Thu, Jul 17, 2014 at 01:27:06PM -0700, Andy Lutomirski wrote:
> > +	return urandom_read(NULL, buf, count, NULL);
> 
> This can return -ERESTARTSYS.  Does it need any logic to restart correctly?

Nope; because we only return -ERESTARTSYS when we haven't generated
any randomness yet.  The way /dev/urandom and /dev/random devices work
is that if we get interrupted, we return a short read.  We do *not*
resume generation of random bytes from where we got interrupted from
the signal handler.

This is consistent with the definition in the signal(7) man page:

       If a blocked call to one of the following interfaces is
       interrupted by a signal handler, then the call will be
       automatically restarted after the signal handler returns if the
       SA_RESTART flag was used; otherwise the call will fail with the
       error EINTR:

           * read(2), readv(2), write(2), writev(2), and ioctl(2)
             calls on "slow" devices.  A "slow" device is one where
             the I/O call may block for an indefinite time, for
             example, a terminal, pipe, or socket.  (A disk is not a
             slow device according to this definition.)  If an I/O
             call on a slow device has already transferred some data
             by the time it is interrupted by a signal handler, then
             the call will return a success status (normally, the
             number of bytes transferred).

And in answer to Zach's question along these lines, ERESTARTSYS gets
restarted or transformed into EINTR by the system call layer, so long
as you only set ERESTARTSYS when signal_pending(current) is true.  If
you accidentally set the return value to ERESTARTSYS when a signal is
not pending, this error code can escape out to user space, which is
considered a bug.  But we're using this correctly in
drivers/char/random.c.

						- Ted

Re: [PATCH, RFC] random: introduce getrandom(2) system call

[Zach Brown]

On Thu, Jul 17, 2014 at 04:54:17PM -0400, Theodore Ts'o wrote:
> On Thu, Jul 17, 2014 at 12:48:12PM -0700, Zach Brown wrote:
> > 
> > > +	return urandom_read(NULL, buf, count, NULL);
> > 
> > I wonder if we want to refactor the entry points a bit more instead of
> > directly calling the device read functions.
> 
> I could refactor the entropy point, but it probably wouldn't add any
> extra bloat, since the compiler would hopefully compile it away, but
> adding the extra static function would seem to make things less
> readable at least in my opinion.

Fair enough, I don't have a strong preference either way.  It was just
something I noticed when leafing through the (unfamiliar) code.

- z

Re: [PATCH, RFC] random: introduce getrandom(2) system call

[Zach Brown]

On Thu, Jul 17, 2014 at 04:43:40PM -0400, Theodore Ts'o wrote:

> So in practice, the fact that we block at system init time shouldn't
> be a hardship for LibreSSL in most cases --- and in the case where you
> are running on an embedded system where there are barely any devices,
> no cycle counter, and nothing that produces enough interrupts to
> initialize the pool, what would you prefer that we do?  Return data
> that might not be fully "seed grade entropy"?
> 
> If you are determined to get data from a not a fully initialized
> entropy pool, then you can open /dev/urandom and get it via the old
> interface.

That sounds reasonable.  Maybe a slightly edited version of this writeup
could be dropped in the man page to give people context?

> (The fact that most systems try to create OpenSSH's host keys as the
> first thing after an out-of-the-Box first boot situation is something
> I've always considered Crazy-Eddie Bat-Shit Insane....)

(Seriously.  My atrophied sysadmin muscles still still cringe at that.)

- z

Re: [PATCH, RFC] random: introduce getrandom(2) system call

[Andy Lutomirski]

On Thu, Jul 17, 2014 at 2:44 PM, Zach Brown <zab-ugsP4Wv/S6ZeoWH0uzbU5w@public.gmane.org> wrote:
> On Thu, Jul 17, 2014 at 04:43:40PM -0400, Theodore Ts'o wrote:
>
>> So in practice, the fact that we block at system init time shouldn't
>> be a hardship for LibreSSL in most cases --- and in the case where you
>> are running on an embedded system where there are barely any devices,
>> no cycle counter, and nothing that produces enough interrupts to
>> initialize the pool, what would you prefer that we do?  Return data
>> that might not be fully "seed grade entropy"?
>>
>> If you are determined to get data from a not a fully initialized
>> entropy pool, then you can open /dev/urandom and get it via the old
>> interface.
>
> That sounds reasonable.  Maybe a slightly edited version of this writeup
> could be dropped in the man page to give people context?

And we'll be in a sad state in which we have a getrandom(2) syscall
but there's no decent way to use srand without either opening
/dev/urandom or mucking with AT_RANDOM.  And the latter barely works
because I think that most (all?) glibc versions clear it after using
it to initialize their stack canaries.

This isn't a regression, and it isn't a reason to object to
getrandom(2), but if getrandom(2) goes in as is, I'll submit a patch
adding the new flag immediately.

--Andy

Re: [PATCH, RFC] random: introduce getrandom(2) system call

[Theodore Ts'o]

On Thu, Jul 17, 2014 at 03:00:45PM -0700, Andy Lutomirski wrote:
> 
> And we'll be in a sad state in which we have a getrandom(2) syscall
> but there's no decent way to use srand without either opening
> /dev/urandom or mucking with AT_RANDOM.  And the latter barely works
> because I think that most (all?) glibc versions clear it after using
> it to initialize their stack canaries.

Sorry, I really don't think that supporting srand() is a good use of
getrandom(2).  If it's for non-crypto purposes, using getpid() and
time() is *just* *fine*.

If we add such a flag, my big fear is that it gets misused.  Sometimes
it does make sense to create interfaces that a strong point of view.
The primary use of getrandom(2) should be for cryptographic purposes,
and trying to avoid misuse should be the primary objective.

						- Ted