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

crypto/rand: use getentropy(2) on OpenBSD #13785

Closed
mmcco opened this issue Dec 31, 2015 · 21 comments
Closed

crypto/rand: use getentropy(2) on OpenBSD #13785

mmcco opened this issue Dec 31, 2015 · 21 comments

Comments

@mmcco
Copy link

mmcco commented Dec 31, 2015

It's the progenitor of Linux's getrandom(2), which Go already supports. I have a patch that works on AMD64. I won't submit a pull request until it's tested on i386 and ARM (help welcome!).

This generally just involves mirroring the approach used for getrandom(2). However, we don't need to test for availability, as all supported OpenBSD releases have getentropy(2). The only other caveat is that I had to add syscall.Syscall2() for OpenBSD, as getentropy(2) takes two arguments.

The interface is very simple - here's the man page.

It's a pretty straightforward change. I just wanted to mention it as early as possible, as per the Contribution Guidelines.

Thoughts?

@mdempsky mdempsky changed the title Add support for OpenBSD's getentropy(2) syscall internal/syscall/unix: use getentropy(2) on OpenBSD Dec 31, 2015
@mdempsky mdempsky added this to the Go1.7 milestone Dec 31, 2015
@mdempsky
Copy link
Member

Changing internal/syscall/unix to utilize getentropy() on OpenBSD sounds reasonable to me.

It won't be added to package syscall though (which is frozen), and I don't see a need to add it to golang.org/x/sys/unix either, since Linux's getrandom() isn't available there either.

There's no need to add syscall.Syscall2. You can use syscall.Syscall and just pass 0 as a dummy third argument.

Don't send a Github pull request. Upload a CL to Gerrit, like the Contribution Guidelines explain. (But expect to wait until after 1.6 is released for it to be reviewed.)

@mdempsky mdempsky changed the title internal/syscall/unix: use getentropy(2) on OpenBSD crypto/rand: use getentropy(2) on OpenBSD Dec 31, 2015
@minux
Copy link
Member

minux commented Dec 31, 2015 via email

@bradfitz
Copy link
Contributor

SGTM too, putting it in internal/syscall/unix next to the Linux getrandom stuff.

@gopherbot
Copy link

CL https://golang.org/cl/18219 mentions this issue.

@mmcco
Copy link
Author

mmcco commented Jan 4, 2016

Based on the syscall traces I'm seeing, it seems that I misinterpreted the logic in crypto/rand. How should I change it so that getentropy(2) is only used to seed the CSPRNG rather than being called directly? This is how the syscall is intended to be used.

@minux
Copy link
Member

minux commented Jan 4, 2016 via email

@mmcco
Copy link
Author

mmcco commented Jan 4, 2016

@minux I think there is a CSPRNG - see src/crypto/rand/rand_unix.go:85.

However, altGetRandom sources such as Linux's getrandom(2) are used directly rather than fed through the CSPRNG. This may not be the best choice from a performance and reliability perspective. I'd be interested to hear more about the tradeoffs to consider in Go specifically. Most C crypto guides I've read suggest that mature applications use userland CSPRNGs that are seeded with OS-supplied entropy sources rather than using data directly from the OS sources.

I've drafted a patch that uses OpenBSD's getentropy(2) through the CSPRNG rather than querying it directly. I'll share once I've verified this with ktrace(1) and kdump(1).

@mdempsky
Copy link
Member

mdempsky commented Jan 4, 2016

@mmcco If you want to propose that crypto/rand feed the kernel entropy source through another userspace CSPRNG, I suggest you do that as a separate issue. I'm not convinced that should be tied to whether crypto/rand uses getentropy on OpenBSD.

@minux
Copy link
Member

minux commented Jan 4, 2016 via email

@mmcco
Copy link
Author

mmcco commented Jan 4, 2016

@mdempsky:

I'm not convinced that should be tied to whether crypto/rand uses getentropy on OpenBSD.

