[Webkit-unassigned] [Bug 148439] cryptographicallyRandomValuesFromOS should use CCRandomCopyBytes when available.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=148439

--- Comment #13 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 259880
  --> https://bugs.webkit.org/attachment.cgi?id=259880
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150826/bbf42e98/attachment.html>