fixing -fPIE + -fstack-protector-all

fixing -fPIE + -fstack-protector-all

[John Spencer]

using -fPIE + -fstack-protector-all is currently broken for a number of 
architectures (most notably i386) in the default gcc setup (including 
the musl-cross patches), as it depends on a libssp_nonshared.a which 
provides __stack_chk_fail_local().

even when gcc's version is built from ssp-local.c and installed via
`make install-target-libssp`, gcc won't use it to link programs compiled 
with -fstack-protector[-all].
the reason is that (since we provide the rest of the ssp functionality 
in musl) we set gcc_cv_libc_provides_ssp=yes, which, contrary to our 
earlier expectations means: "libc provides ssp *and* ssp_nonshared":

(from gcc/gcc.c)
#ifdef TARGET_LIBC_PROVIDES_SSP
#define LINK_SSP_SPEC "%{fstack-protector:}"
#else
#define LINK_SSP_SPEC 
"%{fstack-protector|fstack-protector-all:-lssp_nonshared -lssp}"
#endif

so my conclusion is that in order to fix this issue cleanly and get musl 
support into upstream, we need to either (on the gcc side) define a new
gcc_cv_libc_provides_ssp_but_not_ssp_nonshared conditional and an 
install target that installs only libssp_nonshared but not libssp, and 
links only to libssp_nonshared if ssp was used, or somehow get that 
symbol (__stack_chk_fail_local()) into musl and linked to binaries in a 
way that doesn't require additional library flags on the linker command 
line.

in the former case, the above snippet would look like

#ifdef TARGET_LIBC_PROVIDES_SSP
#define LINK_SSP_SPEC "%{fstack-protector:}"
#elif TARGET_LIBC_PROVIDES_SSP_BUT_NOT_SSP_NONSHARED
#define LINK_SSP_SPEC 
"%{fstack-protector|fstack-protector-all:-lssp_nonshared}"
#else
#define LINK_SSP_SPEC 
"%{fstack-protector|fstack-protector-all:-lssp_nonshared -lssp}"
#endif

--JS

Re: fixing -fPIE + -fstack-protector-all

[Rich Felker]

On Wed, Nov 05, 2014 at 04:25:03PM +0100, John Spencer wrote:
> using -fPIE + -fstack-protector-all is currently broken for a number
> of architectures (most notably i386) in the default gcc setup
> (including the musl-cross patches), as it depends on a
> libssp_nonshared.a which provides __stack_chk_fail_local().

As discussed on IRC, I would _like_ to be able to simply add the
following to crt/i386/crti.s:

__stack_chk_fail_local: hlt

and equivalent for other archs. This has the added benefit of
effecting a crash without going through the PLT (whereas
libssp_nonshared.a's __stack_chk_fail_local calls __stack_chk_fail via
the PLT) so it's not vulnerable to attacks that have overwritten the
GOT with malicious pointers.

However, this proposed solution breaks one odd corner case: static
linking when all the source files were compiled with -fPIC or -fPIE.
In that case, there would be no references to __stack_chk_fail, only
to __stack_chk_fail_local, and thereby __init_ssp would not get
linked, and a zero canary would be used.

One possible way to handle this would be giving up the conditional
linking of ssp init and just always initializing it. The .o file is 78
bytes on i386 and 70 bytes on x86_64, but there would also be some
savings to offset the cost simply from having the code inline in
__init_libc rather than as an external function.

I'm open to other ideas too.

Rich

Re: fixing -fPIE + -fstack-protector-all

[John Spencer]

