Bug 170095 – [WTF] Introduce WTF::RandomDevice which keeps /dev/urandom opened

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Currently, everytime cryptographicallyRandomValuesFromOS is called, /dev/urandom is opened and closed.
In addition to unnecessary system calls, especially open is not so scalable due to its POSIX requirement returning the lowest file descriptor number[1].

Instead, we introduce a new class RandomDevice. It keeps `/dev/urandom` file opened in its member.
In the subsequent patch, it may be useful that changing cryptographicallyRandomValuesFromOS to use static RandomDevice for the process.

[1]: http://dl.acm.org/citation.cfm?id=2522712

[Yusuke Suzuki]

It's similar to std::random_device. But we can select the implementation, avoid arc4random.

[JF Bastien]

Agreed that opening once is preferable. On MacOS doesn't libc++'s random_device end up using /dev/urandom? That seems to be the case from the code:
  https://github.com/llvm-mirror/libcxx/blob/master/src/random.cpp
A call_once would take care of this, no?

Why is avoiding arc4random preferable?

If we're doing our own random device, shouldn't we also consider SYS_getrandom if available on Linux, or getentropy on BSD?

[Yusuke Suzuki]

(In reply to JF Bastien from comment #2)
> Agreed that opening once is preferable. On MacOS doesn't libc++'s
> random_device end up using /dev/urandom? That seems to be the case from the
> code:
>   https://github.com/llvm-mirror/libcxx/blob/master/src/random.cpp

In macOS libc++, I think we end up using arc4random. (https://github.com/llvm-mirror/libcxx/blob/master/src/random.cpp#L53).
In WTF, currently, we use CCRandomCopyBytes instead of urandom in WTF::cryptographicallyRandomValuesFromOS.
Start using std::random_device inside cryptographicallyRandomValuesFromOS may have a chance to use arc4random things. We explicitly would like to avoid this case. So, we should have our own RandomDevice, which implementaiton would be carried from the existing cryptographicallyRandomValuesFromOS.

> A call_once would take care of this, no?

I think RandomDevice should open the urandom file.
And on the top of it, we can use call_once to guarantee that only one RandomDevice is created.

> Why is avoiding arc4random preferable?

Let's see https://trac.webkit.org/r214329.
While that patch is reverted due to significant Speedometer regression, we would like to keep cryptographicallyRandomValuesFromOS non-ARC4.

> If we're doing our own random device, shouldn't we also consider
> SYS_getrandom if available on Linux, or getentropy on BSD?

getrandom() sounds nice. And getentropy for BSD sound nice too.
And by introducing our own WTF::RandomDevice, we also keep Windows CSP handle acquired. :)

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #3)
> > If we're doing our own random device, shouldn't we also consider
> > SYS_getrandom if available on Linux, or getentropy on BSD?
> 
> getrandom() sounds nice. And getentropy for BSD sound nice too.
> And by introducing our own WTF::RandomDevice, we also keep Windows CSP
> handle acquired. :)

But in that case, we should be careful that Linux getrandom is only supported after 3.17 kernel.

[Yusuke Suzuki]

Created attachment 305374 [details]
Patch

[Build Bot]

Attachment 305374 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RandomDevice.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Comment on attachment 305374 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305374&action=review

> Source/WTF/wtf/RandomDevice.cpp:66
> +    m_fd = open("/dev/urandom", O_RDONLY, 0);

Technically this can receive EINTR. You should:

    do {
      m_fd = open("/dev/urandom", O_RDONLY, 0);
    } while (fd == -1 && errno == EINTR);

> Source/WTF/wtf/RandomDevice.cpp:85
> +    RELEASE_ASSERT(!CCRandomCopyBytes(kCCRandomDefault, buffer, length));

I was wondering if this was thread-safe. Answer is yes according to the header:
  https://opensource.apple.com/source/CommonCrypto/CommonCrypto-60049/include/CommonRandomSPI.h
Implementation:
  https://opensource.apple.com/source/CommonCrypto/CommonCrypto-60049/lib/CommonRandom.c

> Source/WTF/wtf/RandomDevice.h:2
> + * Copyright (C) Google, Inc.

Date?

