Opened 6 months ago

Closed 5 months ago

#25120 closed defect (implemented)

getrandom() syscall failure warning should be a notice and worded better

Reported by: catalyst Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: s8-errors
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor8-can

Description

The logging improvement for error handling of the getrandom() syscall in #24500 could be further improved:

  • The log level should possibly be NOTICE rather than WARN.
  • The log message should mention that tor will fall back to alternative sources of randomness.
  • Maybe we want to mention header/kernel version mismatches as a specific common reason for this issue.

Yes, some of the fallbacks are somewhat dangerous, like /dev/urandom might not be seeded yet. etc,.

Child Tickets

Change History (12)

comment:1 Changed 6 months ago by ahf

Status: newneeds_review

Wording and log-level patches in https://oniongit.eu/ahf/tor/merge_requests/5

These patches do not have the mechanism needed to check if /dev/urandom is safe.

comment:2 Changed 6 months ago by catalyst

Do we think it's dangerous to downgrade the warning without first ensuring that we can safely use /dev/urandom even early in boot?

comment:3 Changed 6 months ago by nickm

My opinion: I think that checking whether urandom is correctly seeded would be a good additional feature to have, but I don't think it needs to block this message downgrade. Anybody who sees the current warning is likelier to get confused than to reason correctly about urandom risks based on it -- and I think nearly everybody sees notice messages as well as warning. IMO.

comment:4 in reply to:  3 Changed 6 months ago by catalyst

Replying to nickm:

and I think nearly everybody sees notice messages as well as warning. IMO.

The patch drops it all the way to INFO though, which isn't readily visible to at least Tor Browser users.

comment:5 Changed 6 months ago by catalyst

Keywords: s8-errors added
Sponsor: Sponsor8-can

comment:6 Changed 6 months ago by nickm

So would you be okay with this patch if it were NOTICE instead?

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

Replying to nickm:

So would you be okay with this patch if it were NOTICE instead?

Yes. Or hearing some rationale for why INFO is more appropriate than NOTICE.

comment:8 Changed 6 months ago by dgoulet

My two cents.

NOTICE level should be information the operator needs or is useful to know. And I also think it is useful to provide basic status information at bootup so if we ever get a report about a misbehaving relay, we can ask for those "status" line.

With this getrandom() thing, if Tor stops because it can't use its crypto, we ought to put a warning on why even though the users would be "omgwtfbbq is that?". At least, at that point, there are possible action items that the operator can do including seeking support about that "in your face" log line.

If tor recovers from it, I would argue that it should be at NOTICE so the operator can see that it is not critical, that tor did recover but actions can still be taken to fix it.

For instance this, I think it should be at NOTICE for the above reasons. This usually happens when someones run a tor not built for their system like Stretch Debian tor package on Ubuntu 10.04. Having the notice log would allow the operator to try to fix it or simply ignore it. At INFO, I believe most of the users will just never notice it.

        log_info(LD_CRYPTO, "Can't get entropy from getrandom()."
                 " You are running a version of Tor built to support"
                 " getrandom(), but the kernel doesn't implement this"
                 " function--probably because it is too old?"
                 " Trying fallback method instead.");

In a nutshell, +1 on removing the warnings *except* if Tor does stop. And then +1 on NOTICE for useful logging for which the operator can notice.

comment:9 Changed 6 months ago by catalyst

Status: needs_reviewneeds_revision

I agree with dgoulet. I also agree with nickm on deferring checking of urandom seeding to another ticket.

comment:10 Changed 5 months ago by ahf

Status: needs_revisionneeds_review

I've added a fixup commit to the PR. Remember to autosquash when pulling :-)

comment:11 Changed 5 months ago by catalyst

Status: needs_reviewmerge_ready

Looks good to me, other than the Rust test failures that are presumably due to #25127. Hopefully those will resolve upon merge.

comment:12 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Squashed and merged; added a changes file; fixed compilation.

Note: See TracTickets for help on using tickets.