Rich Felker wrote:
> On Wed, Nov 05, 2014 at 04:25:03PM +0100, John Spencer wrote:
>> using -fPIE + -fstack-protector-all is currently broken for a number
>> of architectures (most notably i386) in the default gcc setup
>> (including the musl-cross patches), as it depends on a
>> libssp_nonshared.a which provides __stack_chk_fail_local().
> 
> As discussed on IRC, I would _like_ to be able to simply add the
> following to crt/i386/crti.s:
> 
> __stack_chk_fail_local: hlt
> 
> and equivalent for other archs. This has the added benefit of
> effecting a crash without going through the PLT (whereas
> libssp_nonshared.a's __stack_chk_fail_local calls __stack_chk_fail via
> the PLT) so it's not vulnerable to attacks that have overwritten the
> GOT with malicious pointers.
> 
> However, this proposed solution breaks one odd corner case: static
> linking when all the source files were compiled with -fPIC or -fPIE.
> In that case, there would be no references to __stack_chk_fail, only
> to __stack_chk_fail_local, and thereby __init_ssp would not get
> linked, and a zero canary would be used.
> 
> One possible way to handle this would be giving up the conditional
> linking of ssp init and just always initializing it. The .o file is 78
> bytes on i386 and 70 bytes on x86_64, but there would also be some
> savings to offset the cost simply from having the code inline in
> __init_libc rather than as an external function.

that sounds reasonable. do you intend add this symbol and the mandatory 
init_ssp code even to archs that don't need it (for example x86_64) ?

--JS

Re: fixing -fPIE + -fstack-protector-all

[Rich Felker]

On Wed, Nov 05, 2014 at 05:01:31PM +0100, John Spencer wrote:
> Rich Felker wrote:
> >On Wed, Nov 05, 2014 at 04:25:03PM +0100, John Spencer wrote:
> >>using -fPIE + -fstack-protector-all is currently broken for a number
> >>of architectures (most notably i386) in the default gcc setup
> >>(including the musl-cross patches), as it depends on a
> >>libssp_nonshared.a which provides __stack_chk_fail_local().
> >
> >As discussed on IRC, I would _like_ to be able to simply add the
> >following to crt/i386/crti.s:
> >
> >__stack_chk_fail_local: hlt
> >
> >and equivalent for other archs. This has the added benefit of
> >effecting a crash without going through the PLT (whereas
> >libssp_nonshared.a's __stack_chk_fail_local calls __stack_chk_fail via
> >the PLT) so it's not vulnerable to attacks that have overwritten the
> >GOT with malicious pointers.
> >
> >However, this proposed solution breaks one odd corner case: static
> >linking when all the source files were compiled with -fPIC or -fPIE.
> >In that case, there would be no references to __stack_chk_fail, only
> >to __stack_chk_fail_local, and thereby __init_ssp would not get
> >linked, and a zero canary would be used.
> >
> >One possible way to handle this would be giving up the conditional
> >linking of ssp init and just always initializing it. The .o file is 78
> >bytes on i386 and 70 bytes on x86_64, but there would also be some
> >savings to offset the cost simply from having the code inline in
> >__init_libc rather than as an external function.
> 
> that sounds reasonable. do you intend add this symbol and the
> mandatory init_ssp code even to archs that don't need it (for
> example x86_64) ?

Right now I don't have a specific "intent" since this is just an idea.
But I think it would probably be ugly, and preclude the inlining, to
do the __init_ssp stuff differently on different archs. The local
symbol could of course be omitted on archs that don't use it, but it
doesn't hurt to have it either.

Rich

Re: fixing -fPIE + -fstack-protector-all

[Andy Lutomirski]

On 11/05/2014 07:43 AM, Rich Felker wrote:
> On Wed, Nov 05, 2014 at 04:25:03PM +0100, John Spencer wrote:
>> using -fPIE + -fstack-protector-all is currently broken for a number
>> of architectures (most notably i386) in the default gcc setup
>> (including the musl-cross patches), as it depends on a
>> libssp_nonshared.a which provides __stack_chk_fail_local().
> 
> As discussed on IRC, I would _like_ to be able to simply add the
> following to crt/i386/crti.s:
> 
> __stack_chk_fail_local: hlt
> 
> and equivalent for other archs. This has the added benefit of
> effecting a crash without going through the PLT (whereas
> libssp_nonshared.a's __stack_chk_fail_local calls __stack_chk_fail via
> the PLT) so it's not vulnerable to attacks that have overwritten the
> GOT with malicious pointers.
> 
> However, this proposed solution breaks one odd corner case: static
> linking when all the source files were compiled with -fPIC or -fPIE.
> In that case, there would be no references to __stack_chk_fail, only
> to __stack_chk_fail_local, and thereby __init_ssp would not get
> linked, and a zero canary would be used.

Can you do something like:

__stack_chk_fail_local:
  hlt
  .pushsection .discard
  call __init_ssp
  .popsection

and stick it in either its own object or its own group?  Or is ld too
clever for that?

--Andy

Re: Re: fixing -fPIE + -fstack-protector-all

[Rich Felker]

On Wed, Nov 05, 2014 at 05:34:49PM -0800, Andy Lutomirski wrote:
> On 11/05/2014 07:43 AM, Rich Felker wrote:
> > On Wed, Nov 05, 2014 at 04:25:03PM +0100, John Spencer wrote:
> >> using -fPIE + -fstack-protector-all is currently broken for a number
> >> of architectures (most notably i386) in the default gcc setup
> >> (including the musl-cross patches), as it depends on a
> >> libssp_nonshared.a which provides __stack_chk_fail_local().
> > 
> > As discussed on IRC, I would _like_ to be able to simply add the
> > following to crt/i386/crti.s:
> > 
> > __stack_chk_fail_local: hlt
> > 
> > and equivalent for other archs. This has the added benefit of
> > effecting a crash without going through the PLT (whereas
> > libssp_nonshared.a's __stack_chk_fail_local calls __stack_chk_fail via
> > the PLT) so it's not vulnerable to attacks that have overwritten the
> > GOT with malicious pointers.
> > 
> > However, this proposed solution breaks one odd corner case: static
> > linking when all the source files were compiled with -fPIC or -fPIE.
> > In that case, there would be no references to __stack_chk_fail, only
> > to __stack_chk_fail_local, and thereby __init_ssp would not get
> > linked, and a zero canary would be used.
> 
> Can you do something like:
> 
> __stack_chk_fail_local:
>   hlt
>   .pushsection .discard
>   call __init_ssp
>   .popsection
> 
> and stick it in either its own object or its own group?  Or is ld too
> clever for that?

That certainly could go in libssp_nonshared.a, and it could be
implemented in C as:

void __stack_chk_fail_local()
{
	a_crash();
}

void __stack_chk_fail_ref()
{
	__stack_chk_fail();
}

or similar. However this couldn't be done by putting it in crti.s
since crti.o is always linked and the effect would just be the same as
unconditionally initializing the stack pointer (but less optimizable).

Rich

Re: fixing -fPIE + -fstack-protector-all

[Anthony G. Basile]

On 11/05/14 10:43, Rich Felker wrote:
> On Wed, Nov 05, 2014 at 04:25:03PM +0100, John Spencer wrote:
>> using -fPIE + -fstack-protector-all is currently broken for a number
>> of architectures (most notably i386) in the default gcc setup
>> (including the musl-cross patches), as it depends on a
>> libssp_nonshared.a which provides __stack_chk_fail_local().
>
> As discussed on IRC, I would _like_ to be able to simply add the
> following to crt/i386/crti.s:
>
> __stack_chk_fail_local: hlt
>
> and equivalent for other archs. This has the added benefit of
> effecting a crash without going through the PLT (whereas
> libssp_nonshared.a's __stack_chk_fail_local calls __stack_chk_fail via
> the PLT) so it's not vulnerable to attacks that have overwritten the
> GOT with malicious pointers.

For what its worth, hardening in gentoo (PaX kernel + userland hardening 
with relro and bindnow) tries to prevent this kind of attack by making 
the GOT read only after initial linking.

>
> However, this proposed solution breaks one odd corner case: static
> linking when all the source files were compiled with -fPIC or -fPIE.
> In that case, there would be no references to __stack_chk_fail, only
> to __stack_chk_fail_local, and thereby __init_ssp would not get
> linked, and a zero canary would be used.

I would rather not see this solution.

>
> One possible way to handle this would be giving up the conditional
> linking of ssp init and just always initializing it. The .o file is 78
> bytes on i386 and 70 bytes on x86_64, but there would also be some
> savings to offset the cost simply from having the code inline in
> __init_libc rather than as an external function.
>
> I'm open to other ideas too.
>
> Rich
>


-- 
Anthony G. Basile, Ph. D.
Chair of Information Technology
D'Youville College
Buffalo, NY 14201
(716) 829-8197

Re: fixing -fPIE + -fstack-protector-all

