Hi Guillem,
On Sun, Dec 18, 2011 at 09:42:50AM +0100, Guillem Jover wrote:
> On Fri, 2011-12-16 at 16:39:25 -0800, Kees Cook wrote:
> > Fresh patch attached! :)
>
> Thanks! Could you split the refactoring/cleaning into its own patch
> (actually something that already crossed my mind when first seeing
> the original buildflags code), and the new functionality into another
> one?
Sure! Hopefully I chose the right line for this split. I also split out the
documentation adjustment that was unrelated to these changes.
> > +.br
> > +
>
> To start a new paragraph use “.P”.
I had tried that, but it lost the indentation. It seems that I wanted
".IP", so I've switched to that.
> Use .nf / .fi here:
Ah-ha! Perfect. I've fixed this.
> > + --query-features <area>
> > + output the list of features for the given area,
> > + along with their enablement status, in RFC822 style.
>
> Just nitpicking, but I don't think we need the --help output to be
> that verbose. If it could be reworded to take one line less, great,
> otherwise I guess it's fine.
Sure; I've dropped the mention of RFC822 since that's documented in the
manpage sufficiently.
> > +} elsif ($action eq "query-features") {
> > + if ($build_flags->has_features($param)) {
> > + my %features = $build_flags->get_features($param);
> > + my @report;
> > + foreach my $feature (sort keys %features) {
> > + my $item = "Feature: $feature\nEnabled: ";
> > + $item .= $features{$feature} ? "yes\n" : "no\n";
> > + push(@report, $item);
> > + }
> > + print join("\n", @report);
>
> Hmm, why not just print as we go? The code would seem cleaner to me.
I had wanted to avoid the trailing black line, but it is harmless so I've
removed the array logic now.
Thanks for the review! New patches attached...
-Kees
--
Kees Cook @debian.org
>From d3f7d8c41bca70887c4e6a24ec8736a36b9da828 Mon Sep 17 00:00:00 2001
From: Kees Cook <k...@outflux.net>
Date: Wed, 28 Dec 2011 15:22:55 -0800
Subject: [PATCH 1/3] docs: clarify the relationship between relro/bindnow
Clarify the documentation about how bindnow will be forced off if relro
is not enabled or available.
Signed-off-by: Kees Cook <k...@debian.org>
---
man/dpkg-buildflags.1 | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/man/dpkg-buildflags.1 b/man/dpkg-buildflags.1
index a018edb..b86ae0d 100644
--- a/man/dpkg-buildflags.1
+++ b/man/dpkg-buildflags.1
@@ -231,7 +231,8 @@ This setting (enabled by default) adds
to \fBLDFLAGS\fP. During program load, several ELF memory sections need
to be written to by the linker. This flags the loader to turn these
sections read-only before turning over control to the program. Most
-notably this prevents GOT overwrite attacks.
+notably this prevents GOT overwrite attacks. If this option is disabled,
+\fBbindnow\fP will become disabled as well.
.
.TP
.B bindnow
@@ -239,7 +240,7 @@ This setting (disabled by default) adds
.B \-Wl,\-z,now
to \fBLDFLAGS\fP. During program load, all dynamic symbols are resolved,
allowing for the entire PLT to be marked read-only (due to \fBrelro\fP
-above).
+above). The option cannot become enabled if \fBrelro\fP is not enabled.
.
.TP
.B pie
--
1.7.5.4
>From 1d208333d42fc1606542d27346c16ad1b71b3de4 Mon Sep 17 00:00:00 2001
From: Kees Cook <k...@outflux.net>
Date: Wed, 28 Dec 2011 15:03:44 -0800
Subject: [PATCH 2/3] BuildFlags: Create feature interface
Refactor the hardened compiler flag logic to use a new "get/set/has
features" interface to BuildFlags.pm so as to communicate the current
logical state of the hardening feature.
Signed-off-by: Kees Cook <k...@debian.org>
---
scripts/Dpkg/BuildFlags.pm | 40 ++++++++++++++++++++++++-
scripts/Dpkg/Vendor/Debian.pm | 66 ++++++++++++++++++++++++++++------------
2 files changed, 85 insertions(+), 21 deletions(-)
diff --git a/scripts/Dpkg/BuildFlags.pm b/scripts/Dpkg/BuildFlags.pm
index 6112a9f..e832b39 100644
--- a/scripts/Dpkg/BuildFlags.pm
+++ b/scripts/Dpkg/BuildFlags.pm
@@ -18,7 +18,7 @@ package Dpkg::BuildFlags;
use strict;
use warnings;
-our $VERSION = "1.01";
+our $VERSION = "1.02";
use Dpkg::Gettext;
use Dpkg::BuildOptions;
@@ -68,6 +68,7 @@ sub load_vendor_defaults {
my ($self) = @_;
$self->{'options'} = {};
$self->{'source'} = {};
+ $self->{'features'} = {};
my $build_opts = Dpkg::BuildOptions->new();
my $default_flags = $build_opts->has("noopt") ? "-g -O0" : "-g -O2";
$self->{flags} = {
@@ -202,6 +203,19 @@ sub set {
$self->{origin}->{$flag} = $src if defined $src;
}
+=item $bf->set_feature($area, $feature, $enabled)
+
+Update the boolean state of whether a specific feature within a known
+feature area has been enabled. The only currently known feature area is
+"hardening".
+
+=cut
+
+sub set_feature {
+ my ($self, $area, $feature, $enabled) = @_;
+ $self->{features}->{$area}->{$feature} = !!$enabled;
+}
+
=item $bf->strip($flag, $value, $source)
Update the build flag $flag by stripping the flags listed in $value and
@@ -306,6 +320,18 @@ sub get {
return $self->{'flags'}{$key};
}
+=item $bf->get_features($area)
+
+Return, for the given area, a hash with keys as feature names, and values
+as booleans indicating whether the feature is enabled or not.
+
+=cut
+
+sub get_features {
+ my ($self, $area) = @_;
+ return %{$self->{'features'}{$area}};
+}
+
=item $bf->get_origin($flag)
Return the origin associated to the flag. It might be undef if the
@@ -318,6 +344,18 @@ sub get_origin {
return $self->{'origin'}{$key};
}
+=item $bf->has_features($area)
+
+Returns true if the given area of features is known, and false otherwise.
+The only currently recognized area is "hardening".
+
+=cut
+
+sub has_features {
+ my ($self, $area) = @_;
+ return exists $self->{'features'}{$area};
+}
+
=item $bf->has($option)
Returns a boolean indicating whether the flags exists in the object.
diff --git a/scripts/Dpkg/Vendor/Debian.pm b/scripts/Dpkg/Vendor/Debian.pm
index e824d0e..07935d9 100644
--- a/scripts/Dpkg/Vendor/Debian.pm
+++ b/scripts/Dpkg/Vendor/Debian.pm
@@ -83,7 +83,7 @@ sub add_hardening_flags {
my $arch = get_host_arch();
my ($abi, $os, $cpu) = debarch_to_debtriplet($arch);
- # Decide what's enabled
+ # Features enabled by default for all builds.
my %use_feature = (
"pie" => 0,
"stackprotector" => 1,
@@ -92,6 +92,8 @@ sub add_hardening_flags {
"relro" => 1,
"bindnow" => 0
);
+
+ # Adjust features based on Maintainer's desires.
my $opts = Dpkg::BuildOptions->new(envvar => "DEB_BUILD_MAINT_OPTIONS");
foreach my $feature (split(",", $opts->get("hardening") // "")) {
$feature = lc($feature);
@@ -112,47 +114,71 @@ sub add_hardening_flags {
}
}
- # PIE
- if ($use_feature{"pie"} and
- $os =~ /^(linux|knetbsd|hurd)$/ and
- $cpu !~ /^(hppa|m68k|mips|mipsel|avr32)$/) {
- # Only on linux/knetbsd/hurd (see #430455 and #586215)
+ # Mask features that are not available on certain architectures.
+ if ($os !~ /^(linux|knetbsd|hurd)$/ or
+ $cpu =~ /^(hppa|m68k|mips|mipsel|avr32)$/) {
+ # Disabled on non-linux/knetbsd/hurd (see #430455 and #586215).
# Disabled on hppa, m68k (#451192), mips/mipsel (#532821), avr32
- # (#574716)
- $flags->append("CFLAGS", "-fPIE");
- $flags->append("CXXFLAGS", "-fPIE");
- $flags->append("LDFLAGS", "-fPIE -pie");
+ # (#574716).
+ $use_feature{"pie"} = 0;
}
- # Stack protector
- if ($use_feature{"stackprotector"} and
- $cpu !~ /^(ia64|alpha|mips|mipsel|hppa)$/ and $arch ne "arm") {
+ if ($cpu =~ /^(ia64|alpha|mips|mipsel|hppa)$/ or $arch eq "arm") {
# Stack protector disabled on ia64, alpha, mips, mipsel, hppa.
# "warning: -fstack-protector not supported for this target"
# Stack protector disabled on arm (ok on armel).
# compiler supports it incorrectly (leads to SEGV)
+ $use_feature{"stackprotector"} = 0;
+ }
+ if ($cpu =~ /^(ia64|hppa|avr32)$/) {
+ # relro not implemented on ia64, hppa, avr32.
+ $use_feature{"relro"} = 0;
+ }
+
+ # Handle logical feature interactions.
+ if ($use_feature{"relro"} == 0) {
+ # Disable bindnow if relro is not enabled, since it has no
+ # hardening ability without relro and may incur load penalties.
+ $use_feature{"bindnow"} = 0;
+ }
+
+ # PIE
+ if ($use_feature{"pie"}) {
+ $flags->append("CFLAGS", "-fPIE");
+ $flags->append("CXXFLAGS", "-fPIE");
+ $flags->append("LDFLAGS", "-fPIE -pie");
+ }
+
+ # Stack protector
+ if ($use_feature{"stackprotector"}) {
$flags->append("CFLAGS", "-fstack-protector --param=ssp-buffer-size=4");
$flags->append("CXXFLAGS", "-fstack-protector --param=ssp-buffer-size=4");
}
- # Fortify
+
+ # Fortify Source
if ($use_feature{"fortify"}) {
$flags->append("CPPFLAGS", "-D_FORTIFY_SOURCE=2");
}
- # Format
+
+ # Format Security
if ($use_feature{"format"}) {
$flags->append("CFLAGS", "-Wformat -Wformat-security -Werror=format-security");
$flags->append("CXXFLAGS", "-Wformat -Wformat-security -Werror=format-security");
}
- # Relro
- if ($use_feature{"relro"} and $cpu !~ /^(ia64|hppa|avr32)$/) {
+
+ # Read-only Relocations
+ if ($use_feature{"relro"}) {
$flags->append("LDFLAGS", "-Wl,-z,relro");
- } else {
- # Disable full relro if relro is not enabled.
- $use_feature{"bindnow"} = 0;
}
+
# Bindnow
if ($use_feature{"bindnow"}) {
$flags->append("LDFLAGS", "-Wl,-z,now");
}
+
+ # Store the feature usage.
+ for (keys %use_feature) {
+ $flags->set_feature("hardening", $_, $use_feature{$_})
+ }
}
1;
--
1.7.5.4
>From c92970f2d45d3fc15a263a7125b63b18a696ae42 Mon Sep 17 00:00:00 2001
From: Kees Cook <k...@outflux.net>
Date: Thu, 8 Dec 2011 15:53:14 -0800
Subject: [PATCH 3/3] dpkg-buildflags: provide feature query ability
Since the logic for having a hardening flag enabled or disabled depends
on the architecture, and since the flags may change over time for each
hardening feature, there needs to be a way to externally query the state
of the hardening features. Specifically, lintian needs this to be able
to figure out if a binary package is missing expected hardening features.
Instead of maintaining multiple hard-coded lists of expected hardening
features, this makes dpkg-buildflags the canonical location of the
information, which can be queried by externally. (See bug 650536.)
Signed-off-by: Kees Cook <k...@debian.org>
---
man/dpkg-buildflags.1 | 16 ++++++++++++++++
scripts/dpkg-buildflags.pl | 13 ++++++++++++-
2 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/man/dpkg-buildflags.1 b/man/dpkg-buildflags.1
index b86ae0d..2b5fb2e 100644
--- a/man/dpkg-buildflags.1
+++ b/man/dpkg-buildflags.1
@@ -87,6 +87,22 @@ the flag is set/modified by a user-specific configuration;
the flag is set/modified by an environment-specific configuration.
.RE
.TP
+.BI \-\-query\-features " area"
+Print the features enabled for a given area. The only currently recognized
+area is \fBhardening\fP. Exits with 0 if the area is known otherwise exits
+with 1.
+.IP
+The output format is RFC822 header-style, with one section per feature.
+For example:
+.IP
+.nf
+ Feature: pie
+ Enabled: no
+
+ Feature: stackprotector
+ Enabled: yes
+.fi
+.TP
.B \-\-help
Show the usage message and exit.
.TP
diff --git a/scripts/dpkg-buildflags.pl b/scripts/dpkg-buildflags.pl
index ee33961..f273ca1 100755
--- a/scripts/dpkg-buildflags.pl
+++ b/scripts/dpkg-buildflags.pl
@@ -47,6 +47,8 @@ Actions:
--get <flag> output the requested flag to stdout.
--origin <flag> output the origin of the flag to stdout:
value is one of vendor, system, user, env.
+ --query-features <area>
+ output the status of features for the given area.
--list output a list of the flags supported by the current vendor.
--export=(sh|make|configure)
output something convenient to import the
@@ -62,7 +64,7 @@ my ($param, $action);
while (@ARGV) {
$_ = shift(@ARGV);
- if (m/^--(get|origin)$/) {
+ if (m/^--(get|origin|query-features)$/) {
usageerr(_g("two commands specified: --%s and --%s"), $1, $action)
if defined($action);
$action = $1;
@@ -115,6 +117,15 @@ if ($action eq "get") {
print $build_flags->get_origin($param) . "\n";
exit(0);
}
+} elsif ($action eq "query-features") {
+ if ($build_flags->has_features($param)) {
+ my %features = $build_flags->get_features($param);
+ foreach my $feature (sort keys %features) {
+ print "Feature: $feature\nEnabled: ";
+ print $features{$feature} ? "yes\n\n" : "no\n\n";
+ }
+ exit(0);
+ }
} elsif ($action =~ m/^export-(.*)$/) {
my $export_type = $1;
foreach my $flag ($build_flags->list()) {
--
1.7.5.4
