[LLVMdev] Adding diversity for security (and testing)

[Geremy Condra]

+jfb for completeness


On Thu, Jan 23, 2014 at 5:50 PM, Stephen Crane <sjcrane at uci.edu> wrote:

> Hi all,
>
> For anyone who's interested in this work, we have a pending patch
> discussion on the llvm-commits list:
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140120/202449.htmlWe'd really appreciate input from anyone interested, especially everyone
> involved in this discussion thread.
>
> Thanks,
> Stephen
>
>
> On Fri, Sep 20, 2013 at 2:36 PM, Stephen Crane <sjcrane at uci.edu> wrote:
>
>> 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
>>
>>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140127/0e780b9b/attachment.html>