Bug 148439 - cryptographicallyRandomValuesFromOS should use CCRandomCopyBytes when available.
Summary: cryptographicallyRandomValuesFromOS should use CCRandomCopyBytes when available.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-08-25 11:56 PDT by Keith Miller
Modified: 2015-09-11 14:34 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.91 KB, patch)
2015-08-25 12:06 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (3.91 KB, patch)
2015-08-25 13:27 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (3.90 KB, patch)
2015-08-25 13:33 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (3.83 KB, patch)
2015-08-25 14:07 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (3.81 KB, patch)
2015-08-25 14:14 PDT, Keith Miller
ggaren: review-
Details | Formatted Diff | Diff
Patch (3.88 KB, patch)
2015-08-25 14:26 PDT, Keith Miller
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
Patch (2.20 KB, patch)
2015-08-26 12:42 PDT, Keith Miller
ap: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (2.24 KB, patch)
2015-09-04 16:30 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (7.26 KB, patch)
2015-09-11 13:15 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (7.27 KB, patch)
2015-09-11 14:21 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (7.39 KB, patch)
2015-09-11 14:25 PDT, Keith Miller
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2015-08-25 11:56:48 PDT
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.
Comment 1 Keith Miller 2015-08-25 12:06:32 PDT
Created attachment 259869 [details]
Patch
Comment 2 Keith Miller 2015-08-25 13:27:52 PDT
Created attachment 259873 [details]
Patch

Should have pulled before formatting the patch...
Comment 3 WebKit Commit Bot 2015-08-25 13:29:33 PDT
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.
Comment 4 Keith Miller 2015-08-25 13:33:00 PDT
Created attachment 259874 [details]
Patch

Didn't unmangle well enough.
Comment 5 Keith Miller 2015-08-25 14:07:36 PDT
Created attachment 259877 [details]
Patch

Removed debugging code that is now dead.
Comment 6 Keith Miller 2015-08-25 14:14:00 PDT
Created attachment 259878 [details]
Patch
Comment 7 Keith Miller 2015-08-25 14:14:46 PDT
(In reply to comment #6)
> Created attachment 259878 [details]
> Patch

Tried to manually delete whitespace changes... it was a bad idea.
Comment 8 Geoffrey Garen 2015-08-25 14:21:33 PDT
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
Comment 9 Geoffrey Garen 2015-08-25 14:21:56 PDT
Comment on attachment 259878 [details]
Patch

EWS build failed.
Comment 10 Keith Miller 2015-08-25 14:26:57 PDT
Created attachment 259880 [details]
Patch

Added defines for 0 case also.
Comment 11 Daniel Bates 2015-08-25 15:41:41 PDT
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.
Comment 12 Alexey Proskuryakov 2015-08-25 16:12:42 PDT
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.
Comment 13 Keith Miller 2015-08-26 10:10:17 PDT
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.
Comment 14 Alexey Proskuryakov 2015-08-26 10:21:11 PDT
> 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.
Comment 15 Keith Miller 2015-08-26 11:14:01 PDT
> > 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.
Comment 16 Keith Miller 2015-08-26 12:42:26 PDT
Created attachment 259970 [details]
Patch
Comment 17 Alexey Proskuryakov 2015-08-27 20:13:20 PDT
Keith, did you intend to mark the latest patch for review?
Comment 18 Alexey Proskuryakov 2015-09-04 13:15:32 PDT
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.
Comment 19 Keith Miller 2015-09-04 13:35:07 PDT
rdar://problem/21939126
Comment 20 Keith Miller 2015-09-04 13:38:54 PDT
> 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.
Comment 21 WebKit Commit Bot 2015-09-04 14:00:34 PDT
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
Comment 22 Keith Miller 2015-09-04 16:30:12 PDT
Created attachment 260648 [details]
Patch
Comment 23 Keith Miller 2015-09-04 16:30:59 PDT
I guess I somehow deleted the reviewer field in the ChangeLog so Commit-Queue got confused.
Comment 24 Darin Adler 2015-09-11 09:06:47 PDT
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.
Comment 25 Keith Miller 2015-09-11 13:15:55 PDT
Created attachment 261011 [details]
Patch
Comment 26 Keith Miller 2015-09-11 14:21:13 PDT
Created attachment 261013 [details]
Patch for landing
Comment 27 Keith Miller 2015-09-11 14:25:06 PDT
Created attachment 261015 [details]
Patch for landing
Comment 28 WebKit Commit Bot 2015-09-11 14:27:17 PDT
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
Comment 29 Keith Miller 2015-09-11 14:34:32 PDT
Committed r189633: <http://trac.webkit.org/changeset/189633>