[LLVMdev] Adding diversity for security (and testing)

[Stephen Crane]

Nick,

Thanks so much for such a detailed review. I definitely missed a few of the details of the LLVM standards. Sorry. Here's a new patch that should resolve the issues you pointed out. I've also included a few comments below -- anything I haven't replied to has been fixed.

In particular, I'd like to discuss RNG seeding with the list. I currently use a static singleton to make sure there is only a single RNG ever instantiated, but this is obviously undesirable. Is there a better place where we can initialize the RNG in a thread-safe manner? We really need an RNG for each thread in that case, so that the results are consistent, regardless of thread ordering. I'm not familiar enough with LLVM to know if such a place even exists.


On Sep 20, 2013, at 00:52 , Nick Lewycky <nicholas at mxc.ca> wrote:
> Awesome! I'll jump right in to reviewing. Note that my reviews often focus on low-level stuff (formatting, typos). I understand that these patches may not be ready for that, and rather expect a higher-level review, but I'll do what I do anyways.

Works for me! You're completely right that I was initially expecting some higher-level feedback, but this is great.

> You have separate LLVM_WITH_OPENSSL and LLVM_ENABLE_RNG settings. Why? Either LLVM_ENABLE_RNG should always be on and then people can choose to link in openssl or not. Or maybe you're worried that this is insecure and you should remove LLVM_ENABLE_RNG and make it driven solely on LLVM_ENABLE_RNG? If you think we can put a good enough RNG into LLVM, we should go with turning on LLVM_ENABLE_RNG permanently, but if not then we should make the whole thing conditional on whether LLVM_WITH_OPENSSL is set.

I had initially included this because I expected resistance to adding an RNG at all, but it now appears that we're all on the same page that some RNG is required. I'll go ahead and remove the ENABLE_RNG defines.

> Are you familiar with std::random_device and the random number generator C++ 11 standard libary components? Yep, I'm going to ask you to expose a compatible API, or reasons why that doesn't work (or is silly). std::random_device is defined to be non-deterministic, and you needn't provide methods that you don't call (entropy()) but it would be convenient for everyone reading the API if it were a subset of an existing standard, or with clearly documented deviations from the standard.

I had not actually thought about using the std::random_device interface. If we were able to use C++11 features, this would be quite nice, since that includes builtin things like shuffle over RandomAccessIterators and such niceties. However, since this is not the case, I think complying to the <random> API (which is quite odd, there's not even an overarching interface) might be more complication than is necessary. We would have to emulate not only the generator, but also the distribution classes and the generic shuffle method. Even using operator() makes the client code particularly unreadable, since it currently uses a static Generator() getter to get the instance of the RNG. I'd like to resolve this first, then we can look at whether using the operator() interface makes sense.

> +void RandomNumberGenerator::ReadStateFile(StringRef StateFilename) {
> 
> There's nothing wrong with this function per se. It isn't buggy. I'm just wondering how this API fits with the general "llvm as a library" approach. LLVM doesn't generally bake in the assumption that it's running on a system with a filesystem. For example, the bitcode reader has a convenience method which reads a .bc file, sure, but that just does the action of loading it into a MemoryBuffer, and forwards it along to the API using MemoryBuffer that does the work. I think you should follow that pattern here too.

On thinking this over further, I think this feature is simply not needed any more. We had some trouble with getting 64-bit command-line options working, but since this works now, the state file really shouldn't be needed. Nuked.

> Overall this looks like a great start. However, I would like some other people to review things:
>  - I don't actually approve lib/CodeGen and lib/Target changes. Somebody else is going to have to think about whether "InsertedNOP" belongs as an MIFlag.

The reasoning behind making this an MIFlag was to be able to skip over NOPs in the terminators of a block. For uniform security we want to be able to place NOPs in the terminators, but still need to skip over them when finding the first terminator.

>  - I'm concerned about seeding the RNG. I'm especially concerned about seeding it in the LTO case, I hadn't thought of that until I saw it in the patch. I'm appreciate if somebody with a security background could ponder that one.
>  - There's also a clang patch which should be reviewed by cfe-commits.
> 
> Nick

-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm_diversity-0.2.diff
Type: application/octet-stream
Size: 49872 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130920/dd455f79/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang_diversity-0.2.diff
Type: application/octet-stream
Size: 7007 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130920/dd455f79/attachment-0001.obj>