Switch on compiler hardening defaults

Henrique de Moraes Holsch · External Since: Nov 11, 2004 Posts: 648

On Thu, 29 Oct 2009, Christoph Anton Mitterer wrote:
> On Tue, 2009-10-27 at 22:19 -0200, Henrique de Moraes Holschuh wrote:
> > Well, the issue raised in LKML is that you absolutely should *not* enable
> > -fstack-protector-all unless you _really_ know what you're doing, and most
> > certainly not by default. It has nothing to do with -fstack-protector, just
> > with -fstack-protector-all. But it does show that extra stack usage CAN
> > have bad effects on performance in pathological cases (which -all seems
> > to cause more readly :-p ).
> Isn't this what they've done starting with the 2.6.31 debian packages?
> CONFIG_CC_STACKPROTECTOR_ALL=y
> CONFIG_CC_STACKPROTECTOR=y
>
> Should we bugreport this agains src:linux2.6 ?

I think so.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

--
To UNSUBSCRIBE, email to debian-devel-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org

Henrique de Moraes Holsch · External Since: Nov 11, 2004 Posts: 648

On Tue, 27 Oct 2009, Kees Cook wrote:
> On Mon, Oct 26, 2009 at 11:14:25AM +0100, Bastian Blank wrote:
> > On Sun, Oct 25, 2009 at 11:55:25AM -0700, Kees Cook wrote:
> > > I would like to propose enabling[1] the GCC hardening patches that Ubuntu
> > > uses[2].
> >
> > How do they work? Do they also change the free-standing compiler or only
> > the hosted one? There is a lot of software, which (I would say) missuse
> > the hosted compiler to build non-userspace-code, including the Linux
> > kernel.
>
> The stack protector is conditional on being linked with libc, so, if you
> build with -nostdlib (as the kernel does), it is implicitly disabled.

This doesn't make sense. The kernel can, and does use stack protector
functionality for its built if you ask it to. Do you mean the defaults are
changed only when -nostdlib is NOT given?

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

--
To UNSUBSCRIBE, email to debian-devel-REQUEST.DeleteThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.DeleteThis@lists.debian.org

Kees Cook · External Since: May 06, 2009 Posts: 13

On Thu, Oct 29, 2009 at 10:01:08PM -0200, Henrique de Moraes Holschuh wrote:
> On Tue, 27 Oct 2009, Kees Cook wrote:
> > On Mon, Oct 26, 2009 at 11:14:25AM +0100, Bastian Blank wrote:
> > > On Sun, Oct 25, 2009 at 11:55:25AM -0700, Kees Cook wrote:
> > > > I would like to propose enabling[1] the GCC hardening patches that Ubuntu
> > > > uses[2].
> > >
> > > How do they work? Do they also change the free-standing compiler or only
> > > the hosted one? There is a lot of software, which (I would say) missuse
> > > the hosted compiler to build non-userspace-code, including the Linux
> > > kernel.
> >
> > The stack protector is conditional on being linked with libc, so, if you
> > build with -nostdlib (as the kernel does), it is implicitly disabled.
>
> This doesn't make sense. The kernel can, and does use stack protector
> functionality for its built if you ask it to. Do you mean the defaults are
> changed only when -nostdlib is NOT given?

Yes, I was a bit unclear, sorry. The -fstack-protector option is not
added to the option list when either -fno-stack-protector or -nostdlib
are already in the option list. The GCC spec[1] for this is:

%{!fno-stack-protector:%{!nostdlib:-fstack-protector}}

If you add -fstack-protector to a build (regardless of -nostdlib), gcc
will attempt to use the stack protector. This is how the kernel builds
when the CC_STACKPROTECTOR option is enabled.

And I can prove this works. Smile

The Ubuntu kernel uses both the hardened
compiler and the CC_STACKPROTECTOR option, and you can see the results on
an Ubuntu system:
$ readelf -s /lib/modules/$(uname -r)/kernel/fs/nfs/nfs.ko | grep stack_chk
1114: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __stack_chk_fail

-Kees

[1] http://patch-tracker.debian.org/patch/series/view/gcc-4.4/4.4.2-1/gcc-...ault-ss

--
Kees Cook @debian.org

