Opened 2 years ago

Closed 2 years ago

#17686 closed defect (fixed)

Make our openssl-RNG calling code less scary.

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rng, crypto
Cc: teor, yawning Actual Points:
Parent ID: #17694 Points:
Reviewer: Sponsor:

Description

crypto_rand() returns even when RAND_bytes fails. That's bad!

It's not actually a security bug though, since RAND_bytes can't fail given how we set it up. But we should change the warning in RAND_bytes() to an assertion. Then we can make crypto_rand() return void.

Similarly, in crypto_seed_rng(), we should check RAND_status() or something, and flip out or assert if the openssl RNG is not fully initialized and seeded.

Child Tickets

Attachments (1)

0001-Change-type-signature-of-the-mock-function.patch (1.6 KB) - added by cypherpunks 2 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 2 years ago by yawning

Signing off on the "not actually a security bug" bit, at least with all 4 of the latest OpenSSL release branches and master.

A few more thoughts, while we're messing with this code (may deserve separate tickets?):

  • crypto_strongest_rand() could/should use getrandom/getentropy when available.
  • If we trust in libottery(-lite), we could fold it in and have OpenSSL use that as the RAND method.

comment:2 Changed 2 years ago by nickm

Status: newneeds_review

First cut in bug17686_027. Needs a changes file.

crypto_strongest_rand() could/should use getrandom/getentropy when available.

That's already a ticket as #13696.

comment:3 Changed 2 years ago by yawning

nitpick: Comments for crypto_rand and crypto_rand_unmocked should be updated to reflect no return value.

Apart from that and the changes file, lgtm.

comment:4 Changed 2 years ago by nickm

Okay, tried to clean up the remaining issues. Will wait for another review. (Teor, I'd dig your opinion on this too.)

comment:5 Changed 2 years ago by teor

LGTM - I think it looks good.

comment:6 Changed 2 years ago by teor

Parent ID: #17694

This ticket needs to merge before #17694: #17694 depends on these changes.

comment:7 Changed 2 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.7.x-final

Squashed as bug17686_v2_027 and merged to master. We should consider this for backport to 0.2.7 as well.

comment:8 Changed 2 years ago by asn

Changes look good to me!

comment:9 Changed 2 years ago by cypherpunks

Compilation started failing after the merge because of a mismatch between the type signature of crypto_rand and a mock function. The attached patch fixes the issue.

comment:10 in reply to:  9 Changed 2 years ago by teor

Replying to cypherpunks:

Compilation started failing after the merge because of a mismatch between the type signature of crypto_rand and a mock function. The attached patch fixes the issue.

This unit test used to return failure from the mocked crypto_rand. But the real crypto_rand never returned failure. It's probably not an issue, but we should check that the unit test still tests what we expect it to test.

comment:11 Changed 2 years ago by teor

Keywords: TorCoreTeam201511 added
Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

Bumped back to master, we should switch it back to 027 once it's been merged.

comment:12 Changed 2 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.7.x-final

Fixed with fe46fffd980cb97661b6bf0f16c66522d7e58e61.

(The original fix doesn't make sense, since the unit test was supposed to test what happened if crypto_rand failed... but that can't happen now. No backport on that fix, since the tortls tests aren't in 0.2.7)

comment:13 Changed 2 years ago by cypherpunks

The change in commit fe46fffd980cb97661b6bf0f16c66522d7e58e61 introduces a double newline which makes check-spaces complain.

comment:14 Changed 2 years ago by nickm

Keywords: TorCoreTeam201512 201511-deferred added; TorCoreTeam201511 removed

Bulk-move uncompleted items to december. :/

comment:15 Changed 2 years ago by nickm

Keywords: TorCoreTeam201512 201511-deferred removed

We can backport this to 0.2.7 or not at our leisure

comment:16 Changed 2 years ago by nickm

My current vote here is "this is a feature; no backport." Anybody disagree?

comment:17 Changed 2 years ago by teor

It's a feature. (Due to OpenSSL's implementation. Let's hope LibreSSL et al didn't change the semantics.)

comment:18 Changed 2 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final
Resolution: fixed
Status: needs_reviewclosed

Okay, marking these tickets as "no backport." Please reopen if you disagree.

Note: See TracTickets for help on using tickets.