> Source/WTF/wtf/RandomDevice.h:46
> +    // partially filled. Rather than calling this function directly, consider

Is the "partially filled" thing true? I though the code looped until it fills fully? How would one figure this out and remedy it?

[Jiewen Tan]

Comment on attachment 305374 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305374&action=review

>> Source/WTF/wtf/RandomDevice.h:46
>> +    // partially filled. Rather than calling this function directly, consider
> 
> Is the "partially filled" thing true? I though the code looped until it fills fully? How would one figure this out and remedy it?

Dived into the core crypto codes. It seems doing the same thing as Linux/Windows code paths. It loops until it fills fully. Otherwise, fails with error. Maybe we should change the description to:
The function loops to get the buffer filled. If any error occurs, it will crash.

[Michael Catanzaro]

The other thing we need in RandomDevice is a randomness cache (that somehow needs to be fork-safe so different web processes don't wind up with the same random numbers), see bug #169623.

[JF Bastien]

(In reply to Michael Catanzaro from comment #9)
> The other thing we need in RandomDevice is a randomness cache (that somehow
> needs to be fork-safe so different web processes don't wind up with the same
> random numbers), see bug #169623.

That bug isn't accessible.

[Yusuke Suzuki]

Comment on attachment 305374 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305374&action=review

Thanks!

>> Source/WTF/wtf/RandomDevice.cpp:66
>> +    m_fd = open("/dev/urandom", O_RDONLY, 0);
> 
> Technically this can receive EINTR. You should:
> 
>     do {
>       m_fd = open("/dev/urandom", O_RDONLY, 0);
>     } while (fd == -1 && errno == EINTR);

Right! Fixed.

>> Source/WTF/wtf/RandomDevice.cpp:85
>> +    RELEASE_ASSERT(!CCRandomCopyBytes(kCCRandomDefault, buffer, length));
> 
> I was wondering if this was thread-safe. Answer is yes according to the header:
>   https://opensource.apple.com/source/CommonCrypto/CommonCrypto-60049/include/CommonRandomSPI.h
> Implementation:
>   https://opensource.apple.com/source/CommonCrypto/CommonCrypto-60049/lib/CommonRandom.c

Cool, great.

>> Source/WTF/wtf/RandomDevice.h:2
>> + * Copyright (C) Google, Inc.
> 
> Date?

Fixed.

>>> Source/WTF/wtf/RandomDevice.h:46
>>> +    // partially filled. Rather than calling this function directly, consider
>> 
>> Is the "partially filled" thing true? I though the code looped until it fills fully? How would one figure this out and remedy it?
> 
> Dived into the core crypto codes. It seems doing the same thing as Linux/Windows code paths. It loops until it fills fully. Otherwise, fails with error. Maybe we should change the description to:
> The function loops to get the buffer filled. If any error occurs, it will crash.

OK, I'll fix this comment.

[Yusuke Suzuki]

*** Bug 170096 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Created attachment 305622 [details]
Patch

[Yusuke Suzuki]

Created attachment 305623 [details]
Patch

[Build Bot]

Attachment 305623 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RandomDevice.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 305624 [details]
Patch

[Build Bot]

Attachment 305624 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RandomDevice.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Comment on attachment 305624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305624&action=review

> Source/WTF/wtf/RandomDevice.cpp:104
> +        ssize_t currentRead = syscall(SYS_getrandom, buffer, length, 0);

It would be easier to read if Linux were separate from Unix IMO. I kinda almost want each OS to be separate in this file, the interleaving is confusing :)

Is SYS_getrandom always defined for the versions we care about?

Also, don't you need to advance buffer and subtract from length?

[Yusuke Suzuki]

Comment on attachment 305624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305624&action=review

>> Source/WTF/wtf/RandomDevice.cpp:104
>> +        ssize_t currentRead = syscall(SYS_getrandom, buffer, length, 0);
> 
> It would be easier to read if Linux were separate from Unix IMO. I kinda almost want each OS to be separate in this file, the interleaving is confusing :)
> 
> Is SYS_getrandom always defined for the versions we care about?
> 
> Also, don't you need to advance buffer and subtract from length?

