[Owasp-esapi-c++] ESAPI and Secure storage, allocation, and deletion

[Kevin W. Wall]

Jeff,

Good thoughtful answers. Not sure I will have much time to respond
before Sun night
in any amount of details.

BinStr and SecBinStr sound as though they probably would be useful in
their own right,
but to me this almost seems like a shell game. Isn't BinStr and/or
SecBinStr going
to have to have CTORs that take byte* and size_t args? If so, aren't we just
moving this problem one level up.

Maybe if you could outline the interface for SecBinStr (no implmentation
needed at this point) along w/ some minimal comments, I could follow
along more.  But I do have a lot more to say about this. Much of it
I agree with and some I disagree with.

Unfortunately, between soccer, guests, and other obligations, I won't
have much time until late Sun evening.  And even then, I will need
to focus on finishing my ESAPI presentation which I give on 8/18
and am still only 1/2 way done.

-kevin

On Thu, Aug 11, 2011 at 12:52 AM, Jeffrey Walton <noloader at gmail.com> wrote:
> On Wed, Aug 10, 2011 at 11:59 PM, Kevin W. Wall <kevin.w.wall at gmail.com> wrote:
>> On Wed, Aug 10, 2011 at 9:50 PM, Jeffrey Walton <noloader at gmail.com> wrote:
>> [Snip URLs]
>>> ********************
>>>
>>>  The C++ dialect of Java's constructors are:
>>>
>>>    // Create an instance PRNG with a seed.
>>>    explicit SecureRandom(const byte* seed, size_t size);
>>>
>>>    // Create an instance PRNG with a seed.
>>>    explicit SecureRandom(const std::vector<byte>& seed);
>>>
>>> Neither method accepts a container with secure storage. I believe we
>>> need the byte* variant for accepting 'raw' data from users. But the
>>> safe version using a vector does not provide a secure allocation which
>>> zeroizes on deletion. So when the user's vector is destroyed, secret
>>> values might linger in memory. Its not secure, and not FIPS compliant
>>> (zeorization must occur even at FIPS level 1).
>>
>> First off, the most common use of SecureRandom's CTOR in Java is
>> the default no-args CTOR, which generates its on seed. On systems where
>> /dev/random and /dev/urandom is available, it will read however
>> # of bytes from that, after which it keeps it's own entropy
>> pool of sorts.  The ESAPI for Java project almost exclusively
>> uses the SecureRandom no-args CTOR. There may be exceptions in
>> the JUnit tests, but that would be to make the SecureRandom sequence
>> deterministic for testing purposes.
> OK. I'll add comments that the caller is responsible for zeroization
> of the material provided in the alternate CTORs (it should be self
> evident, but....).
>
>> Secondly, there is no reason why you can't add additional CTORs.
>> For example, you could add one like this:
>>      explicit SecureRandom(const std::vector<CryptoPP::SecByteBlock>& seed);
> Do we want to expose Crypto++ gear? I would prefer the project keep
> Crypto++ locked up and hidden from plain site.
>
> I think we should supply a BinStr and SecBinStr or similar.
> [Sec]BinStr would then supplant both SecureRandom(const byte* seed,
> size_t size) and SecureRandom(const std::vector<byte>& seed).
>
>> Wouldn't this then solve your problem about zeroing out memory?
>>
>> Thirdly, I think
>>     explicit SecureRandom(const byte* seed, size_t size);
> I don't believe we are responsible for the zeroization. In this case,
> the caller is responsible. Can we trust the caller to 'be informed' or
> 'not be lazy'? At minimum, we need to document that we are not
> clearing the data after use.
>
>> is going to be prone to SIGSEGV issues. Do we really need it?
> It could cause a fault, and I'm not opposed to yanking it in favor of
> a type safe container. I would prefer to use either BinStr and
> SecBinStr (and drop the vector overload also).
>
>> At a minimum, I think you need to do a memcpy() of the byte pointer
>> seed. You could still then call memset() on 'seed' to zero it out,
>> but you can't reset the pointer to null to (hopefully) prevent it
>> from being used again. So if someone uses 'seed' again (say to
>> create another SecureRandom object), it now provides NO ENTROPY.
>> Very bad. In this particular case, personally, I'd prefer to
>> leave off the 'const' modifier and then reset the 'seed' pointer
>> to null.
> To zeoize for the caller, you would need SecureRandom(byte*& seed,
> size_t& size). Its doable, but very atypical.
>
> As for seed reuse, I feel its outside ESAPI's purview. If you feel a
> user cannot be trusted with seeding, then yank setSeed(const byte*
> seed, size_t size) and friends and ESAPI will auto seed for them.
>
>> IMHO, better to die an immediate horrible death because
>> of a SIGSEGV than to accidentally think you have some crypto
>> security when instead you have none.
> Yep.
>
>> That brings me to the last point on this. Perhaps some measure of
>> entropy is in order for the seed. Nothing fancy, but for instance
>> if you get passed 20 bytes of nulls or a vector of all null bytes,
>> my guess is someone is reusing something that was already zeroed
>> out, in which cause I think that some sort of exception should be
>> thrown. (Really no other way to fail a CTOR.)
> OK, but how would this be implemented?
>
>>> The problem is highlighted with SecureRandom's nextBytes():
>>>
>>>    // Generates a user-specified number of random bytes.
>>>    void nextBytes(std::vector<byte>& bytes);
>>>
>>> In nextBytes(), we have pigeon-holed the user into using an insecure
>>> container when using STL for safety.
>>
>> There are two approaches here.
>>  1) Assume the programmer knows how to clean up for her/himself and that
>>     s/he will zero out the returned vector after they are through with it.
>>  2) Try to do it for them automagically.
> I think (1) is leaving too much to chance - we are not 'secure out of
> the box'. I would prefer either BinStr or SecBinStr - so the user
> knows its one or the other. Hopefully, they would deduce one is
> secure, the other is not.
>
>> In the Java world, #2 was never an option as it has no DTORs and
>> 'finalizers' in Java are not guaranteed to be called at any particular
>> time.
> Right, but (2) is an option, and I feel we should do it for them in SecBinStr.
>
>> The easy thing to do is to punt and put the onus on the ones
>> using ESAPI for C++.
> But is it the right thing to do? Will ESAPI be 'secure out of the
> box'? Or will it be partially implemented, placing demands on a
> [possibly dumb] user? I think this is clearly our responsibility.
>
>> Ultimately, the proper thing to do would be
>> to extend vector<T> (or wrap it with some facade) and make it
>> so that the DTOR cleans up by zeroing the container's contents.
> OK. How about if BinStr and SecBinStr 'have a' vector<byte> using a
> secure allocator as required (BinStr can use a std::allocator). The
> secure allocator will ensure cleanup, even in the event of an
> exception.
>
>> Of course, that may not be generally possible for all types T,
>> but certainly for primitive types it is.
>>
>> The *expedient* thing to do is probably to note it with some TODO
>> comment, log it as a Google issue, and come back to it if and when
>> we have time.  In fact, that is exactly what I would suggest. We
>> can't let the perfect become the enemy of the good. If we do, we
>> will never finish and there will be no ESAPI for C++ at all.
> Agreed. I'll get a issue logged for insecure allocators (and probably
> begin work shortly on the allocator).
>
>>> A nastier problem exists in SecretKey and friends. The user must
>>> accept a bye array only - not {byte,len} - from getEncoded():
>>>
>>>    /**
>>>     * Returns the key in its primary encoding format, or nullptr
>>>     * if this key does not support encoding.
>>>     */
>>>    virtual const byte* getEncoded() const;
>>
>> We *could* change that. This was just to make it easier to port
>> from Java. But not in Java, byte[] is returned and the '.length'
>> notation automatically gives the size.  You could pass it a
>> non-const reference that will set the size of the returned
>> value.
> Our method could simply return a SecBinStr, which has a byte* and
> length; and performs proper cleanup.
>
>> Alternately, you can assume that the developer supposedly knows
>> the algorithm and thus the key size. (Note: This fails miserably for
>> DESede as it can be either 112-bits or 168-bits. The Java implementation
>> of KeyGenerator gets around this by always generating a 168-bit
>> DESede key, but when the encryption is done and you tell it to
>> use only 112-bit key, it only uses the first 112-bits. I believe
>> there's a comment that I put in JavaEncryptor that describes this.)
> No, let's not give the user any credit. Its safe for a dumb user, and
> savy users will appreciate the [small amount] extra work we performed
> on his/her behalf.
>
>>> ********************
>>>
>>> I believe the project needs to provide a secure, random access,
>>> sequence container with copy semantics. I believe we can build upon
>>> the STL containers, since they allow us to provide an allocator:
>>>  * string (basic_string): http://www.sgi.com/tech/stl/basic_string.html
>>>  * vector: http://www.sgi.com/tech/stl/Vector.html
>>>
>>> Conceptually, I think vector is closest to what we need. However,
>>> vector does not allow for copying out of the box. basic_string is
>>> copyable, but it carries excess baggage due to locales and facets for
>>> language functionality such as searching.
>>
>> Actually, I could see that such a container might a useful 'util' class
>> in its own right. What I'm not sure of is how you can make something
>> like
>>        vector<T, SecureAllocator>
> Yes, this is over half the battle! Its basically what I envision for SecBinStr.
>
> Our SecureAllocator will offer a rebind(), so users can switch between
> BinStr and SecBinStr (I believe that's what it needs, need to test
> though).
>
>> to work with *all* types T. Some T make not have means to zero / clear
>> them and I suppose you can do this with a global operator new & delete,
>> what's to say that someone else doesn't define these as well.
> I'm more concerned about the built in types, specifically a byte
> (unsigned char). We can audit other objects for proper zeroization,
> but that usually boils down to zeroizing a byte[].
>
>> maybe (I hope) I'm just not seeing this because I've been on hiatus from
>> C++ for so long. (Note to Bjarne: I didn't miss it.)
>>
>>> Any thoughts?
>>
>> Hmm...nothing comes to mind.
> I didn't think so ;)
>
> While I focused on dumb users*, keep in mind ESAPI might be part of a
> FIPS compliant solution. In this case, we *really* should be providing
> the user these containers since zeroization is part of FIPS Level 1.
> Otherwise, we will have our own internal zeroizng stuff, and the user
> has to duplicate the same functionality. Not a very smart way to go
> about things.
>
> And if I were a dumb user, I would cry bloody murder if my underlying
> library did not have - and offer me use of -  secure
> storage/allocation/deletion. Its standard warez.
>
> jeff
>



-- 
Blog: http://off-the-wall-security.blogspot.com/
"The most likely way for the world to be destroyed, most experts agree,
is by accident. That's where we come in; we're computer professionals.
We *cause* accidents."        -- Nathaniel Borenstein