[John Spencer]

Anthony G. Basile wrote:
> On 11/05/14 10:43, Rich Felker wrote:
>> However, this proposed solution breaks one odd corner case: static
>> linking when all the source files were compiled with -fPIC or -fPIE.
>> In that case, there would be no references to __stack_chk_fail, only
>> to __stack_chk_fail_local, and thereby __init_ssp would not get
>> linked, and a zero canary would be used.
> 
> I would rather not see this solution.
> 

why ?

Re: fixing -fPIE + -fstack-protector-all

[Anthony G. Basile]

On 11/06/14 07:36, John Spencer wrote:
> Anthony G. Basile wrote:
>> On 11/05/14 10:43, Rich Felker wrote:
>>> However, this proposed solution breaks one odd corner case: static
>>> linking when all the source files were compiled with -fPIC or -fPIE.
>>> In that case, there would be no references to __stack_chk_fail, only
>>> to __stack_chk_fail_local, and thereby __init_ssp would not get
>>> linked, and a zero canary would be used.
>>
>> I would rather not see this solution.
>>
>
> why ?

I want to save that corner case.  In gentoo we compile everything 
pic/pie, even our executables:

# readelf -h /bin/bash  | grep Type:
   Type:                              DYN (Shared object file)

This randomizes even the address of main.  The following

#include <stdio.h>
int main() {
   printf("%p\n", main);
}

yields

0x61f64b845
0x33fa0b7845
0x189ab51845
0x58531cd845

on successive runs when compiled with our default gcc specs.  Along with 
PaX's enhenced aslr this helps against brute forcing addresses.  Compare 
to when I turn off pie:

0x400605
0x400605
0x400605

Having said that, currently we do not support *static* pic/pie in 
Gentoo, but I would really like to, especially with musl.  Also, I'm not 
as worried about the GOT for reasons I stated elsewhere.


-- 
Anthony G. Basile, Ph. D.
Chair of Information Technology
D'Youville College
Buffalo, NY 14201
(716) 829-8197

Re: fixing -fPIE + -fstack-protector-all

[Andy Lutomirski]

On 11/06/2014 03:45 AM, Anthony G. Basile wrote:
> On 11/05/14 10:43, Rich Felker wrote:
>> On Wed, Nov 05, 2014 at 04:25:03PM +0100, John Spencer wrote:
>>> using -fPIE + -fstack-protector-all is currently broken for a number
>>> of architectures (most notably i386) in the default gcc setup
>>> (including the musl-cross patches), as it depends on a
>>> libssp_nonshared.a which provides __stack_chk_fail_local().
>>
>> As discussed on IRC, I would _like_ to be able to simply add the
>> following to crt/i386/crti.s:
>>
>> __stack_chk_fail_local: hlt
>>
>> and equivalent for other archs. This has the added benefit of
>> effecting a crash without going through the PLT (whereas
>> libssp_nonshared.a's __stack_chk_fail_local calls __stack_chk_fail via
>> the PLT) so it's not vulnerable to attacks that have overwritten the
>> GOT with malicious pointers.
> 
> For what its worth, hardening in gentoo (PaX kernel + userland hardening
> with relro and bindnow) tries to prevent this kind of attack by making
> the GOT read only after initial linking.

What does the PaX kernel have to do with this?

--Andy

Re: Re: fixing -fPIE + -fstack-protector-all

[Anthony G. Basile]

On 11/06/14 21:10, Andy Lutomirski wrote:
> On 11/06/2014 03:45 AM, Anthony G. Basile wrote:
>> On 11/05/14 10:43, Rich Felker wrote:
>>> On Wed, Nov 05, 2014 at 04:25:03PM +0100, John Spencer wrote:
>>>> using -fPIE + -fstack-protector-all is currently broken for a number
>>>> of architectures (most notably i386) in the default gcc setup
>>>> (including the musl-cross patches), as it depends on a
>>>> libssp_nonshared.a which provides __stack_chk_fail_local().
>>>
>>> As discussed on IRC, I would _like_ to be able to simply add the
>>> following to crt/i386/crti.s:
>>>
>>> __stack_chk_fail_local: hlt
>>>
>>> and equivalent for other archs. This has the added benefit of
>>> effecting a crash without going through the PLT (whereas
>>> libssp_nonshared.a's __stack_chk_fail_local calls __stack_chk_fail via
>>> the PLT) so it's not vulnerable to attacks that have overwritten the
>>> GOT with malicious pointers.
>>
>> For what its worth, hardening in gentoo (PaX kernel + userland hardening
>> with relro and bindnow) tries to prevent this kind of attack by making
>> the GOT read only after initial linking.
>
> What does the PaX kernel have to do with this?
>
> --Andy
>

