-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Security Fix: don't buffer reads from /dev/urandom #5848
Security Fix: don't buffer reads from /dev/urandom #5848
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably add a comment explaining the reason for using the C API directly in the code.
src/crystal/system/unix/urandom.cr
Outdated
@@ -1,35 +1,51 @@ | |||
{% skip_file unless flag?(:unix) && !flag?(:openbsd) && !flag?(:linux) %} | |||
{% skip_file unless flag?(:unix) && !flag?(:openbsd) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -0,0 +1,15 @@ | |||
{% skip_file unless flag?(:unix) && !flag?(:openbsd) && !flag?(:linux) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to test for Unix flag here, it's already in the unix folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole line is useless because we correctly require in system/random
, but for some reason (someone doing a stupid require "./src/*"
) we must prevent files to be parsed, and add duplicates.
So no, we must check for unix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the other files in this folder (or win32
) has such a check, there are only skip_file
s to limit to a specific Unix variant.
If someone tries to require everything in the stdlib's src path, then be it. Live with the errors. I don't see a reasonable cause to safeguard against this.
src/crystal/system/unix/random.cr
Outdated
def self.next_u : UInt8 | ||
buf = uninitialized UInt8[1] | ||
Crystal::System::Urandom.random_bytes(buf.to_slice) | ||
buf.unsafe_as(UInt8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely buf[0]
would be nicer?
src/crystal/system/unix/urandom.cr
Outdated
read_bytes = LibC.read(fd, buf, buf.size) | ||
if read_bytes < 0 | ||
if Errno.value == Errno::EINTR || Errno.value == Errno::EAGAIN || Errno.value == Errno::EWOULDBLOCK | ||
Fiber.yield |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I highly doubt that urandom would ever block, and even if it did this implementation is far from the correct one using libevent or epoll. I'd recommend just not setting nonblock at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe filling a large slice will, but the call may be interrupted anyway, so we should rescue and yield to the scheduler (there is probably a signal handler pending). It's not ideal, but it does the job —and there are no easy primitives to access the libevent base, or create a simple fd event 😢
Note that I'm not fond of this pull request. I wish there were some nice structs/modules to quickly and safely leverage open/stat/fnctl/read/write/close and creating events without having to go bare libc, but without requiring full crystal either. |
@ysbaddaden too many layers of abstraction are also a bit of a bad thing. |
Well, it already is:
I wish fd related IO modules were more composable and reusable for different scenarios. For example here I'd love to extend |
This sounds like |
@ysbaddaden i'd rather just make
|
Sadly, using I'm not even sure we should go through the event loop at all (we may buffer crypto related data for longer than required), so I'll remove |
I just checked how go handled this (evented or not), but... they use a |
This avoids the weird behavior of IO::Buffered that always buffers reads (and only stops buffering writes). This is a security issue in the case of Random::Secure. Switching to C calls directly completely avoids such issues. We also avoid evented/nonblocking IO since reading from `/dev/urandom` should never block (except maybe very early during the boot process). Opening `/dev/urandom` now relies on a lock, so a temporary failure (e.g. exhausted file descriptor pool) can later be recovered from.
ab80a3a
to
d81d41c
Compare
What's the status of this? |
To decide whether we 1. merge this (use low level libc calls) or 2. merely fix IO#sync to stop buffering reads in addition to writes or 3. merge both. |
I think we'll go with "fix IO#sync to stop buffering reads". But maybe we should rename |
Sounds good. |
It wasn't immediately clear to me what the "weird" behavior is, and what the security issue is, care to explain them a bit, or point me to a link where it is described? Thanks! |
Buffering caused an immediate bug with fork, where all child processes (and parent) had the exact same random sequences until the buffer was emptied. You also don't want to read 8KB of random data from the system CSPRNG into memory, when all you need is 8 bytes to initialize the Hash seed (for example), leaving 8184 bytes sitting in memory forever or for long enough that someone may eavesdrop (someone said spectre?) and know further random byte sequences in advance, among other things :( |
OK fair enough. Though if somebody is able to access random memory bytes I'd call that an even bigger problem (startup penalty is pretty minimal... :) |
This avoids the weird behavior of IO::Buffered that always buffers reads (and only stops buffering writes). This is a security issue in the case of Random::Secure. Let's switch to direct C calls to completely avoid such issues (now and in the future).
Also unifies the urandom implementation for UNIX (generic) and Linux (getrandom fallback).