--
To UNSUBSCRIBE, email to debian-devel-REQUEST.RemoveThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.RemoveThis@lists.debian.org

Henrique de Moraes Holsch · External Since: Nov 11, 2004 Posts: 648

On Thu, 29 Oct 2009, Kees Cook wrote:
> On Thu, Oct 29, 2009 at 10:01:08PM -0200, Henrique de Moraes Holschuh wrote:
> > On Tue, 27 Oct 2009, Kees Cook wrote:
> > > On Mon, Oct 26, 2009 at 11:14:25AM +0100, Bastian Blank wrote:
> > > > On Sun, Oct 25, 2009 at 11:55:25AM -0700, Kees Cook wrote:
> > > > > I would like to propose enabling[1] the GCC hardening patches that Ubuntu
> > > > > uses[2].
> > > >
> > > > How do they work? Do they also change the free-standing compiler or only
> > > > the hosted one? There is a lot of software, which (I would say) missuse
> > > > the hosted compiler to build non-userspace-code, including the Linux
> > > > kernel.
> > >
> > > The stack protector is conditional on being linked with libc, so, if you
> > > build with -nostdlib (as the kernel does), it is implicitly disabled.
> >
> > This doesn't make sense. The kernel can, and does use stack protector
> > functionality for its built if you ask it to. Do you mean the defaults are
> > changed only when -nostdlib is NOT given?
>
> Yes, I was a bit unclear, sorry. The -fstack-protector option is not
> added to the option list when either -fno-stack-protector or -nostdlib
> are already in the option list. The GCC spec[1] for this is:

That, and the fact that -fstack-protector-all is NOT used, removes all
objections I might have: it means the kernel build won't be affected, and it
preserves the decisions made by the kernel upstream about which files should
get -fstack-protector and which files shouldn't.

Thanks!

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

--
To UNSUBSCRIBE, email to debian-devel-REQUEST.RemoveThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.RemoveThis@lists.debian.org

Matthias Klose · External Since: Aug 28, 2004 Posts: 1198

On 25.10.2009 19:55, Kees Cook wrote:
> Hello,
>
> I would like to propose enabling[1] the GCC hardening patches that Ubuntu
> uses[2]. Ubuntu has used it successfully for 1.5 years now (3 releases),
> and many of the issues have already been fixed in packages that needed
> adjustment[3]. After all this time, use of the hardening-wrapper[4]
> package is still very low, so I think the right thing to do is to just fix
> this in the compiler and everyone wins. I'm not suggesting that there
> won't be added work to fix problems, but I believe that for Debian the
> benefits now out-weigh the risks.
>
> I do not expect to reach consensus with all developers on this, so I'm
> not sure who I need to convince to move it forward. (Perhaps just the
> GCC maintainers?) Regardless, if you agree with this, please speak up.
> I think it's very important that this change happens.

Introducing hardening defaults is a project decision, but as you may know I
don't support the current approach to enable these defaults.

> My candid commentary and possible trolling...
>
> Arguments for:
> - users of Debian become safer (real[5] security vulnerabilities are
> either non-issues or reduced to a DoS).
> - security team has less work to do.
> - software quality improves by finding subtle bugs (and not just
> packaged software -- anything compiled with the Debian gcc).

> Arguments against:
> - makes the compiler's behavior different than stock compiler.
> Rebuttal: honestly, I don't care -- it seems like such a
> huge win for safety and is easy to debug. Debian
> already carries plenty of patches anyway -- there
> is no such thing as the "stock compiler".

Honestly I do care. While the security team may have less work, you dump work on
the Debian GCC maintainers with a patch which is unmaintained upstream, and
which requires the compiler to be built with --disable-werror. Getting the
testsuite fixed took more than twelve months, and the changes to cleanly build
without --disable-werror are missing. The GCC packages don't "already carry
plenty of patches" of this style; you'll only see patches needed for packaging
or backports from upstream. I don't plan to have an exception for this patch
set. Please get the testsuite fixes accepted upstream, plus patches to build
without --disable-werror.

> - makes more work for dealing with warnings.
> Rebuttal: those warnings are there for a reason -- they can
> be real security issues, and should be fixed.

there are some functions in glibc which are questionably declared with the "warn
about unused result" attribute (fwrite*). This seems to force a programming
style which not everybody agrees with (having to check the return value after
each operation instead of checking errno later).