The creators of getentropy frequently specify that it is intended to be used for seeding, not for direct randomness access. I can try to get more information on why this is the case, although I've mentioned a few probable reasons above.

I think there's a chance of a slight misunderstanding, though, based on your wording:

If you want to propose that crypto/rand feed the kernel entropy source through another userspace CSPRNG, I suggest you do that as a separate issue.

What is meant by "another" here? If you're referring to getentropy as a userland CSPRNG, remember: it's a system call directly to the kernel.

@mmcco
Copy link
Author

mmcco commented Jan 4, 2016

That said, I'm not opposed to opening a second CL.

@minux
Copy link
Member

minux commented Jan 4, 2016 via email

@mmcco
Copy link
Author

mmcco commented Jan 4, 2016

@mdempsky @minux In what order to you imagine these CLs being merged? The OpenBSD kernel developers probably won't want getentropy to be used directly even for a little while. But we can't link it into Go's CSPRNG until we support it.

Should I remove the assignment of altGetRandom in my existing CL so that getentropy remains unused? This might give an dead code error.

@mmcco
Copy link
Author

mmcco commented Jan 4, 2016

@minux From what I've read, calling directly is a misuse of the API. I don't think we should start there. I'll try to ask for more clarification.

I don't think any usual crypto use scenario is limited by randomness throughput unless you're building a service to generate random keys.

This sounds a lot like a network daemon that uses forward secret encryption. :-)

@mdempsky
Copy link
Member

mdempsky commented Jan 4, 2016

@mmcco I'm the OpenBSD kernel developer that added getentropy. There's no difference between reading from /dev/urandom and calling getentropy except that the latter limits you to 256 bytes per system call.

@mmcco
Copy link
Author

mmcco commented Jan 4, 2016

@mdempsky Oh, Jesus, I thought that name looked familiar. :P I assumed I must have been conflating names.

That, of course, gives me more confidence. I noticed that there's logic in the kernel's randomread() (the randomness pseudo-devices' backend) which creates a separate ChaCha20 instance for >2048-byte requests. I therefore figured that this logic was considered more robust for large or repeated entropy pulls. How does that factor in?

Most importantly, it seems that there can't be an information leak from doing too many getentropy reads because it's functionally no different from a series of /dev/urandom reads.

@mdempsky
Copy link
Member

mdempsky commented Jan 4, 2016

The extra ChaCha20 logic in randomread for >2048-byte reads is just a performance improvement so the kernel doesn't need to keep re-acquiring rndlock when generating a lot of randomness. It's not otherwise relevant to correctness/security here.

So I'd propose you start by just seeing if replacing /dev/urandom reads with getentropy on OpenBSD. Unless there are lots of Read calls that ask for >256 bytes at a time, that should be a pure net win.

Separately, you can propose using a userspace CSPRNG that we seed from the kernel, but I'm not convinced at this time that that's necessary. (I also agree with DJB that "We want the OS to
be centralizing not just the entropy gathering but also the PRNG."
.)

Cheers

@mikioh
Copy link
Contributor

mikioh commented Jan 14, 2016

As per https://github.com/golang/go/wiki/OpenBSD, please open a new issue if you want to drop support for OpenBSD 5.5 in Go 1.7 and above. No need to change codes, just for release notes and WiKi.

@mmcco
Copy link
Author

mmcco commented Jan 14, 2016

@mikioh Thanks for that, I wasn't aware.

I'll edit the wiki if/when this gets merged. It's planned for Go 1.7, which will ship first with OpenBSD 6.0, so I don't think this should be an issue for anyone being remotely responsible with system upgrades.

@mikioh
Copy link
Contributor

mikioh commented Feb 26, 2016

@mmcco,

Please open a new issue that describes Go 1.7 drops support for OpenBSD 5.5. and below for people who have to write doc/go1.7.html, WiKi, etc.

@mmcco
Copy link
Author

mmcco commented Feb 29, 2016

@mikioh Done in #14572. Thanks!

@golang golang locked and limited conversation to collaborators Feb 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants