[PATCH 2/2] Make _gcry_burn_stack use variable length array

[Jussi Kivilinna]

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 [1] and that this
>> kind of optimization is allowed.
> 
> From [1]:
> 
>   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
>   library.dd
> 
> 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.

No problem.

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.

-Jussi

> 
> 
> Shalom-Salam,
> 
>    Werner
>