> - makes running Debian slower.
> Rebuttal: no, nothing supports this. The bulk of _FORTIFY_SOURCE
> is compile-time. Run-time checks, including those from
> -fstack-protector are just not measurable. The burden of
> evidence for anyone claiming this is on them. I'm not
> suggesting we turn on PIE; that option can be a problem.

no, the burden would be on you. You did claim the same for PIE without checking
yourself :-/ Please don't just check ix86.

> Inflammatory observation: Debian may be the single remaining major Linux
> distribution that does not use the stack protector and _FORTIFY_SOURCE
> when building its packages. I find this embarrassing. Check[6] for
> yourself.

The majority of distributions does turn on these options during package build
time, which IMO is the right thing to do. Debian should do the same. There's now
Raphael's new framework in place which makes the injection of macros in
dpkg-buildpackage in the environment obsolete.

Matthias

--
To UNSUBSCRIBE, email to debian-devel-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org

Samuel Thibault · External Since: May 08, 2009 Posts: 47

Ben Hutchings, le Sun 01 Nov 2009 19:06:59 +0000, a écrit :
> On Sun, 2009-11-01 at 19:53 +0100, Matthias Klose wrote:
> > On 25.10.2009 19:55, Kees Cook wrote:
> [...]
> > > - makes more work for dealing with warnings.
> > > Rebuttal: those warnings are there for a reason -- they can
> > > be real security issues, and should be fixed.
> >
> > there are some functions in glibc which are questionably declared with the "warn
> > about unused result" attribute (fwrite*). This seems to force a programming
> > style which not everybody agrees with (having to check the return value after
> > each operation instead of checking errno later).
> [...]
>
> In general you cannot rely on checking errno because it is not defined
> whether a successful operation clears it.

But you can clear it by hand before calling them.

Samuel

--
To UNSUBSCRIBE, email to debian-devel-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 774

On Sun, Nov 01, 2009 at 08:10:44PM +0100, Samuel Thibault wrote:
> Ben Hutchings, le Sun 01 Nov 2009 19:06:59 +0000, a écrit :
> > On Sun, 2009-11-01 at 19:53 +0100, Matthias Klose wrote:
> > > there are some functions in glibc which are questionably declared with the "warn
> > > about unused result" attribute (fwrite*). This seems to force a programming
> > > style which not everybody agrees with (having to check the return value after
> > > each operation instead of checking errno later).
> > In general you cannot rely on checking errno because it is not defined
> > whether a successful operation clears it.
> But you can clear it by hand before calling them.

No, you can't. The value of errno is only defined after a failed call.
It is undefined after a sucessfull call. You can see it as a special
return value.

Bastian

--
It is a human characteristic to love little animals, especially if
they're attractive in some way.
-- McCoy, "The Trouble with Tribbles", stardate 4525.6

--
To UNSUBSCRIBE, email to debian-devel-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org

Gabor Gombas · External Since: Mar 22, 2005 Posts: 154

On Sun, Nov 01, 2009 at 08:10:44PM +0100, Samuel Thibault wrote:

> > In general you cannot rely on checking errno because it is not defined
> > whether a successful operation clears it.
>
> But you can clear it by hand before calling them.

That's only true in some special cases; for example, SuSv3 says you
should manually clear errno before calling functions in <math.h> and you
should check errno to see if the function was successful. But otherwise,
even a successful operation may have called other operations internally
that have failed and thus have modified errno.

Gabor

--
---------------------------------------------------------
MTA SZTAKI Computer and Automation Research Institute
Hungarian Academy of Sciences
---------------------------------------------------------

--
To UNSUBSCRIBE, email to debian-devel-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org

Kees Cook · External Since: May 06, 2009 Posts: 13

Hi Matthias,

On Sun, Nov 01, 2009 at 07:53:20PM +0100, Matthias Klose wrote:
> On 25.10.2009 19:55, Kees Cook wrote:
> >I do not expect to reach consensus with all developers on this, so I'm
> >not sure who I need to convince to move it forward. (Perhaps just the
> >GCC maintainers?) Regardless, if you agree with this, please speak up.
> >I think it's very important that this change happens.
>
> Introducing hardening defaults is a project decision, but as you may
> know I don't support the current approach to enable these defaults.

