more -fstack-protector!

[Andrew Bartlett]

On Thu, 2013-03-07 at 10:51 +0100, Andreas Schneider wrote:
> On Thursday 07 March 2013 13:09:58 Andrew Bartlett wrote:
> > On Wed, 2013-03-06 at 19:08 -0500, Ira Cooper wrote:
> > > On Wed, Mar 6, 2013 at 6:16 PM, Andrew Bartlett <abartlet at samba.org> 
> wrote:
> > > > On Wed, 2013-03-06 at 09:22 -0500, Ira Cooper wrote:
> > > > > This patch adds the ability to:
> > > > > 
> > > > > --disable-stack-protector (For those who don't want it.)
> > > > > --enable-stack-protector-all (For debugging or the paranoid.)
> > > > > 
> > > > > Both builds have been tested locally on illumos.  (As far as that they
> > > > > build.)
> > > > > 
> > > > > In the future, please don't default flags like this, without a toggle
> > > > > to
> > > > > turn them off.
> > > > 
> > > > On a more meta level:
> > > > 
> > > > In Samba, with very, very few fixed-length strings on the stack (die,
> > > > pstring, die), where does -fstack-protector help?  Can we get some
> > > > diagnostics as to where this is being applied, such that the cost 1%
> > > > cost is showing up?
> > > 
> > > Sorry,
> > > 
> > > I've got other things to work on.  The only reason I actually jumped in at
> > > all was it broke my build, and then I realized I only 1/2 fixed what was
> > > wrong, so I mopped up my work.
> > > 
> > > Others appear to have a better grasp on what is going on.  I just want a
> > > way to turn it off.
> > 
> > You suggested this option has a speed cost.  I'm at the very least
> > asking that you detail that.
> 
> fstack-protector puts a canary value on the stack of key functions. Just 
> before the return address and just before returning from that value, that 
> canary value is verified. If there was a buffer overflow, the canary no longer 
> matches and the program aborts. The canary value is random for each time the 
> application is started and makes it impossible to guess remotely.
> 
> The speed cost is setting and checking the canary...

It seems incredible to me that this, if properly applied (eg the
criteria we discuss below) can really cost 1%, which is why I'm wanting
detail. 

> Red Hat and SUSE are are using it by default ...
> 
> > I don't like having any more configure options that we absolutely have
> > to have, because each of them has to be tested somehow.  Where at all
> > possible we should just do the right thing, whatever that is.
> > 
> > As such, I would like detail from you and from those who added this in
> > the first place as to what the costs and specific benefits are.
> 
> It is a stack smash protection.

Given the way we handle strings and buffers in Samba, can you point at
the buffers you think might be subject to such an attack?

> > In particular, what array declarations 'buffers larger than 8 bytes'
> > does the compiler see in our hot path?
> 
> I don't see what this has todo with buffers larger than 8 bytes. This is for 
> buffer overflow detection for any buffer.

That is a quote from the GCC manpage on my Fedora 17 box.  I take it
that this is the default criteria for it guessing that a buffer is a
string or something a bad memcpy() might be aimed at, rather than just
integers (for example), and so to apply this protection to that
particular function:

       -fstack-protector
           Emit extra code to check for buffer overflows, such as stack
smashing attacks.  This is done by adding a guard variable to functions
with vulnerable objects.  This includes
           functions that call alloca, and functions with buffers larger
than 8 bytes.  The guards are initialized when a function is entered and
then checked when the function exits.
           If a guard check fails, an error message is printed and the
program exits.

       -fstack-protector-all
           Like -fstack-protector except that all functions are
protected.


If this really has a 1% cost (which seems high, and now I can't find the
reference where I thought that was stated), I would like to see the
intersection between the hot path and functions that need stack smash
protection, to see if we might find a way to avoid it on those hot paths
(perhaps with some small re-factor, or assistance to upstream GCC to be
smarter).

Or, if this really doesn't have a high cost on subsequent analysis or
because I've miss-remembered or imagined it, I would prefer we just
enable it, rather than add even more options. 

I say this because on the whole, I didn't think we used many
stack-allocated fixed length buffers in the post fstring/pstring era. 

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org