Overspoke.  The userland stuff is sufficient to freeze the GOT.  I was 
just tangentially thinking of PaX's enhanced aslr.

-- 
Anthony G. Basile, Ph. D.
Chair of Information Technology
D'Youville College
Buffalo, NY 14201
(716) 829-8197

Re: fixing -fPIE + -fstack-protector-all

[Timo Teras]

On Wed, 05 Nov 2014 16:25:03 +0100
John Spencer <maillist-musl@barfooze.de> wrote:

> using -fPIE + -fstack-protector-all is currently broken for a number
> of architectures (most notably i386) in the default gcc setup
> (including the musl-cross patches), as it depends on a
> libssp_nonshared.a which provides __stack_chk_fail_local().

In Alpine Linux we are patching gcc to unconditionally to have
-lssp_nonshared:
http://git.alpinelinux.org/cgit/aports/tree/main/gcc/gcc-4.8-musl-libssp.patch

And making musl package provide that library:
http://git.alpinelinux.org/cgit/aports/tree/main/musl/__stack_chk_fail_local.c
http://git.alpinelinux.org/cgit/aports/tree/main/musl/APKBUILD#n60

This is for two reasons:

1. gcc bootstrap is broken if it's to be compiled with
-fstack-protector otherwise

2. Linking without "-fstack-protector" flag with .a or .o files that
have been compiled with SSP would break.

Basically, __stack_chk_fail_local symbol should be provided always.
Agreeably gcc should emit 'hlt' or similar instead of that function
call. Or at least provide implementation for that function with 'once'
linking.

/Timo

Re: fixing -fPIE + -fstack-protector-all

[Anthony G. Basile]

On 11/05/14 11:01, Timo Teras wrote:
> On Wed, 05 Nov 2014 16:25:03 +0100
> John Spencer <maillist-musl@barfooze.de> wrote:
>
>> using -fPIE + -fstack-protector-all is currently broken for a number
>> of architectures (most notably i386) in the default gcc setup
>> (including the musl-cross patches), as it depends on a
>> libssp_nonshared.a which provides __stack_chk_fail_local().
>
> In Alpine Linux we are patching gcc to unconditionally to have
> -lssp_nonshared:
> http://git.alpinelinux.org/cgit/aports/tree/main/gcc/gcc-4.8-musl-libssp.patch

The piepatches do not need to link in anything on glibc or uclibc, 
whereas in your approach they need to pick up the symbol from 
libc_unshared.  It also doesn't depend on whether its a cross compile or 
native attempt to boostrap into a hardened system (gentoo style) form a 
non-hardened musl i686.  In both cases it will fail unless you provide 
__stack_chk_fail_local by some "hack".

>
> And making musl package provide that library:
> http://git.alpinelinux.org/cgit/aports/tree/main/musl/__stack_chk_fail_local.c
> http://git.alpinelinux.org/cgit/aports/tree/main/musl/APKBUILD#n60
>
> This is for two reasons:
>
> 1. gcc bootstrap is broken if it's to be compiled with
> -fstack-protector otherwise
>
> 2. Linking without "-fstack-protector" flag with .a or .o files that
> have been compiled with SSP would break.
>
> Basically, __stack_chk_fail_local symbol should be provided always.

Agreed.  The symbol is there on both x86_64 and i386 in libc_nonshared.a 
(glibc).

What I've never understood is why this appears only as an issue in i686 
and not x86_64 for musl.  I haven't had time to dig into gcc internals 
to find out why.

> Agreeably gcc should emit 'hlt' or similar instead of that function
> call. Or at least provide implementation for that function with 'once'
> linking.
>
> /Timo
>