Ah, sorry. I had thought you didn't support it due to resistance from the
rest of the Debian developer community (for which I was trying to clear
the way with this thread). It seems I need to convince you directly! Smile

> >Arguments against:
> > - makes the compiler's behavior different than stock compiler.
> > Rebuttal: honestly, I don't care -- it seems like such a
> > huge win for safety and is easy to debug. Debian
> > already carries plenty of patches anyway -- there
> > is no such thing as the "stock compiler".
>
> Honestly I do care. While the security team may have less work, you
> dump work on the Debian GCC maintainers with a patch which is
> unmaintained upstream, and which requires the compiler to be built

So you take issue with the effects on the gcc testsuite and the compiling
of gcc itself? I'm willing (as you know) to work on these pieces. I
didn't even consider suggesting this for Debian until I'd nailed down all
the testsuite fixes in Ubuntu's gcc.

> with --disable-werror. Getting the testsuite fixed took more than
> twelve months, and the changes to cleanly build without
> --disable-werror are missing.

I wasn't aware of the --disable-werror issue, I can go look at that. Yes,
the testsuite fixes took a long time; most of that was me coming up to
speed on the gcc packaging itself. Running individual tests was no
trivial exercise. Wink

> The GCC packages don't "already carry
> plenty of patches" of this style; you'll only see patches needed for
> packaging or backports from upstream. I don't plan to have an
> exception for this patch set. Please get the testsuite fixes
> accepted upstream, plus patches to build without --disable-werror.

So that I understand what your requirements would be:
- testsuite fixes upstream
- fixes for --disable-werror upstream
You're not implying that the defaults be made upstream -- I expect that to
be entirely impossible. There is at least one person I know of at Gentoo
that took my gcc defaults patchset and is trying to implement it as a
configure option to the gcc package. So far, upstreaming that hasn't gone
anywhere that I'm aware of.

> > - makes more work for dealing with warnings.
> > Rebuttal: those warnings are there for a reason -- they can
> > be real security issues, and should be fixed.
>
> there are some functions in glibc which are questionably declared
> with the "warn about unused result" attribute (fwrite*). This seems
> to force a programming style which not everybody agrees with (having
> to check the return value after each operation instead of checking
> errno later).

The most egregious of these were patched out of eglibc in Ubuntu.
We could do the same in Debian. As for the general problem, yes, you're
correct -- there is no way to disable the "warn unused" warnings in gcc.
Perhaps upstream would be interested in gaining that ability?

> > - makes running Debian slower.
> > Rebuttal: no, nothing supports this. The bulk of _FORTIFY_SOURCE
> > is compile-time. Run-time checks, including those from
> > -fstack-protector are just not measurable. The burden of
> > evidence for anyone claiming this is on them. I'm not
> > suggesting we turn on PIE; that option can be a problem.
>
> no, the burden would be on you. You did claim the same for PIE
> without checking yourself :-/ Please don't just check ix86.

Alone, I am not able to know all the workloads people try. This was
especially true when I was originally interested in PIE, and was unable
to create a workload that caused serious problems (but for which you
were very able to demonstrate).

An expanded version of my terse trolling might be: "Other people need to
provide testcases that demonstrate the performance problem." Without
that, it's not reasonable to show that something is broken -- I can only
do the testing of workloads I can imagine and have time to implement.

> >Inflammatory observation: Debian may be the single remaining major Linux
> >distribution that does not use the stack protector and _FORTIFY_SOURCE
> >when building its packages. I find this embarrassing. Check[6] for
> >yourself.
>
> The majority of distributions does turn on these options during
> package build time, which IMO is the right thing to do. Debian
> should do the same. There's now Raphael's new framework in place
> which makes the injection of macros in dpkg-buildpackage in the
> environment obsolete.

This would certainly be better than nothing, and better than the
hardening-wrapper package, but it would require that every package in
Debian be modified to respect external environments. Also, I think
having the compiler itself be hardened is the bigger win.

Out of curiosity, where can I and others find the documentation for the
dpkg-buildpackage environment framework? We should immediately add the
hardening options to it now for the packages that it will work on.

-Kees

--
Kees Cook @debian.org

--
To UNSUBSCRIBE, email to debian-devel-REQUEST RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org