Opened 6 months ago

Closed 4 weeks ago

#26040 closed enhancement (worksforme)

Improve getrandom handling

Reported by: Hello71 Owned by: Hello71
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 034-deferred-20180602, 035-removed-20180711
Cc: Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Description

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.

Child Tickets

Change History (16)

comment:1 Changed 6 months ago by Hello71

Status: assignedneeds_review

comment:2 Changed 6 months ago by nickm

Milestone: Tor: 0.3.4.x-final

comment:3 Changed 6 months ago by dgoulet

Reviewer: catalyst

comment:4 Changed 6 months ago by catalyst

Status: needs_reviewneeds_revision

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!

comment:5 Changed 6 months ago by 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.

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

comment:6 Changed 6 months ago by 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.

comment:7 in reply to:  6 Changed 6 months ago by catalyst

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)

comment:8 Changed 6 months ago by catalyst

Status: needs_revisionneeds_review

comment:9 Changed 6 months ago by catalyst

Status: needs_reviewmerge_ready

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

comment:10 Changed 6 months ago by nickm

Status: merge_readyneeds_revision

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

comment:11 Changed 6 months ago by 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.

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.

comment:12 in reply to:  5 Changed 6 months ago by cypherpunks

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.

Last edited 6 months ago by cypherpunks (previous) (diff)

comment:13 Changed 6 months ago by nickm

Keywords: 034-deferred-20180602 added
Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

Deferring non-must tickets to 0.3.5

comment:14 Changed 4 months ago by nickm

Keywords: 035-removed-20180711 added
Milestone: Tor: 0.3.5.x-finalTor: unspecified

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

comment:15 Changed 5 weeks ago by Hello71

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

comment:16 Changed 4 weeks ago by Hello71

Resolution: worksforme
Status: needs_revisionclosed
Note: See TracTickets for help on using tickets.