According to the issue https://bugs.webkit.org/show_bug.cgi?id=170096, the Linux versions we care must support getrandom.
Oops, we should do the same thing done in read(). Fixed.
Personally, I like the current one file thing since only this function is significantly different. And difference is small enough I think.

[Yusuke Suzuki]

Comment on attachment 305624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305624&action=review

>>> Source/WTF/wtf/RandomDevice.cpp:104
>>> +        ssize_t currentRead = syscall(SYS_getrandom, buffer, length, 0);
>> 
>> It would be easier to read if Linux were separate from Unix IMO. I kinda almost want each OS to be separate in this file, the interleaving is confusing :)
>> 
>> Is SYS_getrandom always defined for the versions we care about?
>> 
>> Also, don't you need to advance buffer and subtract from length?
> 
> According to the issue https://bugs.webkit.org/show_bug.cgi?id=170096, the Linux versions we care must support getrandom.
> Oops, we should do the same thing done in read(). Fixed.
> Personally, I like the current one file thing since only this function is significantly different. And difference is small enough I think.

Hmm, buildbot is sad. OK, my intension in this patch is avoiding frequent file open/close.
So, let's just use UNIX implementation for Linux right now.

[Yusuke Suzuki]

Created attachment 305633 [details]
Patch

[Build Bot]

Attachment 305633 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RandomDevice.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 305645 [details]
Patch

[Build Bot]

Attachment 305645 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RandomDevice.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 305645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305645&action=review

> Source/WTF/wtf/OSRandomSource.cpp:37
> +    static NeverDestroyed<RandomDevice> device;
> +    device.get().cryptographicallyRandomValues(buffer, length);

Do we need a mutex here?

