Security Fix: don't buffer reads from /dev/urandom #5848

ysbaddaden · 2018-03-21T11:19:25Z

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).

straight-shoota

You should probably add a comment explaining the reason for using the C API directly in the code.

straight-shoota · 2018-03-21T11:25:20Z

src/crystal/system/unix/urandom.cr

@@ -1,35 +1,51 @@
-{% skip_file unless flag?(:unix) && !flag?(:openbsd) && !flag?(:linux) %}
+{% skip_file unless flag?(:unix) && !flag?(:openbsd) %}


straight-shoota · 2018-03-21T11:25:24Z

src/crystal/system/unix/random.cr

@@ -0,0 +1,15 @@
+{% skip_file unless flag?(:unix) && !flag?(:openbsd) && !flag?(:linux) %}


You don't need to test for Unix flag here, it's already in the unix folder.

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.

None of the other files in this folder (or win32) has such a check, there are only skip_files 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.

RX14 · 2018-03-21T11:37:49Z

src/crystal/system/unix/random.cr

+  def self.next_u : UInt8
+    buf = uninitialized UInt8[1]
+    Crystal::System::Urandom.random_bytes(buf.to_slice)
+    buf.unsafe_as(UInt8)


Surely buf[0] would be nicer?

RX14 · 2018-03-21T11:42:39Z

src/crystal/system/unix/urandom.cr

+        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


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.

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 😢

ysbaddaden · 2018-03-21T13:07:42Z

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.

RX14 · 2018-03-21T13:29:28Z

@ysbaddaden too many layers of abstraction are also a bit of a bad thing.

ysbaddaden · 2018-03-21T14:09:12Z

Well, it already is:

File end-user interface;
IO::FileDescriptor —supposed fd abstraction, forcefully includes IO::Buffered, and duplicated code with Socket;
Crystal::System::FileDescriptor —actual fd abstraction;
IO::Syscall —actually handles pending read/write events, maybe it should be a generic IO::Evented module to transparently handle evented read/write calls;
Scheduler —to create the fd read/write events but limited to IO::FileDescriptor and Socket (more duplicated code).

I wish fd related IO modules were more composable and reusable for different scenarios. For example here I'd love to extend IO, include Crystal::System::FileDescriptor (maybe IO::FileDescriptor if bare enough) as well as IO::Evented (just in case) but certainly not IO::Buffered, yet, I can't choose that.

straight-shoota · 2018-03-21T14:49:02Z

This sounds like IO::FileDescriptor might need a review?

RX14 · 2018-03-21T15:11:35Z

@ysbaddaden i'd rather just make File more configurable. If we made sync = true turn off read buffering then we could continue to use File here.

IO::Syscall and Crystal::System::FileDescriptor and Scheduler are pretty much implementation details, I agree they could be done better or rethought but that can be done at any time, even after 1.0.

ysbaddaden · 2018-03-22T11:09:17Z

Sadly, using File for reading from /dev/urandom is only pushing for more security issues down the line.

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 O_NONBLOCK and only rescue the EINTR errno.

ysbaddaden · 2018-03-22T11:27:01Z

I just checked how go handled this (evented or not), but... they use a bufio to buffer reads from /dev/urandom 😨

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.

asterite · 2018-04-28T13:47:29Z

What's the status of this?

ysbaddaden · 2018-04-28T23:53:05Z

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.

asterite · 2018-04-29T00:02:10Z

I think we'll go with "fix IO#sync to stop buffering reads". But maybe we should rename sync to buffered, because "sync" means "as soon as I write something, I want it written, so writes are synced with their corresponding operations (and not delayed)". But "sync" when reading makes no sense.

ysbaddaden · 2018-04-29T10:42:18Z

Sounds good.

rdp · 2019-11-16T00:07:21Z

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!

ysbaddaden · 2019-11-16T08:46:10Z

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 :(

rdp · 2019-11-16T15:33:53Z

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... :)

straight-shoota reviewed Mar 21, 2018

View reviewed changes

RX14 reviewed Mar 21, 2018

View reviewed changes

ysbaddaden force-pushed the security-fix-dont-buffer-dev-urandom branch from ab80a3a to d81d41c Compare March 22, 2018 14:01

ysbaddaden closed this Apr 29, 2018

ysbaddaden deleted the security-fix-dont-buffer-dev-urandom branch June 27, 2018 08:08

rdp mentioned this pull request Mar 4, 2020

Is Crystal slower than Ruby in generating randoms? #6340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security Fix: don't buffer reads from /dev/urandom #5848

Security Fix: don't buffer reads from /dev/urandom #5848

ysbaddaden commented Mar 21, 2018

straight-shoota left a comment

straight-shoota Mar 21, 2018

straight-shoota Mar 21, 2018

ysbaddaden Mar 21, 2018

straight-shoota Mar 21, 2018 •

edited

RX14 Mar 21, 2018

RX14 Mar 21, 2018

ysbaddaden Mar 21, 2018

ysbaddaden commented Mar 21, 2018

RX14 commented Mar 21, 2018

ysbaddaden commented Mar 21, 2018

straight-shoota commented Mar 21, 2018

RX14 commented Mar 21, 2018 •

edited

ysbaddaden commented Mar 22, 2018

ysbaddaden commented Mar 22, 2018

asterite commented Apr 28, 2018

ysbaddaden commented Apr 28, 2018

asterite commented Apr 29, 2018

ysbaddaden commented Apr 29, 2018

rdp commented Nov 16, 2019

ysbaddaden commented Nov 16, 2019

rdp commented Nov 16, 2019

		@@ -1,35 +1,51 @@
		{% skip_file unless flag?(:unix) && !flag?(:openbsd) && !flag?(:linux) %}
		{% skip_file unless flag?(:unix) && !flag?(:openbsd) %}

		@@ -0,0 +1,15 @@
		{% skip_file unless flag?(:unix) && !flag?(:openbsd) && !flag?(:linux) %}

Security Fix: don't buffer reads from /dev/urandom #5848

Security Fix: don't buffer reads from /dev/urandom #5848

Conversation

ysbaddaden commented Mar 21, 2018

straight-shoota left a comment

Choose a reason for hiding this comment

straight-shoota Mar 21, 2018

Choose a reason for hiding this comment

straight-shoota Mar 21, 2018

Choose a reason for hiding this comment

ysbaddaden Mar 21, 2018

Choose a reason for hiding this comment

straight-shoota Mar 21, 2018 • edited

Choose a reason for hiding this comment

RX14 Mar 21, 2018

Choose a reason for hiding this comment

RX14 Mar 21, 2018

Choose a reason for hiding this comment

ysbaddaden Mar 21, 2018

Choose a reason for hiding this comment

ysbaddaden commented Mar 21, 2018

RX14 commented Mar 21, 2018

ysbaddaden commented Mar 21, 2018

straight-shoota commented Mar 21, 2018

RX14 commented Mar 21, 2018 • edited

ysbaddaden commented Mar 22, 2018

ysbaddaden commented Mar 22, 2018

asterite commented Apr 28, 2018

ysbaddaden commented Apr 28, 2018

asterite commented Apr 29, 2018

ysbaddaden commented Apr 29, 2018

rdp commented Nov 16, 2019

ysbaddaden commented Nov 16, 2019

rdp commented Nov 16, 2019

straight-shoota Mar 21, 2018 •

edited

RX14 commented Mar 21, 2018 •

edited