Bug 170095

Summary: [WTF] Introduce WTF::RandomDevice which keeps /dev/urandom opened
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: Web Template FrameworkAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, fpizlo, jfbastien, jiewen_tan, mcatanzaro, sam, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=169623
https://bugs.webkit.org/show_bug.cgi?id=170071
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch mcatanzaro: review+

Description Yusuke Suzuki 2017-03-25 00:25:04 PDT
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
Comment 1 Yusuke Suzuki 2017-03-25 00:39:16 PDT
It's similar to std::random_device. But we can select the implementation, avoid arc4random.
Comment 2 JF Bastien 2017-03-25 01:08:02 PDT
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?
Comment 3 Yusuke Suzuki 2017-03-25 01:47:56 PDT
(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. :)
Comment 4 Yusuke Suzuki 2017-03-25 02:06:07 PDT
(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.
Comment 5 Yusuke Suzuki 2017-03-25 04:02:04 PDT
Created attachment 305374 [details]
Patch
Comment 6 Build Bot 2017-03-25 04:04:41 PDT
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.
Comment 7 JF Bastien 2017-03-25 13:04:34 PDT
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?
Comment 8 Jiewen Tan 2017-03-27 11:42:32 PDT
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.
Comment 9 Michael Catanzaro 2017-03-27 13:49:45 PDT
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.
Comment 10 JF Bastien 2017-03-27 13:56:38 PDT
(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.
Comment 11 Yusuke Suzuki 2017-03-28 12:28:11 PDT
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.
Comment 12 Yusuke Suzuki 2017-03-28 12:28:23 PDT
*** Bug 170096 has been marked as a duplicate of this bug. ***
Comment 13 Yusuke Suzuki 2017-03-28 12:51:39 PDT
Created attachment 305622 [details]
Patch
Comment 14 Yusuke Suzuki 2017-03-28 12:53:28 PDT
Created attachment 305623 [details]
Patch
Comment 15 Build Bot 2017-03-28 12:54:22 PDT
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.
Comment 16 Yusuke Suzuki 2017-03-28 12:55:11 PDT
Created attachment 305624 [details]
Patch
Comment 17 Build Bot 2017-03-28 12:57:11 PDT
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.
Comment 18 JF Bastien 2017-03-28 13:02:23 PDT
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?
Comment 19 Yusuke Suzuki 2017-03-28 13:11:51 PDT
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.
Comment 20 Yusuke Suzuki 2017-03-28 13:19:32 PDT
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.
Comment 21 Yusuke Suzuki 2017-03-28 13:20:59 PDT
Created attachment 305633 [details]
Patch
Comment 22 Build Bot 2017-03-28 13:22:23 PDT
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.
Comment 23 Yusuke Suzuki 2017-03-28 14:34:48 PDT
Created attachment 305645 [details]
Patch
Comment 24 Build Bot 2017-03-28 14:36:04 PDT
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.
Comment 25 Michael Catanzaro 2017-03-28 19:46:46 PDT
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?
Comment 26 Yusuke Suzuki 2017-04-03 00:24:31 PDT
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.
Comment 27 Yusuke Suzuki 2017-04-03 00:36:13 PDT
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.
Comment 28 Yusuke Suzuki 2017-04-03 00:38:52 PDT
Created attachment 306066 [details]
Patch
Comment 29 Build Bot 2017-04-03 00:40:58 PDT
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.
Comment 30 Michael Catanzaro 2017-04-03 07:34:13 PDT
(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.
Comment 31 JF Bastien 2017-04-03 09:39:12 PDT
(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.
Comment 32 Michael Catanzaro 2017-04-03 11:00:11 PDT
(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.
Comment 33 JF Bastien 2017-04-03 11:06:54 PDT
(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/.
Comment 34 Yusuke Suzuki 2017-04-03 22:39:46 PDT
(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.
Comment 35 JF Bastien 2017-04-03 22:50:50 PDT
> 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!
Comment 36 Yusuke Suzuki 2017-04-04 00:04:44 PDT
Committed r214867: <http://trac.webkit.org/changeset/214867>