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

Switches Darwin to arc4random #6350

Closed

Conversation

chris-huxtable
Copy link
Contributor

@chris-huxtable chris-huxtable commented Jul 7, 2018

Based on discussions on #6340.

  • Performance improvements
  • Works in chroot
  • No File Descriptors needed

Note: The same could also be done for FreeBSD.

@@ -7,6 +7,8 @@ lib LibC
rem : Int
end

fun arc4random : UInt32
fun arc4random_buf(x0 : Void*, x1 : SizeT) : Void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please:

  • Replace Void* with UInt8*
  • Remove : Void

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be the case even if it goes against the existing c definition?

void
arc4random_buf(void *buf, size_t nbytes);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that works... I thought Void* didn't work in Crystal, but for some reason it does...

It seems there's also a Void top-level type... it seems it was never removed from the language...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The : Void is implicit and the void * should be UInt8* in crystal. @asterite is correct.

@chris-huxtable chris-huxtable changed the title Switches Darwin to arc4Random Switches Darwin to arc4random Jul 7, 2018
@RX14
Copy link
Contributor

RX14 commented Jul 8, 2018

From the libsodium developers:

On MacOS:

  • arc4random() is not available on older versions of MacOS
  • arc4random() seems to read /dev/random to reseed itself, and opens/closes the file every time. Which is not safe in a chroot()ed environment and violates libsodium guarantees
  • before MacOS 10.2, arc4random() was actually using RC4, which should not be used for cryptographic keys due to biases.

It seems arc4random on osx is actually a userspace PRNG, which is not what we want here.

@chris-huxtable
Copy link
Contributor Author

chris-huxtable commented Jul 8, 2018

@RX14 - Based on my current research.

  • arc4random introduced in macOS 10.6 (2009)

  • arc4random_buf introduced in macOS 10.7 (2011)

  • arc4random is reseeded with getentropy, not /dev/[u]random

  • The documentation explicitly states to use arc4random as it is safe in chroot.

man arc4random:

They can be called in almost all environments, including chroot(2), and their use is encouraged over all other standard library functions for random numbers.

  • arc4random is based on an AES cipher as of macOS 10.12 (2016)

  • With regards to whether you should use arc4random or /dev/urandom/. The documentation is explicit. You should always use arc4random.

man random:

Applications requiring cryptographic quality randomness should use arc4random(3).

man urandom:

The same random data is also available from getentropy(2). Using the getentropy(2) system call interface will provide resiliency to file descriptor exhaustion, chroot, or sandboxing which can make /dev/random unavailable. Additionally, the arc4random(3) API provides a fast userspace random number generator built on the random data source and is preferred over directly accessing the system's random device.

man getentropy:

However, it should be noted that getentropy() is primarily intended for use in the construction and seeding of userspace PRNGs like arc4random(3) [...].

Its great that libsodium thinks they can do better. OpenBSD I am sure goes even further then libsodium. But I am not proposing we shim in (if even possible) OpenBSD's implementation or link in yet another library. Crystal should use the fastest/best built-in CSPRNG. On Darwin based systems that is arc4random.

@RX14
Copy link
Contributor

RX14 commented Jul 8, 2018

I'd trust the libsodium guys far far more than you or I. If they say it's wrong, it's wrong. And risking this just for performance?

@chris-huxtable
Copy link
Contributor Author

chris-huxtable commented Jul 8, 2018

@RX14 - You can trust whoever you like. I don't trust the libsodium guys and I would certainly trust the Apple guys before them with regards to how something is implemented in macOS.

I happen to be of the perspective everyone should just duplicate OpenBSD's arc4random and keep it up to date. That isn't the case though. What is the case is macOS has /dev/urandom and their implementation of arc4random.

All the official documentation says use arc4random, as does the preponderance of unofficial documentation/discussions. arc4random is a better choice then /dev/urandom on macOS.

In case I need to say it again. Apple explicitly says:

Applications requiring cryptographic quality randomness should use arc4random(3).

and,

Additionally, the arc4random(3) API [...] is preferred over directly accessing the system's random device.

@RX14
Copy link
Contributor

RX14 commented Jul 8, 2018

And until recently the linux manpages said using urandom wasn't cryptographically safe, which was against the advice of every cryptographer out there.

At the end of the day, this is a performance problem, and if there's any conflict at all, i'd rather stick with what we know works and is secure.

@konovod
Copy link
Contributor

konovod commented Jul 9, 2018

So, there are two main problems with arc4random

so even if we ignore first point, arc4random shouldn't be used on OSX10.11 and older.

On the other hand, getentropy was introduced in macOS 10.12 (Sep 20, 2016) and is a (strictly) better solution then /dev/urandom - it yields same data (not a userspace prng) and is safer because doesn't depends on file descriptors. So if a getentropy is available - it is a modern OSX and we can use either arc4random or getentropy (depending on the opinion about first point).

On the other other hand, it a libsodium issue cited before tells that getentropy uses \dev\urandom. I think the additional research is needed on this, man page isn't a proof.
From what Apple opensourced

if (getentropy(buf, size) == 0) {
		return;
	}
	// The above should never fail, but we'll try /dev/random anyway

so it in theory can fallback to reading from /dev/random, but does it possible in practice depends from can getentropy fail. I wasn't able to find getentropy source to check.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random::Secure MUST be a CSPRNG (cryptographically secure pseudo random number generator) and the underlying OS syscalls/sources we use must be proven secure by cryptographers and outlandish libraries written by them (e.g. libsodium).

I believe it was demonstrated that arc4random is either bad or imperfect as a trustable CSPRNG in older FreeBSD and macOS releases (at least). Maybe that changed in recent releases, but we need proof of that. Please conduct researches to debunk libsodium statements, they'll love it ❤️

If proven, we must use arc4random on safe systems only, preferably with a runtime check —e.g. we use the getrandom syscall on linux then fallback to /dev/urandom, binaries are safe to run on any kernel.

Until then, let's hold on.

@RX14
Copy link
Contributor

RX14 commented Jul 9, 2018

And even if arc4random is secure on 10.12, are all the runtime checks and complexity (read: vulnerabilities) worth it? Just for some performance?

@ysbaddaden
Copy link
Contributor

Closing. We can't trust ourselves on security topics. We need strong guarantees from cryptographers that something is safe and correct.

Sadly, we don't have enough guarantees that arc4random is cryptographically correct on macOS. The only guarantee we have is that it was very bad prior to macOS 10.12 😞

@ysbaddaden ysbaddaden closed this Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants