Improve getrandom handling

https://cgit.alxu.ca/tor.git/commit/?h=better-getrandom

Improve getrandom handling.

Do not check for EINTR and EAGAIN because getrandom is documented to never return those if size < 256 and flags = 0.

Also revise obsolete and inaccurate comment.

changed milestone to %Tor: unspecified

added 034-deferred-20180602 035-removed-20180711 component::core tor/tor milestone::Tor: unspecified owner::Hello71 priority::medium resolution::worksforme reviewer::catalyst severity::normal status::closed type::enhancement labels

Trac:
Status: assigned to needs_review

Trac:
Milestone: N/A to Tor: 0.3.4.x-final

Trac:
Reviewer: N/A to catalyst

Thanks for the patch! The getrandom(2) manpage on one of my Ubuntu VMs says

       The behavior when a call to getrandom() that is blocked  while  reading
       from  /dev/urandom  is  interrupted  by a signal handler depends on the
       initialization state of the entropy buffer and  on  the  request  size,
       buflen.  If the entropy is not yet initialized, then the call will fail
       with the EINTR error.

so in the case of tor starting up soon after boot, I think it might be possible to get EINTR if tor receives a signal while blocked on insufficient entropy. Arguably we want to retry in this case. (Also I would be interested in hearing if there are good reasons to treat this as a bug when it occurs anyway.)

Please make a GitHub pull request for your revised patches so CI can run on them. Thanks!

Trac:
Status: needs_review to needs_revision

Hm... I see, you are correct. I really really hope that nowadays everybody has random seed persistence, but it is of course better to be conservative here.

You can see the Travis output at https://travis-ci.org/Hello71/tor/builds/376061939 (via https://travis-ci.org/Hello71/tor/branches).

Ah, I remember. I read the EINTR part of the ERRORS section, and I interpreted it to mean that we will never receive EINTR, contrary to your quote. I filed https://bugzilla.kernel.org/show_bug.cgi?id=199711 to ask which one it is.

Replying to Hello71:

Ah, I remember. I read the EINTR part of the ERRORS section, and I interpreted it to mean that we will never receive EINTR, contrary to your quote. I filed https://bugzilla.kernel.org/show_bug.cgi?id=199711 to ask which one it is. Thanks for filing the kernel bug report! It looks like libevent might always set SA_RESTART when installing signal handlers on systems with sigaction()? (at least based on my quick skim of the source)

Trac:
Status: needs_revision to needs_review

Looks good to me! I made a squashed and rebased patch in https://github.com/torproject/tor/pull/107 to double check coveralls results.

Trac:
Status: needs_review to merge_ready

Code seems plausible. Could somebody please write a changes file for this patch?

Trac:
Status: merge_ready to needs_revision

When thinking about how to describe the user-visible parts of this change, I realized that the previous code would loop on EINTR, while the patch causes a failure and disables getrandom() thereafter. This is unlikely to be a problem in practice, because libevent seems to always set SA_RESTART, which should prevent us from getting EINTR.

Maybe we should mention this in the changes file. On the other hand, maybe the conservative and likely harmless thing to do is to leave the existing loop as it is, even if it doesn't ever end up looping. If we restore the loop, I think the remaining parts of the patch are some comment improvements and handling of a (also unlikely) short-read condition.

Replying to Hello71:

Hm... I see, you are correct. I really really hope that nowadays everybody has random seed persistence, but it is of course better to be conservative here.

Unfortunately, the random seed takes quite some time (on the order of minutes) to actually take effect. The seed is written to the non-blocking character device which triggers the random_write file operation which uses write_pool to send the data to the input pool. Unfortunately it can take a while for the secondary pools to receive the seed, since they have to wait for the push_to_pool workqueue function to be triggered. On newer Linux kernels (using ChaCha20 rather than SHA-1 for the non-blocking character device), the input pool is queried every 5 minutes, and it only reseeds the stream cipher if more than 128 bits of entropy have been collected in the input pool since the last reseed.

If you do not check for EINTR (and avoid the blocking behavior altogether), then even if you are using a persistent random seed, you will end up obtaining potentially predictable random data.

Replying to catalyst:

When thinking about how to describe the user-visible parts of this change, I realized that the previous code would loop on EINTR, while the patch causes a failure and disables getrandom() thereafter. This is unlikely to be a problem in practice, because libevent seems to always set SA_RESTART, which should prevent us from getting EINTR.

It would be foolish to rely on a library's current behavior if it's not explicitly standardized.

Maybe we should mention this in the changes file. On the other hand, maybe the conservative and likely harmless thing to do is to leave the existing loop as it is, even if it doesn't ever end up looping. If we restore the loop, I think the remaining parts of the patch are some comment improvements and handling of a (also unlikely) short-read condition.

It would be completely harmless. You can even use LTO to allow the compiler to optimize the loop out.

Deferring non-must tickets to 0.3.5

Trac:
Keywords: N/A deleted, 034-deferred-20180602 added
Milestone: Tor: 0.3.4.x-final to Tor: 0.3.5.x-final

Removing needs_revision tickets from 0.3.5 that seem to be stalled. Please move back if they are under active revision or discussion.

Trac:
Keywords: N/A deleted, 035-removed-20180711 added
Milestone: Tor: 0.3.5.x-final to Tor: unspecified

I don't think there's enough left here to bother revising. Please close.

Trac:
Status: needs_revision to closed
Resolution: N/A to worksforme

closed

moved to tpo/core/tor#26040 (closed)