-- 
Anthony G. Basile, Ph. D.
Chair of Information Technology
D'Youville College
Buffalo, NY 14201
(716) 829-8197

Re: fixing -fPIE + -fstack-protector-all

[Rich Felker]

On Thu, Nov 06, 2014 at 07:11:43AM -0500, Anthony G. Basile wrote:
> >Basically, __stack_chk_fail_local symbol should be provided always.
> 
> Agreed.  The symbol is there on both x86_64 and i386 in
> libc_nonshared.a (glibc).
> 
> What I've never understood is why this appears only as an issue in
> i686 and not x86_64 for musl.  I haven't had time to dig into gcc
> internals to find out why.

__stack_chk_fail_local is needed on any arch/ABI where calls through
the PLT require a valid GOT pointer to be setup by the caller. GCC
always makes a local call for SSP violations so that it doesn't impose
GOT pointer initialization on every single function; the GOT pointer
initialization is deferred to the local function, which is then able
to safely call the non-local function __stack_chk_fail.

On x86_64, a GOT pointer is needed because the ISA supports direct
PC-relative addressing which can be used by the PLT to jump to get the
actual function address from the GOT. I'm not sure if there are others
that have this nice property.

Rich

Re: fixing -fPIE + -fstack-protector-all

[John Spencer]

Timo Teras wrote:
> On Wed, 05 Nov 2014 16:25:03 +0100
> John Spencer <maillist-musl@barfooze.de> wrote:
> 
>> using -fPIE + -fstack-protector-all is currently broken for a number
>> of architectures (most notably i386) in the default gcc setup
>> (including the musl-cross patches), as it depends on a
>> libssp_nonshared.a which provides __stack_chk_fail_local().
> 
> In Alpine Linux we are patching gcc to unconditionally to have
> -lssp_nonshared:
> http://git.alpinelinux.org/cgit/aports/tree/main/gcc/gcc-4.8-musl-libssp.patch
> 
> And making musl package provide that library:
> http://git.alpinelinux.org/cgit/aports/tree/main/musl/__stack_chk_fail_local.c
> http://git.alpinelinux.org/cgit/aports/tree/main/musl/APKBUILD#n60
> 

yeah, i originally looked at what alpine does but then noticed that gcc 
builds its own libssp_nonshared.a anyway.
btw: the gcc one is, unlike alpine's, built with hidden visibility (from 
ssp-local.c):

extern void __stack_chk_fail (void);

void
__attribute__((visibility ("hidden")))
__stack_chk_fail_local (void)
{
   __stack_chk_fail ();
}

Re: fixing -fPIE + -fstack-protector-all

[Timo Teras]

On Thu, 06 Nov 2014 13:40:43 +0100
John Spencer <maillist-musl@barfooze.de> wrote:

> Timo Teras wrote:
> > On Wed, 05 Nov 2014 16:25:03 +0100
> > John Spencer <maillist-musl@barfooze.de> wrote:
> > 
> >> using -fPIE + -fstack-protector-all is currently broken for a
> >> number of architectures (most notably i386) in the default gcc
> >> setup (including the musl-cross patches), as it depends on a
> >> libssp_nonshared.a which provides __stack_chk_fail_local().
> > 
> > In Alpine Linux we are patching gcc to unconditionally to have
> > -lssp_nonshared:
> > http://git.alpinelinux.org/cgit/aports/tree/main/gcc/gcc-4.8-musl-libssp.patch
> > 
> > And making musl package provide that library:
> > http://git.alpinelinux.org/cgit/aports/tree/main/musl/__stack_chk_fail_local.c
> > http://git.alpinelinux.org/cgit/aports/tree/main/musl/APKBUILD#n60
> > 
> 
> yeah, i originally looked at what alpine does but then noticed that
> gcc builds its own libssp_nonshared.a anyway.
> btw: the gcc one is, unlike alpine's, built with hidden visibility
> (from ssp-local.c):

We do set the hidden visibility attribute. If it's not working then
something is wrong somewhere.

And yes, gcc is eager to build it's own libssp. You need to
--disable-libssp for the above to work. We do it at:
http://git.alpinelinux.org/cgit/aports/tree/main/gcc/APKBUILD#n273

/Timo