Bug 148439 – cryptographicallyRandomValuesFromOS should use CCRandomCopyBytes when available.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Keith Miller]

Recently, we switched to using arc4random_buf on Darwin but further research indicates that arc4random_buf has the same behavior we had before and thus we were just pushing the problem further down the stack. CCRandomCopyBytes, however, appears to be more advanced and has much better error handling than we had before. Additionally, to make any future debugging easier I added a CRASH_WITH_MESSAGE to Assertions that adds a debug trace to crash logs with the CCRandomCopyBytes error message.

[Keith Miller]

Created attachment 259869 [details]
Patch

[Keith Miller]

Created attachment 259873 [details]
Patch

Should have pulled before formatting the patch...

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/OSRandomSource.cpp:70:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 3 files


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

[Keith Miller]

Created attachment 259874 [details]
Patch

Didn't unmangle well enough.

[Keith Miller]

Created attachment 259877 [details]
Patch

Removed debugging code that is now dead.

[Keith Miller]

Created attachment 259878 [details]
Patch

[Keith Miller]

(In reply to comment #6)
> Created attachment 259878 [details]
> Patch

Tried to manually delete whitespace changes... it was a bad idea.

[Geoffrey Garen]

Wpointer-arith -Wundef -Wwrite-strings -fPIC -MMD -MT Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o -MF Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o.d -o Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o -c ../../Source/WTF/wtf/Atomics.cpp
In file included from ../../Source/WTF/wtf/StdLibExtras.h:32:0,
                 from ../../Source/WTF/wtf/FastMalloc.h:26,
                 from ../../Source/WTF/config.h:51,
                 from ../../Source/WTF/wtf/Atomics.cpp:59:
../../Source/WTF/wtf/Assertions.h:179:5: error: "HAVE_OS_TRACE" is not defined [-Werror=undef]
 #if HAVE_OS_TRACE

[Geoffrey Garen]

Comment on attachment 259878 [details]
Patch

EWS build failed.

[Keith Miller]

Created attachment 259880 [details]
Patch

Added defines for 0 case also.

[Daniel Bates]

Comment on attachment 259880 [details]
Patch

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

> Source/WTF/wtf/Assertions.h:55
> +#if defined(__has_include)
> +#if __has_include(<os/trace.h>)
> +#define HAVE_OS_TRACE 1
> +#include <os/trace.h>
> +#else
> +#define HAVE_OS_TRACE 0
> +#endif
> +#endif

Would it make sense to move the define for HAVE_OS_TRACE to wtf/Platform.h? Notice that wtf/Platform.h defines HAVE_OS_ACTIVITY, which we use to guard inclusion and data types defined in header os/activity.h.

[Alexey Proskuryakov]

Comment on attachment 259880 [details]
Patch

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

> Source/WTF/wtf/Assertions.h:51
> +#include <os/trace.h>

is this what breaks iOS build?

/Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/spi/darwin/XPCSPI.h:37:1: error: typedef redefinition with different types ('NSObject<OS_xpc_object> *' vs 'void *')
OS_OBJECT_DECL(xpc_object);
^

> Source/WTF/wtf/OSRandomSource.cpp:33
> +#if __has_include(<CommonCrypto/CommonRandomSPI.h>)

This check will fail for open source WebKit builds (including nightlies), because CommonRandomSPI.h is an internal header.

In WebCore, we declare it in crypto/CommonCryptoUtilities.h - or you could add a CommonCryptoSPI header in WTF.

> Source/WTF/wtf/OSRandomSource.cpp:72
> +    if (result != kCCSuccess)
> +        CRASH_WITH_MESSAGE("crytographicallyRandomValuesFromOS could not get cryptographically random information from CCRandomCopyBytes and had error: %d", result);

I'm not sure how this is more helpful than a simple RELEASE_ASSERT. We can see where that one happens, and CCRandomCopyBytes can't fail anyway.

[Keith Miller]

Comment on attachment 259880 [details]
Patch

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

>> Source/WTF/wtf/Assertions.h:51
>> +#include <os/trace.h>
> 
> is this what breaks iOS build?
> 
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/spi/darwin/XPCSPI.h:37:1: error: typedef redefinition with different types ('NSObject<OS_xpc_object> *' vs 'void *')
> OS_OBJECT_DECL(xpc_object);
> ^

It looks like it. There might be a work around however.

>> Source/WTF/wtf/Assertions.h:55
>> +#endif
> 
> Would it make sense to move the define for HAVE_OS_TRACE to wtf/Platform.h? Notice that wtf/Platform.h defines HAVE_OS_ACTIVITY, which we use to guard inclusion and data types defined in header os/activity.h.

I'm not sure, It doesn't look like anyone else in the webkit uses os_trace as far as I can tell. I'm not opposed to this idea.

>> Source/WTF/wtf/OSRandomSource.cpp:33
>> +#if __has_include(<CommonCrypto/CommonRandomSPI.h>)
> 
> This check will fail for open source WebKit builds (including nightlies), because CommonRandomSPI.h is an internal header.
> 
> In WebCore, we declare it in crypto/CommonCryptoUtilities.h - or you could add a CommonCryptoSPI header in WTF.

I see, do you know of a reasonable upper bound on when CommonCrypto will be available? I would use the ENABLE(SUBTLE_CRYTPO) that is used in WebCore but it is not set in WTF and I'm not sure what conditions cause it to be set.

>> Source/WTF/wtf/OSRandomSource.cpp:72
>> +        CRASH_WITH_MESSAGE("crytographicallyRandomValuesFromOS could not get cryptographically random information from CCRandomCopyBytes and had error: %d", result);
> 
> I'm not sure how this is more helpful than a simple RELEASE_ASSERT. We can see where that one happens, and CCRandomCopyBytes can't fail anyway.

It's helpful in that if/when CCRandomCopyBytes fails the crash log will tell us the exact reason why it failed. Perhaps you are right, the documentation only states that CCRandomCopyBytes returns kCCSuccess if it succeeds but it doesn't say what it will do if it fails. Regardless, if in the future CCRandomCopyBytes can fail I would rather avoid a potential security risk and crash.

[Alexey Proskuryakov]

> I see, do you know of a reasonable upper bound on when CommonCrypto will be available?

CCRandomCopyBytes is available on all OS X and iOS versions that we build for. CommonCrypto/CommonRandomSPI.h is available on all OS X and iOS versions that we build for, but only for internal builds.

I would just keep using #if PLATFORM(DARWIN).

> Regardless, if in the future CCRandomCopyBytes can fail I would rather avoid a potential security risk and crash.

Yes, we should have a RELEASE_ASSERT here. Adding as much code as you did just for this case doesn't seem worth it to me.

If you still think that a more detailed message would be useful here, please send an e-mail to webkit-dev, so that everyone could discuss the best way to do this. Personally, I would do RELEASE_ASSERT_WITH_MESSAGE, so that we wouldn't need two lines of code at call sites.

[Keith Miller]

> > Regardless, if in the future CCRandomCopyBytes can fail I would rather avoid a potential security risk and crash.
> 
> Yes, we should have a RELEASE_ASSERT here. Adding as much code as you did
> just for this case doesn't seem worth it to me.
> 
> If you still think that a more detailed message would be useful here, please
> send an e-mail to webkit-dev, so that everyone could discuss the best way to
> do this. Personally, I would do RELEASE_ASSERT_WITH_MESSAGE, so that we
> wouldn't need two lines of code at call sites.

Based on conversations I have had with others it seems like having the ability to add debug information to crash logs seems desired. However, I can create an email chain to figure out the exact functionality/implementation we want. It is also unfortunate that currently os_trace does not allow non-scalar format strings so perhaps we should wait until that is added before moving forward.

[Keith Miller]

Created attachment 259970 [details]
Patch

[Alexey Proskuryakov]

Keith, did you intend to mark the latest patch for review?

[Alexey Proskuryakov]

Comment on attachment 259970 [details]
Patch

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

> Source/WTF/wtf/OSRandomSource.cpp:43
> +#if OS(DARWIN)

This is not the way we've been doing this recently. A better way to add forward declarations is to put them into a separate SPI header, as that lets us reuse the code, and to reduce the noise at the top of .cpp files.

[Keith Miller]

rdar://problem/21939126

[Keith Miller]

> This is not the way we've been doing this recently. A better way to add
> forward declarations is to put them into a separate SPI header, as that lets
> us reuse the code, and to reduce the noise at the top of .cpp files.

I was thinking that it seemed unlikely anyone would use these in the future so it seemed like file bloat to add an SPI for just one function. If anyone else did want access to the forward declarations then I would agree that creating an SPI file makes sense.

[WebKit Commit Bot]

Comment on attachment 259970 [details]
Patch

Rejecting attachment 259970 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 259970, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WTF/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/140765

[Keith Miller]

Created attachment 260648 [details]
Patch

[Keith Miller]

I guess I somehow deleted the reviewer field in the ChangeLog so Commit-Queue got confused.

[Darin Adler]

Comment on attachment 260648 [details]
Patch

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

> Source/WTF/wtf/OSRandomSource.cpp:47
> +#if OS(DARWIN)
> +typedef struct __CCRandom* CCRandomRef;
> +extern const CCRandomRef kCCRandomDefault;
> +extern "C" int CCRandomCopyBytes(CCRandomRef rnd, void *bytes, size_t count);
> +#endif

This is not the correct idiom for new uses of SPI in WebKit. See examples like WTF/wtf/spi/darwin/XPCSPI.h and WTF/wtf/spi/cocoa/NSMapTableSPI.h for the way we now do it.

[Keith Miller]

Created attachment 261011 [details]
Patch

[Keith Miller]

Created attachment 261013 [details]
Patch for landing

[Keith Miller]

Created attachment 261015 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 261015 [details]
Patch for landing

Rejecting attachment 261015 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 261015, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
a/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 54, in run
    if changelog_entry.has_valid_reviewer():
AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer'

Full output: http://webkit-queues.webkit.org/results/160990

[Keith Miller]

Committed r189633: <http://trac.webkit.org/changeset/189633>