Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix seeding from random device w/o getrandom syscall #8251

Closed
wants to merge 4 commits into from
Closed

Fix seeding from random device w/o getrandom syscall #8251

wants to merge 4 commits into from

Conversation

bernd-edlinger
Copy link
Member

Use select to wait for /dev/random in readable state,
but do not actually read anything from /dev/random,
use /dev/urandom first.

Use linux define __NR_getrandom instead of the
glibc define SYS_getrandom, in case the kernel headers
are more current than the glibc headers.

Fixes #8215

Checklist
  • documentation is added or updated
  • tests are added or updated

@bernd-edlinger
Fix seeding from random device w/o getrandom syscall
cfd6c0f 
Use select to wait for /dev/random in readable state,
but do not actually read anything from /dev/random,
use /dev/urandom first.

Use linux define __NR_getrandom instead of the
glibc define SYS_getrandom, in case the kernel headers
are more current than the glibc headers.

Fixes openssl#8215
@bernd-edlinger bernd-edlinger added branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch 3.0.0 labels Feb 15, 2019
@bernd-edlinger
Copy link
Member Author

This needs a kernel patch to be fully functional:
https://marc.info/?l=linux-kernel&m=155023911204953&w=2

But without the kernel patch it is also an improvement, because
the select will wait until at least 64 bits of entropy are available
instead of the usual 128 bits of entropy.
Also by not reading out data from the /dev/random it prevents
other application which use syscall getrandom from starving.

@bernd-edlinger
Copy link
Member Author

Actually with the kernel patch the syscall getrandom will be faster, because the
blocking pool will be completely empty and need to wait for at least the number
of entropy bits, in /proc/sys/kernel/random/read_wakeup_threshold, 64 by default.

levitte
levitte previously requested changes Feb 15, 2019
View reviewed changes
e_os.h Outdated Show resolved Hide resolved
kroeckx
kroeckx reviewed Feb 16, 2019
View reviewed changes
e_os.h Show resolved Hide resolved
kroeckx
kroeckx reviewed Feb 16, 2019
View reviewed changes
crypto/rand/rand_unix.c Outdated Show resolved Hide resolved
@kroeckx
Copy link
Member

kroeckx commented Feb 28, 2019

Any update on this?

@bernd-edlinger
Copy link
Member Author

I have meanwhile posted a V5 of the linux patch here:
https://marc.info/?l=linux-kernel&m=155059615110666&w=2

I got a first reply from Theodore Y. Ts'o, but I am still waiting for his final decision.
It is possible that the patch needs to be split up in smaller pieces addressing
single issues, or some more changes will be required.

The main issues this patch addresses, are:

  • the first few bytes from /dev/random after system boot can have
    less entropy in the input_pool than expected. Depends on the configuration.
    The default configuration implies 64 bits entropy.
    This is since linux 2.6.12 earlier versions are not in the git archive.
  • /dev/random is readable before the /dev/urandom is fully seeded.
    This started with linux v4.8 and prevails till today.
    Before linux v4.8 it was true, that reading one byte from /dev/random
    did in fact ensure that /dev/urandom is properly seeded, but the
    first few bytes from /dev/random had less entropy than expected.

This fixes also some issues, I did not notice initially:

  • Actively reading from /dev/random can prevent the CRNG from
    ever be fully seeded.
    This started with linux v4.8 and prevails till today.
  • On system with little entropy input there it can happen that select on /dev/random
    indicates readability and then the CRNG is reseeded and takes so much entropy
    out of the input pool that the select changes state, without any read from /dev/random
    This started with linux v4.8 and prevails till today.
    Before v4.8 the reseeding did reserver the last 2*random_read_wakeup_bits,
    and I want to restore that behavior.
  • If a system runs long enough that the jiffies overrun the CRNG re-seeding
    does not happen every 5 minutes, but immediately, since the condition for a
    reseed time_after(crng_global_init_time, crng->init_time) will start to trigger,
    after 248 days system uptime, there are systems that run such long time.
    This would make /dev/random block unexpectedly.
    This started with linux v4.8 and prevails till today.

@kroeckx
Copy link
Member

kroeckx commented Mar 1, 2019 via email

@bernd-edlinger
Copy link
Member Author

Well yes, I think so. @levitte do you agree?

It is theoretically possible to see select blocking but only when you use a mis-configured
cross compiler, like I did, or install a new kernel without updating the kernel headers.
But over time, the kernel will be fixed.

@bernd-edlinger bernd-edlinger dismissed levitte’s stale review March 1, 2019 12:08

OK, @levittes review comments are addressed, unless someone objects I'll merge this in the evening.

levitte pushed a commit that referenced this pull request Mar 1, 2019
@bernd-edlinger
Fix seeding from random device w/o getrandom syscall
c352bd0 
Use select to wait for /dev/random in readable state,
but do not actually read anything from /dev/random,
use /dev/urandom first.

Use linux define __NR_getrandom instead of the
glibc define SYS_getrandom, in case the kernel headers
are more current than the glibc headers.

Fixes #8215

Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
(Merged from #8251)

(cherry picked from commit 38023b8)
levitte pushed a commit that referenced this pull request Mar 1, 2019
@bernd-edlinger
Fix seeding from random device w/o getrandom syscall
38023b8 
Use select to wait for /dev/random in readable state,
but do not actually read anything from /dev/random,
use /dev/urandom first.

Use linux define __NR_getrandom instead of the
glibc define SYS_getrandom, in case the kernel headers
are more current than the glibc headers.

Fixes #8215

Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
(Merged from #8251)
@bernd-edlinger
Copy link
Member Author

Merged to master as 38023b8, and 1.1.1 as c352bd0. Thanks!

@slontis slontis mentioned this pull request Mar 6, 2019
Closed
@slontis
Copy link
Member

slontis commented Mar 6, 2019

Let me know if you want more info - or want me to try something out on this platform.

@slontis
Copy link
Member

slontis commented Mar 6, 2019

@paulidale @jon-oracle

@t8m
Copy link
Member

t8m commented Mar 6, 2019

OK, something more to patch-out downstream. :(

@kroeckx
Copy link
Member

kroeckx commented Mar 6, 2019

Should we change the select on /dev/random a Linux only thing?

@bernd-edlinger
Copy link
Member Author

That would make sense.

@mspncp mspncp mentioned this pull request Jun 4, 2019
Closed
@ofrobots ofrobots mentioned this pull request Aug 1, 2019
Closed
@bernd-edlinger bernd-edlinger mentioned this pull request Aug 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kroeckx kroeckx kroeckx approved these changes

@levitte levitte levitte left review comments

Assignees
No one assigned
Labels
branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RAND_DRBG uses uninitialized urandom read on linux
6 participants
@bernd-edlinger @kroeckx @slontis @t8m @levitte @mspncp