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.

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

system: use getrandom() and fallback on urandom if syscall doesn't work #1280

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

illwieckz
Copy link
Member

Sometime with less common compilers I get an error about getrandom syscall failures, but we already have an alternate implementation. It happens that the checks is currently done at build time, but then even if the headers are there, it may fail at run time. So we technically can try the syscall then the urandom read if the syscall fails.

What I don't know though, is that if doing that is reducing security or not.

@slipher
Copy link
Member

slipher commented Sep 6, 2024

Sometime with less common compilers I get an error about getrandom syscall failures

Why would it change depending on the compiler?

@illwieckz
Copy link
Member Author

illwieckz commented Sep 6, 2024

I have no idea, I get this kind of error for example when:

  • I build with Zig
  • I build with ICC on Ubuntu Noble and run on Ubuntu Noble

Even more funny: building with ICC from Ubuntu 24.04 Noble (bind mount on the compiler folder) in Ubuntu 22.10 Kinetic chroot onside Ubuntu Noble and running on Ubuntu Noble doesn't reproduce the error…

I guess some libc mismatch or things like that may happen…

@illwieckz illwieckz changed the title system: fallback on urandom if getrandom syscall doesn't work system: use getrandom() and fallback on urandom if syscall doesn't work Sep 12, 2024
@illwieckz
Copy link
Member Author

illwieckz commented Sep 12, 2024

I noticed that if I do getrandom(dest, size, GRND_NONBLOCK) instead of syscall(SYS_getrandom, dest, size, GRND_NONBLOCK), it works on ICC and Zig too. And getrandom is expected to be the same syscall. So I guess the error was due to a weird library issue.

Anyway, I investigated more and the getrandom syscall also uses /dev/urandom, the only difference with reading /dev/urandom directly is that the getrandom syscall with GRND_NONBLOCK is not blocking. So there should not be any security issue at reading /dev/urandom as a fallback.

I also don't know what this randomness is for, but such fallback can't harm.

For reference here is the PR that added the feature:

Discussion happened there:

Actually, the only expected situation where the syscall would not work is that if the kernel is 10 years old, which is unlikely, but in case we face again a weird bug like the one I faced, it looks harmless to keep the fallback.

Edit: Also I noticed my initial code had an obvious bug, it was doing the fallback only if it succeeded… 🤦‍♀️️ It's now fixed.

src/common/System.cpp Outdated Show resolved Hide resolved
@illwieckz illwieckz force-pushed the illwieckz/getrandom branch 2 times, most recently from 9b81592 to a7f658f Compare September 12, 2024 01:28
@illwieckz
Copy link
Member Author

Just for the knowledge, the error I was getting before with syscall(SYS_getrandom, …) was “Invalid argument”, so EINVAL.

src/common/System.cpp Outdated Show resolved Hide resolved
@slipher
Copy link
Member

slipher commented Oct 30, 2024

LGTM

@illwieckz illwieckz merged commit f74d8e4 into master Oct 31, 2024
9 checks passed
@illwieckz illwieckz deleted the illwieckz/getrandom branch October 31, 2024 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants