code review 123260044: crypto/rand: use getrandom system call on Linux

crypto/rand: use getrandom system call on Linux

Adds internal/syscall package.

Fixes Issue 8520

[bradfitz, 2014-08-12 19:42:50]

Hello agl@chromium.org (cc: golang-codereviews@googlegroups.com,
iant@golang.org, rsc@golang.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[agl1, 2014-08-12 20:32:35]

https://codereview.appspot.com/123260044/diff/40001/src/pkg/crypto/rand/rand_...
File src/pkg/crypto/rand/rand_linux.go (right):

https://codereview.appspot.com/123260044/diff/40001/src/pkg/crypto/rand/rand_...
src/pkg/crypto/rand/rand_linux.go:16: n, err := syscall.GetRandom(p, 0)
The new system call isn't actually like /dev/urandom in one important respect:
there's no "do the best that you can" mode.

While /dev/urandom doesn't block and always returns data, the new system call
will refuse to return anything until the pool is ready. This stops attackers
from climbing up the pool(*), but might be a big surprise to Go programs.

I think that GRND_NONBLOCK should be passed here so that, if the pool isn't
ready, we'll fall back on /dev/urandom until the syscall will work.

(* The attack is this: imagine that the pool is currently empty and receives one
bit of entropy per millisecond, like clockwork. If I, as a userspace process,
can read from it immediately then I can figure out what the bit is every
millisecond by reading repeatedly and thus know the full state of the pool -
which is bad. If the pool waits until it has, say, 128 bits before disclosing
anything then I'll never be able to get over that step.)

[rsc, 2014-08-12 20:34:59]

+r for creation of internal/syscall in this CL (it is tiny)

[r, 2014-08-12 20:38:21]

LGTM for internal/syscall

https://codereview.appspot.com/123260044/diff/40001/src/pkg/internal/syscall/...
File src/pkg/internal/syscall/getrandom_linux.go (right):

https://codereview.appspot.com/123260044/diff/40001/src/pkg/internal/syscall/...
src/pkg/internal/syscall/getrandom_linux.go:25: // GRND_NONBLOCK means "Don't
block and return EAGAIN instead"
Return EAGAIN rather than blocking.

[bradfitz, 2014-08-12 20:54:26]

Hello agl@chromium.org, agl@golang.org, rsc@golang.org, r@golang.org (cc:
golang-codereviews@googlegroups.com, iant@golang.org),

Please take another look.

[r, 2014-08-12 20:55:06]

LGTM for internal/syscall

https://codereview.appspot.com/123260044/diff/60001/src/pkg/internal/syscall/...
File src/pkg/internal/syscall/getrandom_linux.go (right):

https://codereview.appspot.com/123260044/diff/60001/src/pkg/internal/syscall/...
src/pkg/internal/syscall/getrandom_linux.go:28: // GRND_RANDOM means the use the
/dev/random pool instead of /dev/urandom.
s/the/to/

[bradfitz, 2014-08-12 20:55:08]

Updated per conversation with agl.

https://codereview.appspot.com/123260044/diff/40001/src/pkg/internal/syscall/...
File src/pkg/internal/syscall/getrandom_linux.go (right):

https://codereview.appspot.com/123260044/diff/40001/src/pkg/internal/syscall/...
src/pkg/internal/syscall/getrandom_linux.go:25: // GRND_NONBLOCK means "Don't
block and return EAGAIN instead"
On 2014/08/12 20:38:21, r wrote:
> Return EAGAIN rather than blocking.

I was just copying the text from the manpage but I've rewritten them both in Go
style.

[bradfitz, 2014-08-12 20:56:38]

Hello agl@chromium.org, agl@golang.org, rsc@golang.org, r@golang.org (cc:
golang-codereviews@googlegroups.com, iant@golang.org),

Please take another look.

[bradfitz, 2014-08-12 20:56:38]

https://codereview.appspot.com/123260044/diff/60001/src/pkg/internal/syscall/...
File src/pkg/internal/syscall/getrandom_linux.go (right):

https://codereview.appspot.com/123260044/diff/60001/src/pkg/internal/syscall/...
src/pkg/internal/syscall/getrandom_linux.go:28: // GRND_RANDOM means the use the
/dev/random pool instead of /dev/urandom.
On 2014/08/12 20:55:05, r wrote:
> s/the/to/

Done.

[agl1, 2014-08-12 20:58:41]

https://codereview.appspot.com/123260044/diff/60001/src/pkg/crypto/rand/rand_...
File src/pkg/crypto/rand/rand_linux.go (right):

https://codereview.appspot.com/123260044/diff/60001/src/pkg/crypto/rand/rand_...
src/pkg/crypto/rand/rand_linux.go:24: // machine without hardware entropy
available, it'll fall back
There's something wrong with this sentence.

https://codereview.appspot.com/123260044/diff/60001/src/pkg/crypto/rand/rand_...
File src/pkg/crypto/rand/rand_unix.go (right):

https://codereview.appspot.com/123260044/diff/60001/src/pkg/crypto/rand/rand_...
src/pkg/crypto/rand/rand_unix.go:30: Reader = &devReader{name: "/dev/urandom"}
maybe make "/dev/urandom" a const?

https://codereview.appspot.com/123260044/diff/60001/src/pkg/crypto/rand/rand_...
src/pkg/crypto/rand/rand_unix.go:46: r.mu.Lock()
if we moved the altGetRandom test and call outside of the lock, we could see a
significant reduction in contention for code that reads lots of random bytes.

https://codereview.appspot.com/123260044/diff/60001/src/pkg/crypto/rand/rand_...
src/pkg/crypto/rand/rand_unix.go:50: if altGetRandom(b) {
this line can just be another && clause in the if on the previous line.

[r, 2014-08-12 20:58:53]

https://codereview.appspot.com/123260044/diff/80001/src/pkg/internal/syscall/...
File src/pkg/internal/syscall/getrandom_linux.go (right):

https://codereview.appspot.com/123260044/diff/80001/src/pkg/internal/syscall/...
src/pkg/internal/syscall/getrandom_linux.go:28: // GRND_RANDOM means to use the
/dev/random pool instead of /dev/urandom.
this time for sure:
s/to //
for consistency with the comment above.

not a submit-stopper i admit

[bradfitz, 2014-08-12 21:11:27]

https://codereview.appspot.com/123260044/diff/60001/src/pkg/crypto/rand/rand_...
File src/pkg/crypto/rand/rand_linux.go (right):

https://codereview.appspot.com/123260044/diff/60001/src/pkg/crypto/rand/rand_...
src/pkg/crypto/rand/rand_linux.go:24: // machine without hardware entropy
available, it'll fall back
On 2014/08/12 20:58:40, agl1 wrote:
> There's something wrong with this sentence.

Done.

https://codereview.appspot.com/123260044/diff/60001/src/pkg/crypto/rand/rand_...
File src/pkg/crypto/rand/rand_unix.go (right):

https://codereview.appspot.com/123260044/diff/60001/src/pkg/crypto/rand/rand_...
src/pkg/crypto/rand/rand_unix.go:30: Reader = &devReader{name: "/dev/urandom"}
On 2014/08/12 20:58:40, agl1 wrote:
> maybe make "/dev/urandom" a const?

Done.

https://codereview.appspot.com/123260044/diff/60001/src/pkg/crypto/rand/rand_...
src/pkg/crypto/rand/rand_unix.go:46: r.mu.Lock()
On 2014/08/12 20:58:40, agl1 wrote:
> if we moved the altGetRandom test and call outside of the lock, we could see a
> significant reduction in contention for code that reads lots of random bytes.

Done.

https://codereview.appspot.com/123260044/diff/80001/src/pkg/internal/syscall/...
File src/pkg/internal/syscall/getrandom_linux.go (right):

https://codereview.appspot.com/123260044/diff/80001/src/pkg/internal/syscall/...
src/pkg/internal/syscall/getrandom_linux.go:28: // GRND_RANDOM means to use the
/dev/random pool instead of /dev/urandom.
On 2014/08/12 20:58:52, r wrote:
> this time for sure:
> s/to //
> for consistency with the comment above.
> 
> not a submit-stopper i admit
> 

Done.

[bradfitz, 2014-08-12 21:11:28]

Hello agl@chromium.org, agl@golang.org, rsc@golang.org, r@golang.org (cc:
golang-codereviews@googlegroups.com, iant@golang.org),

Please take another look.

[agl1, 2014-08-12 21:15:09]

LGTM

[bradfitz, 2014-08-12 21:35:30]

*** Submitted as https://code.google.com/p/go/source/detail?r=a70c897599c4 ***

crypto/rand: use getrandom system call on Linux

Adds internal/syscall package.

Fixes Issue 8520

LGTM=r, agl
R=agl, rsc, r
CC=golang-codereviews, iant
https://codereview.appspot.com/123260044