[LLVMdev] Adding diversity for security (and testing)

[Stephen Crane]

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140123/8f51910f/attachment.html>