This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Add a function to gather random bytes
ClosedPublic

Authored by cryptoad on Jun 20 2017, 11:27 AM.

Details

Summary

AFAICT compiler-rt doesn't have a function that would return 'good' random
bytes to seed a PRNG. Currently, the SizeClassAllocator64 uses addresses
returned by mmap to seed its PRNG, which is not ideal, and
SizeClassAllocator32 doesn't benefit from the entropy offered by its 64-bit
counterpart address space, so right now it has nothing. This function aims at
solving this, allowing to implement good 32-bit chunk randomization. Scudo also
has a function that does this for Cookie purposes, which would go away in a
later CL once this lands.

This function will try the getrandom syscall if available, and fallback to
/dev/urandom if not.

Unfortunately, I do not have a way to implement and test a Mac and Windows
version, so those are unimplemented as of now. Note that kRandomShuffleChunks
is only used on Linux for now.

Event Timeline

cryptoad created this revision.Jun 20 2017, 11:27 AM

Assuming sanitizer windows doesn't mind to take a dependency on advapi32.dll (maybe it already has one?) this can be implemented pretty trivially using CryptGenRandom or RtlGenRandom (latter is easier to use).

In any case, shouldn't the unimplemented versions at least do *something*, even if they are not cryptographically strong?

cryptoad added a comment.EditedJun 20 2017, 12:09 PM

Assuming sanitizer windows doesn't mind to take a dependency on advapi32.dll (maybe it already has one?) this can be implemented pretty trivially using CryptGenRandom or RtlGenRandom (latter is easier to use).

Looks like this is what is needed indeed. I can't speak for what's going on with Sanitizer Windows though.

In any case, shouldn't the unimplemented versions at least do *something*, even if they are not cryptographically strong?

I am not in disagreement. The reasoning behind leaving them unimplemented is twofold:

  • without a dev platform (and experience on the platform) for either, I am not comfortable writing something and debugging through the bots;
  • it might lead to false expectation as to what the function is doing.

If that can be done in parts, this one first, then the other platforms by people with experience, that help solve this.
Or if you have an idea that can work for a simple initialization that is bound to work, I'll gladly take it.

rnk added a comment.Jun 20 2017, 3:28 PM

Assuming sanitizer windows doesn't mind to take a dependency on advapi32.dll (maybe it already has one?) this can be implemented pretty trivially using CryptGenRandom or RtlGenRandom (latter is easier to use).

We can definitely do that. We probably already link against it.

In any case, shouldn't the unimplemented versions at least do *something*, even if they are not cryptographically strong?

The use case for this random data is more ASLR. We can definitely add this API on Windows now, but scudo is only supported on Linux for now, so there isn't much reason to do it or to block this patch for Windows or Mac. I support landing this as is.

alekseyshl edited edge metadata.Jun 20 2017, 3:54 PM

I think it's fine to leave them unimplemented on win/mac and let someone with access to the actual platform to add and test the code.

lib/sanitizer_common/sanitizer_common.h
924

Why u8*? Why not customary void*?

lib/sanitizer_common/sanitizer_mac.cc
928

Maybe UNIMPLEMENTED(); is slightly more robust than return false?

lib/sanitizer_common/tests/sanitizer_common_test.cc
316

Please use ARRAY_SIZE(buffer_*) instead of literal 32.

cryptoad updated this revision to Diff 103285.Jun 20 2017, 4:07 PM
cryptoad marked 3 inline comments as done.

Addressing review comments.

alekseyshl accepted this revision.Jun 20 2017, 6:07 PM
This revision is now accepted and ready to land.Jun 20 2017, 6:07 PM
cryptoad closed this revision.Jun 21 2017, 8:56 AM