Bug#687704: speex: Hardening flags missing
On Mon, Sep 17, 2012 at 11:26:44PM +0930, Ron wrote: The following flag is missing: -Werror=format-security Uh. That's not a hardening option. That's road spikes for people who blindly applied dpkg-buildflags and didn't actually bother to look at their build logs ... It's not really a hardening flag, true. But it prevents oversights and I think it can't hurt to add it. The patch removes the manually added flags, but uses dpkg-buildflags which automatically applies all the default hardening flags. ... which provides a lesser degree of checking than what the existing options enable. And makes it far more difficult to know for certain what any individual build might actually use on a given system. And a greater burden to track how it might silently change over time. On the other hand it doesn't require you as maintainer to track the support of certain hardening flags on different architectures in all your packages. [snip] It was a mistake for dpkg to start messing with package build options, and it's only slightly less of a mistake to let some other external tool be doing that too (the slightly less part being it's more obvious how (and the default) to opt out). I don't think it was a mistake. It enables a basic set of flags for all packages without too much effort for the maintainer. Of course it would be better if all maintainers would carefully take care of hardening their package, but at least it enables basic hardening for many packages. The hardening flags that dpkg-buildflags provides were cargo-culted from the rules and exceptions in the hardening-wrapper package, without paying any attention to (or fixing) how badly out of date some of those exceptions now are. Despite there being comments and references there which should have made that quite obvious to even a casual enquirer. I'm curious. What hardening flags do you consider obsolete or missing? [snip] I'm sorry if that doesn't really fit with your idea of just close your eyes and let other people take care of it here. But there really was That's not my idea. I just proposed these changes because I thought they would make it easier for you as maintainer - and to enable all the default hardening flags set by dpkg-buildflags. [snip] If there are real bugs in that, or valuable options that we might be missing, then I'd love to hear about that. But I don't really see any value in throwing away the work done to make these decisions and just leaving it to the whims of an external agent, which almost certainly will not test its changes against this package. Does that make sense? It does. I can understand why you want to have control of all the flags used in the build and I didn't want to push my patch onto you. I just thought it was the simplest way to get all the hardening flags, but I get your point of manual control. Regards, Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 pgp4zXN4btsv9.pgp Description: PGP signature
Bug#687704: speex: Hardening flags missing
On Sun, Sep 16, 2012 at 07:03:27PM +0200, Simon Ruderich wrote: On Sun, Sep 16, 2012 at 08:10:12PM +0930, Ron wrote: On Sat, Sep 15, 2012 at 12:53:53PM +0200, Simon Ruderich wrote: Some hardening flags (format flags and relro on some archs) are still missing because they are not set in debian/rules. Do you have some actual evidence of that? Of course. Either look at the build logs on your own, or use a tool like blhc to check the build logs. Yes, I read the build logs fairly carefully on my own when I first added these flags. There's not a lot of point enabling additional compiler warnings if you aren't going to actually look to see if they were emitted ... The following flag is missing: -Werror=format-security Uh. That's not a hardening option. That's road spikes for people who blindly applied dpkg-buildflags and didn't actually bother to look at their build logs ... I made a mistake about the relro flags, that was a false positive, they are applied correctly. Sorry for that. Since the patch you attached _removes_ the lines that set these things from rules - I'd have thought the obvious incorrectness of that statement would be obvious. The patch removes the manually added flags, but uses dpkg-buildflags which automatically applies all the default hardening flags. ... which provides a lesser degree of checking than what the existing options enable. And makes it far more difficult to know for certain what any individual build might actually use on a given system. And a greater burden to track how it might silently change over time. The general consensus (see [1], and the links in my original post) is to use dpkg-buildflags as global institution which decides which flags to use by default (and to handle platform specific flags in one place, and not in each package), that includes hardening flags. I'm pretty sure that general consensus isn't arrived at by someone writing their personal preference that people use the new tool they wrote on a wiki page ... Especially when even that page sort of grudgingly half-mentions there are other ways to do this too. It was a mistake for dpkg to start messing with package build options, and it's only slightly less of a mistake to let some other external tool be doing that too (the slightly less part being it's more obvious how (and the default) to opt out). The hardening flags that dpkg-buildflags provides were cargo-culted from the rules and exceptions in the hardening-wrapper package, without paying any attention to (or fixing) how badly out of date some of those exceptions now are. Despite there being comments and references there which should have made that quite obvious to even a casual enquirer. That doesn't really make me have more confidence in it than in the job that I and upstream can do wrt selecting the best set of build options for a particular package to use. I'm sorry if that doesn't really fit with your idea of just close your eyes and let other people take care of it here. But there really was some thought that went into deciding what options were appropriate for this package (and even benchmarks done on options we worried might be more expensive than the protection they added was worth). I'd really rather that it actually remain perfectly clear that the options being used were a conscious and informed selection - and that the history of the package should show how and when things related to that changed. If there are real bugs in that, or valuable options that we might be missing, then I'd love to hear about that. But I don't really see any value in throwing away the work done to make these decisions and just leaving it to the whims of an external agent, which almost certainly will not test its changes against this package. Does that make sense? Cheers, Ron -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#687704: speex: Hardening flags missing
On Sun, Sep 16, 2012 at 08:10:12PM +0930, Ron wrote:
On Sat, Sep 15, 2012 at 12:53:53PM +0200, Simon Ruderich wrote:
Some hardening flags (format flags and relro on some archs) are
still missing because they are not set in debian/rules.
Do you have some actual evidence of that?
Hello Ron,
Of course. Either look at the build logs on your own, or use a
tool like blhc to check the build logs. The following flag is
missing:
-Werror=format-security
I don't want to paste the result here because it's just a long
list of compiler commands each with the flag missing.
I made a mistake about the relro flags, that was a false
positive, they are applied correctly. Sorry for that.
Since the patch you attached _removes_ the lines that set these
things from rules - I'd have thought the obvious incorrectness
of that statement would be obvious.
The patch removes the manually added flags, but uses
dpkg-buildflags which automatically applies all the default
hardening flags.
[snip]
The attached patch would appear to do nothing whatsoever except
make it entirely opaque as to what hardening flags will be applied.
(and if applied to some of my hardened packages, would actually
_remove_ some hardening flags that are presently applied too)
The general consensus (see [1], and the links in my original
post) is to use dpkg-buildflags as global institution which
decides which flags to use by default (and to handle platform
specific flags in one place, and not in each package), that
includes hardening flags.
You're right, I've missed one flag, sorry for that. I forgot to
re-add -Wformat=2. But all other flags are applied just as
before.
This automatically takes care of old versions of dpkg-buildpackage
setting different flags, handling noopt and architectures which
don't support certain hardening flags (e.g. relro). -g and -O2
are also added by default (-O0 with noopt).
Things which were already handled by the code your patch removes ...
True, but dpkg-buildflags handles that for you automatically (man
dpkg-buildflags | less -p noopt), no need to duplicate that in
each package.
[snip]
If there is something really missing or broken (aside from a false
positive from hardening-check), then please do explain clearly what
that is. Otherwise I see no bug here at all, and if there is one,
the patch that was attached wouldn't appear to fix it anyway.
I admit there was a false positive in blhc (it didn't accept
-Wformat=2 as -Wformat) - which I used to check the logs, and I
failed to look close enough. But the -Werror=format-security flag
is missing and should be added.
Using the flags from dpkg-buildflags makes automatic checks
easier to spot packages with missing flags - and reduces the
burden of you as maintainer to maintain the platform specific
checks.
And it allows other users to rebuild the package with different
flags without modifying it, they can just use dpkg-buildflags to
change the build flags (man dpkg-buildflags | less -p
'^ENVIRONMENT').
Ron
[We could possibly make speex{enc,dec} pie, but it's not clear that
they have an attack surface which really makes that worth the effort
of patching upstream - and dpkg-buildflags isn't helpful for that
job either ... If we're going to worry about that, then these flags
should all be supplied by the upstream build itself and it dropped
from the rules file entirely.]
Regards,
Simon
[1]: https://wiki.debian.org/HardeningWalkthrough
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
pgpoXiUADOg1O.pgp
Description: PGP signature
Bug#687704: speex: Hardening flags missing
Package: speex
Version: 1.2~rc1-6
Severity: normal
Tags: patch
Dear Maintainer,
Some hardening flags (format flags and relro on some archs) are
still missing because they are not set in debian/rules. For more
hardening information please have a look at [1], [2] and [3].
The attached patch fixes the issue by using dpkg-buildflags to
set the default flags. This automatically takes care of old
versions of dpkg-buildpackage setting different flags, handling
noopt and architectures which don't support certain hardening
flags (e.g. relro). -g and -O2 are also added by default (-O0
with noopt). And by using dpkg-buildflags future (hardening)
flags will be automatically added.
To check if all flags were correctly enabled you can use
`hardening-check` from the hardening-includes package and check
the build log with `blhc` (hardening-check doesn't catch
everything):
$ hardening-check /usr/bin/speexenc /usr/bin/speexdec
/usr/lib/x86_64-linux-gnu/libspeexdsp.so.1.5.0
/usr/lib/x86_64-linux-gnu/libspeex.so.1.5.0 ...
/usr/bin/speexenc:
Position Independent Executable: no, normal executable!
Stack protected: yes
Fortify Source functions: yes (some protected functions found)
Read-only relocations: yes
Immediate binding: yes
/usr/bin/speexdec:
Position Independent Executable: no, normal executable!
Stack protected: yes
Fortify Source functions: yes (some protected functions found)
Read-only relocations: yes
Immediate binding: yes
/usr/lib/x86_64-linux-gnu/libspeexdsp.so.1.5.0:
Position Independent Executable: no, regular shared library (ignored)
Stack protected: no, not found!
Fortify Source functions: yes (some protected functions found)
Read-only relocations: yes
Immediate binding: yes
/usr/lib/x86_64-linux-gnu/libspeex.so.1.5.0:
Position Independent Executable: no, regular shared library (ignored)
Stack protected: no, not found!
Fortify Source functions: yes (some protected functions found)
Read-only relocations: yes
Immediate binding: yes
...
(Position Independent Executable is not enabled by default.)
Use find -type f \( -executable -o -name \*.so\* \) -exec
hardening-check {} + on the build result to check all files.
Regards,
Simon
[1]: https://wiki.debian.org/ReleaseGoals/SecurityHardeningBuildFlags
[2]: https://wiki.debian.org/HardeningWalkthrough
[3]: https://wiki.debian.org/Hardening
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
diff -u speex-1.2~rc1/debian/rules speex-1.2~rc1/debian/rules
--- speex-1.2~rc1/debian/rules
+++ speex-1.2~rc1/debian/rules
@@ -18,31 +18,14 @@
DEB_HOST_ARCH ?= $(shell dpkg-architecture -qDEB_HOST_ARCH)
DEB_HOST_MULTIARCH ?= $(shell dpkg-architecture -qDEB_HOST_MULTIARCH)
+# dpkg-buildflags takes care of hardening flags, respects noopt and prevents
+# old versions of dpkg-buildpackage to interfere with the default flags.
+dpkg_buildflags = DEB_BUILD_MAINT_OPTIONS=hardening=+all,-pie dpkg-buildflags
+CPPFLAGS = $(shell $(dpkg_buildflags) --get CPPFLAGS)
+CFLAGS = $(shell $(dpkg_buildflags) --get CFLAGS) -Wall
+CXXFLAGS = $(shell $(dpkg_buildflags) --get CXXFLAGS)
+LDFLAGS = $(shell $(dpkg_buildflags) --get LDFLAGS)
-HARD_CPPFLAGS = -D_FORTIFY_SOURCE=2
-HARD_CFLAGS = -Wformat=2
-HARD_LDFLAGS = -z now
-
-ifneq (,$(filter-out $(DEB_HOST_ARCH), ia64 alpha mips mipsel hppa arm))
- HARD_CFLAGS += -fstack-protector --param ssp-buffer-size=4
-endif
-ifneq (,$(filter-out $(DEB_HOST_ARCH), ia64 hppa avr32))
- HARD_LDFLAGS += -z relro
-endif
-
-# Keep dpkg-buildpackage the hell out of messing with our compile flags,
-# we should trust upstream to know better than it what to use here.
-# We explicitly re-add -g and -O2 here, since not all configurations do
-# set it explicitly (and instead rely on autoconf's default of doing that,
-# which we override here when we set the hardening flags, if we do).
-CPPFLAGS = $(HARD_CPPFLAGS)
-CFLAGS = $(HARD_CFLAGS) -g -O2
-CXXFLAGS = $(HARD_CFLAGS) -g -O2
-LDFLAGS = $(HARD_LDFLAGS)
-
-ifneq (,$(findstring noopt,$(DEB_BUILD_OPTIONS)))
- CFLAGS = -Wall -g -O0
-endif
ifeq (,$(findstring nostrip,$(DEB_BUILD_OPTIONS)))
INSTALL_PROGRAM += -s
endif
signature.asc
Description: Digital signature
