995069 - Open /dev/urandom in JS engine conflicts with Sandbox

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Stephanie Ouillon [:arroway] (needinfo me)]

Hitting an open syscall when the JS engine is generating a random seed: open("/dev/urandom", O_RDONLY)

Return address points to js/src/jsmath.cpp:673

Comment 1

[Stephanie Ouillon [:arroway] (needinfo me)]

This is happening when an app starts up.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Move process sandboxing bugs to their new, separate component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Use getrandom syscall if available

On Linux the preferred way to get random bytes these days is the getrandom syscall because it works better with chroots, etc. Unfortunately the glibc wrapper for this was added very recently, so the most reliable way to use it is syscall().

This patch is pretty conservative: if the syscall fails for whatever reason we use the current /dev/urandom fallback path. (Note that we use arc4random on Android and the BSDs.)

I tested this patch on Ubuntu 16.04 and it works fine. The syscall succeeds and seed is set to a random value.

Comment 4

[Jan de Mooij [:jandem]]

Note that our sandbox already supports this system call, because Rust code uses it too. See bug 1284452.

Comment 5

[Jan de Mooij [:jandem]]

I did a Try push and downloaded the shell build, but it's not using this syscall. Maybe we're using a very old glibc there that does not define SYS_getrandom?

Comment 6

[Jan de Mooij [:jandem]]

Our options are (a) hardcoding the number, like Rust does [0], or (b) keeping the patch as-is so that local and distro builds can use it at least.

[0] https://github.com/rust-lang/rust/blob/3fc8304fd92720c688f3d6ac30b3a728d15f7a33/src/libstd/sys/unix/rand.rs#L52-L57

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Use getrandom if available

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 8834008 [details] [diff] [review]
Use getrandom if available

Review of attachment 8834008 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jan de Mooij [:jandem] from comment #5)
> I did a Try push and downloaded the shell build, but it's not using this
> syscall. Maybe we're using a very old glibc there that does not define
> SYS_getrandom?

(In reply to Jan de Mooij [:jandem] from comment #6)
> Our options are (a) hardcoding the number, like Rust does [0], or (b)
> keeping the patch as-is so that local and distro builds can use it at least.

Your patch LGTM, but what if we do both (a) and (b)? #include syscall.h and, #ifndef SYS_getrandom, then define SYS_getrandom for our supported architectures using Rust's NR_GETRANDOM values? That way the code that calls syscall() is the same on all build machines, only the declaration of SYS_getrandom changes.

If we did this, we should static_assert that syscall.h's SYS_getrandom values match our expected NR_GETRANDOM values. This would help avoid any surprises if syscall.h changes on some architecture.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

FYI, you probably don't need the static_assert; the Linux syscall ABI's stability is taken very seriously, and syscall numbers are not reassigned.  (Contrast Windows, where only the library call interface has a stability guarantee.)

See also: security/sandbox/chromium/sandbox/linux/system_headers (or the original version of that in the Chromium tree).

Comment 10

[Jan de Mooij [:jandem]]

OK, this all sounds good to me. I'll post an updated patch tomorrow.

We are using GenerateRandomSeed now for some other things in the JS engine, so I want to make sure we use a strong RNG even if someone runs Firefox in a chroot environment (without /dev/urandom). Doing both (a) and (b) would be great for that and will also help sandboxing.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Patch v2

This seems to work. I now see the getrandom syscalls with strace when I run a try build locally \o/

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 8834288 [details] [diff] [review]
Patch v2

Review of attachment 8834288 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8137a83c2a
Use getrandom system call on Linux if available instead of opening /dev/urandom. r=cpeterson

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a8137a83c2a

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8834288 [details] [diff] [review]
Patch v2

Given that we now use GenerateRandomSeed in ComputeRandomAllocationAddress, I'd feel better if we uplifted this patch too. It should be very safe as the actual code changes are small and should only affect Linux desktop (on Android and OS X we use arc4random).

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: In some environments more predictable random numbers generated.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only affects Linux desktop and the patch is small.
[String changes made/needed]: None.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8834288 [details] [diff] [review]
Patch v2

use getrandom syscall on linux if available, aurora53+, beta52+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7ea61ef9175d

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8345541aea0b

Comment 19

[Gerry Chang [:gchang]]

Hi :jandem,
For ESR, it doesn't match the esr criteria (only sec-high & sec-critical issues), what is the user impact if we won't fix this in esr45?

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8834288 [details] [diff] [review]
Patch v2

OK I'm fine not taking this on ESR45.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/8345541aea0b

Comment 22

[Jan Horak [:jhorak]]

Attachment: missing GETRANDOM_NR workaround

We cannot build on s390, s390x, the GETRANDOM_NR is missing on s390(x) -  I don't know what syscall numbers this architecture has. Attaching workaround patch for people who are interested.

Comment 23

[Chris Peterson [:cpeterson]]

(In reply to Jan Horak from comment #22)
> We cannot build on s390, s390x, the GETRANDOM_NR is missing on s390(x) -  I
> don't know what syscall numbers this architecture has. Attaching workaround
> patch for people who are interested.

Rust uses syscall number 349 for GETRANDOM on s390x:

https://github.com/rust-lang/rust/blob/3fc8304fd92720c688f3d6ac30b3a728d15f7a33/src/libstd/sys/unix/rand.rs#L58-L59

I see now that ARM and AArch64 use different syscall numbers, too. We should probably copy over all these GETRANDOM definitions.