[PATCH 2/2] Make _gcry_burn_stack use variable length array
jussi.kivilinna at iki.fi
Tue Sep 3 21:57:25 CEST 2013
On 03.09.2013 22:17, Werner Koch wrote:
> On Tue, 3 Sep 2013 19:18, jussi.kivilinna at iki.fi said:
>> 64-byte stack buffer instead of burn stack deeper. It's mentioned in GCC
>> bugzilla that _gcry_burn_stack is doing wrong thing here  and that this
>> kind of optimization is allowed.
> From :
> This is also a libgcrypt bug, because clearly if it wants to burn
> some portion of the stack (assuming for security reasons), then
> 1) if it doesn't prevent tail recursion or tail call to itself, it
> doesn't do what it is trying to do, in particular it keeps overwriting
> with zeros the same portion of memory over and over
> 2) even if it isn't tail recursion/call optimized, on most targets it
> will still leave gaps on the stack not overwritten
> So, all in all, quite questionable way of burning cycles in the crypto
> Right, this is a about security. It is actual a standard practice which
> has been around for 20 years or so.
> I have not seen a reference to the C specs that explains that
> optimization of tail recursion is allowed. Doubtless this one of those
> esoteric interpretations of the standard. Turning C into a high level
> language is a Bad Thing(tm). People who want that should use a high
> level language. I wonder who long it will take that de-facto safe
> assumptions, like CHAR_BIT is 8, will be sacrificed for yet another small
> performance optimization. Do that for C++ but keep hands off the things
> which worked well for more than 30 years. This was not the first case
> were stretching the interpretation of the standard opened holes in
> existing software. Right there is a standard, but there is also a
> common understanding on how certain things are to be interpreted. The
> latter is more important than the wording of standard.
> And yes, there might be gaps on the stack. But for any straight forward
> C compiler that would not matter at all. We merely try our best to not
> leave traces of secrets on the stack while being portable. And while we
> are at it: Why is paged out memory still not encrypted by default?
> Sorry for the rant.
What annoys me, is that this issue was not communicated here, so that burn stack could have been properly adjusted.
>> So lets fix _gcry_burn_stack by using variable length array when VLAs
>> are supported by compiler. This should ensure proper stack burning to
>> the requested depth and avoid GCC loop optimizations.
> Okay, that might work for a while.
>> +#ifdef HAVE_VLA
>> + int buflen = (((bytes <= 0) ? 1 : bytes) + 63) & ~63;
>> + char buf[buflen];
> Add volatile ?
wipememory() casts input pointer to volatile, which should be enough .. but, well, with current compilers you'll never know. I'll throw that volatile in.
More information about the Gcrypt-devel