> Source/WTF/wtf/RandomDevice.cpp:65
> +#if !OS(DARWIN) && !OS(WINDOWS)
> +RandomDevice::RandomDevice()
> +{
> +#if OS(UNIX)

Why two layers of #ifs? Why not either surround the entire constructor with #if OS(UNIX), or simply remove the !OS(DARWIN) && !OS(WINDOWS) guards?

> Source/WTF/wtf/RandomDevice.cpp:106
> +    // FIXME: We cannot ensure that Cryptographic Service Provider context and CryptGenRandom are safe across threads.
> +    // If it is safe, we can acquire context per RandomDevice.

Do we need a mutex here, too?

[Yusuke Suzuki]

Comment on attachment 305645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305645&action=review

Thanks. I'll update the patch.

>> Source/WTF/wtf/OSRandomSource.cpp:37
>> +    device.get().cryptographicallyRandomValues(buffer, length);
> 
> Do we need a mutex here?

We don't need a mutex. Each cryptographicallyRandomValues function call is thread-safe. And the creation of RandomDevice is guarded by C++11 static semantics. (emitting mutex).

>> Source/WTF/wtf/RandomDevice.cpp:65
>> +#if OS(UNIX)
> 
> Why two layers of #ifs? Why not either surround the entire constructor with #if OS(UNIX), or simply remove the !OS(DARWIN) && !OS(WINDOWS) guards?

I planned to have OS(WINDOWS) version here which holds CSP. But I cannot ensure this is safe across threads. So we use the original implementation that acquires CSP each time.
So, we can just surround the constructor with (OS(UNIX) && !(OS(DARWIN))) guard (note that darwin is also unix. so OS(DARWIN) guard is also necessary).

>> Source/WTF/wtf/RandomDevice.cpp:106
>> +    // If it is safe, we can acquire context per RandomDevice.
> 
> Do we need a mutex here, too?

In Windows version, each time we acquire cryptographic service provider. So, this function itself does not share any information.
So, we do not need mutex here.

[Yusuke Suzuki]

Comment on attachment 305645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305645&action=review

>>> Source/WTF/wtf/RandomDevice.cpp:65
>>> +#if OS(UNIX)
>> 
>> Why two layers of #ifs? Why not either surround the entire constructor with #if OS(UNIX), or simply remove the !OS(DARWIN) && !OS(WINDOWS) guards?
> 
> I planned to have OS(WINDOWS) version here which holds CSP. But I cannot ensure this is safe across threads. So we use the original implementation that acquires CSP each time.
> So, we can just surround the constructor with (OS(UNIX) && !(OS(DARWIN))) guard (note that darwin is also unix. so OS(DARWIN) guard is also necessary).

Or, just using the current outer ifdef is fine.

[Yusuke Suzuki]

Created attachment 306066 [details]
Patch

[Build Bot]

Attachment 306066 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RandomDevice.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

(In reply to Yusuke Suzuki from comment #26) 
> We don't need a mutex. Each cryptographicallyRandomValues function call is
> thread-safe. And the creation of RandomDevice is guarded by C++11 static
> semantics. (emitting mutex).

This is news to me, do you have a reference for this? It's hard to search for online.

[JF Bastien]

(In reply to Michael Catanzaro from comment #30)
> (In reply to Yusuke Suzuki from comment #26) 
> > We don't need a mutex. Each cryptographicallyRandomValues function call is
> > thread-safe. And the creation of RandomDevice is guarded by C++11 static
> > semantics. (emitting mutex).
> 
> This is news to me, do you have a reference for this? It's hard to search
> for online.

It was added in C++11, some compilers used to do it anyways, but we can't rely on it because we compile with fno-threadsafe-statics. We should use once_flag instead.

[Michael Catanzaro]

(In reply to JF Bastien from comment #31)
> It was added in C++11, some compilers used to do it anyways, but we can't
> rely on it because we compile with fno-threadsafe-statics. We should use
> once_flag instead.

Interesting! Threadsafe statics seem desirable. Does dropping -fno-threadsafe-statics cause a Speedometer regression?

And are you sure we actually use this flag outside libwebrtc? I grepped for it in Source/ and all the results were under ThirdParty/libwebrtc.

[JF Bastien]

(In reply to Michael Catanzaro from comment #32)
> (In reply to JF Bastien from comment #31)
> > It was added in C++11, some compilers used to do it anyways, but we can't
> > rely on it because we compile with fno-threadsafe-statics. We should use
> > once_flag instead.
> 
> Interesting! Threadsafe statics seem desirable. Does dropping
> -fno-threadsafe-statics cause a Speedometer regression?

Unknown, but...

> And are you sure we actually use this flag outside libwebrtc? I grepped for
> it in Source/ and all the results were under ThirdParty/libwebrtc.

I'm not sure what the full history is. I think on Chrome the issue was with older MSVC versions. I don't know if this impacts WebKit. If we still support building with compilers where static init isn't thread-safe then we can't move away.

However, if supported compilers are all OK then we could move away, assuming it's not a regression as you suggest. There are roughly 80 once_flag uses in Source/.

[Yusuke Suzuki]

(In reply to JF Bastien from comment #33)
> Unknown, but...
> 
> > And are you sure we actually use this flag outside libwebrtc? I grepped for
> > it in Source/ and all the results were under ThirdParty/libwebrtc.
> 
> I'm not sure what the full history is. I think on Chrome the issue was with
> older MSVC versions. I don't know if this impacts WebKit. If we still
> support building with compilers where static init isn't thread-safe then we
> can't move away.
> 
> However, if supported compilers are all OK then we could move away, assuming
> it's not a regression as you suggest. There are roughly 80 once_flag uses in
> Source/.

I think the current WebKit tree does not use no-threadsafe-statics. "no-threadsafe-statics" flag is only shown under the ThirdParty/libwebrtc thing, right?
BTW, I think we already relied this C++11 static semantics in various places. For example, RunLoop::current() in wtf/RunLoop.cpp uses `static NeverDestroyed<ThreadSpecific<Holder>>`. If this C++11 semantics is broken, it becomes racy.

[JF Bastien]

> I think the current WebKit tree does not use no-threadsafe-statics.
> "no-threadsafe-statics" flag is only shown under the ThirdParty/libwebrtc
> thing, right?
> BTW, I think we already relied this C++11 static semantics in various
> places. For example, RunLoop::current() in wtf/RunLoop.cpp uses `static
> NeverDestroyed<ThreadSpecific<Holder>>`. If this C++11 semantics is broken,
> it becomes racy.

OK cool!

[Yusuke Suzuki]

Committed r214867: <http://trac.webkit.org/changeset/214867>