Hi Raphael,
On Fri, Dec 09, 2011 at 12:02:21PM +0100, Raphael Hertzog wrote:
> On Thu, 08 Dec 2011, Kees Cook wrote:
> > This patch adds that ability, and lets the environment correctly adjust it:
> >
> > $ dpkg-buildflags --features hardening
> > -bindnow,+format,+fortify,-pie,+relro,+stackprotector
> >
> > $ DEB_HOST_ARCH=ia64 dpkg-buildflags --features hardening
> > -bindnow,+format,+fortify,-pie,-relro,-stackprotector
>
> This syntax is great for debian/rules because we want to be expressive in
> a single line but it's really not great for standardized output.
>
> I would suggest to follow the standard set for "update-alternatives
> --query" and to use the well known RFC-2822 header block:
Sure! I've adjusted the output now.
> > I'm obviously open to changing how this works, what the output looks
> > like, etc. I'm just interested in the behavior to query expected
> > hardening features.
>
> The set of features is also adjusted by the maintainer in debian/rules and
> this part can't easily be queried.
Right, and I'm not too worried about this yet.
> This limits the usefulness of this query feature.
It is a limitation, but it's not a large one in the face of answering the
question "what _should_ the feature set be for this combination of
arch/cpu/etc?" which is what both "hardening-check" and "lintian" need to
be useful for their checks.
> > --- a/scripts/Dpkg/BuildFlags.pm
> > +++ b/scripts/Dpkg/BuildFlags.pm
>
> You're extending the API, please bump the minor number in $VERSION.
Ah, yes. Done.
> > +=item $bf->get_features($area)
> > +
> > +Return the list of features enabled for the given area.
> > +
> > +=cut
>
> It's a hash reference that is returned. Keys are features identifiers and
> values are booleans indicating whether the feature is enabled or not.
Thanks, fixed the docs.
> You might want to return a hash so that the caller can't modify the
> content of $features.
Also a good idea; done.
> > +=item $bf->features($area)
> > +
> > +Returns a list of enabled features for a given area. Currently the
> > +only recognized area is "hardening".
>
> Bad description. The function returns true if the $area is known, and false
> otherwise.
Fixed.
> $flags->{'features'}{'hardening'} is mostly the same than %use_feature,
> please do not duplicate it but rather modify the code so that it works
> that way:
> 1/ generate %use_feature by directly taking into account the architecture
> specificities
> 2/ record %use_feature with a $flags->set_feature()
> 3/ do the various $flags->apppend() based on %use_feature (and there must
> be no supplementary conditions left in the tests at this point, it's
> solely "if ($use_feature{"foo"}) { $flags->append() }" since the
> supplementary conditions have been taken into account in 1/ already)
While doing this, it seemed that creating a full "set_feature()" callback
was more work than it needed to be. I can certainly add it, but I thought
I'd show you where I am now first. If you still want the set_feature()
calls, I can add those, but I think it'll make the code less readable.
> Please put the "else" on the same line than the preceding closing curly
> braces.
Done!
Thanks for the review! New patch attached...
-Kees
(and for my own reference, I ran local tests using:
DEB_VENDOR=Debian DPKG_DATADIR=/usr/share/dpkg perl -I scripts
./scripts/dpkg-buildflags.pl
)
--
Kees Cook @debian.org
>From 96f647889ad84ad16cf64ebc63365a7c3238177f Mon Sep 17 00:00:00 2001
From: Kees Cook <k...@outflux.net>
Date: Thu, 8 Dec 2011 15:53:14 -0800
Subject: [PATCH] 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 | 8 +++++-
scripts/Dpkg/BuildFlags.pm | 27 +++++++++++++++++-
scripts/Dpkg/Vendor/Debian.pm | 61 +++++++++++++++++++++++++++-------------
scripts/dpkg-buildflags.pl | 19 ++++++++++++-
4 files changed, 92 insertions(+), 23 deletions(-)
diff --git a/man/dpkg-buildflags.1 b/man/dpkg-buildflags.1
index a018edb..7bfbe46 100644
--- a/man/dpkg-buildflags.1
+++ b/man/dpkg-buildflags.1
@@ -87,6 +87,11 @@ the flag is set/modified by a user-specific configuration;
the flag is set/modified by an environment-specific configuration.
.RE
.TP
+.BI \-\-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.
+.TP
.B \-\-help
Show the usage message and exit.
.TP
@@ -239,7 +244,8 @@ 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). This option cannot be enabled without \fBrelro\fP being enabled
+at the same time.
.
.TP
.B pie
diff --git a/scripts/Dpkg/BuildFlags.pm b/scripts/Dpkg/BuildFlags.pm
index 6112a9f..09c3b81 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} = {
@@ -306,6 +307,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, $key) = @_;
+ return %{$self->{'features'}{$key}};
+}
+
=item $bf->get_origin($flag)
Return the origin associated to the flag. It might be undef if the
@@ -318,6 +331,18 @@ sub get_origin {
return $self->{'origin'}{$key};
}
+=item $bf->features($area)
+
+Returns true if the given area is a known, and false otherwise. The only
+recognized area is "hardening".
+
+=cut
+
+sub features {
+ my ($self, $key) = @_;
+ return exists $self->{'features'}{$key};
+}
+
=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..f946a99 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,9 @@ sub add_hardening_flags {
"relro" => 1,
"bindnow" => 0
);
+ $flags->{'features'}{'hardening'} = \%use_feature;
+
+ # 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,43 +115,61 @@ 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");
diff --git a/scripts/dpkg-buildflags.pl b/scripts/dpkg-buildflags.pl
index ee33961..cdfd74e 100755
--- a/scripts/dpkg-buildflags.pl
+++ b/scripts/dpkg-buildflags.pl
@@ -47,6 +47,7 @@ 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.
+ --features <area> output the enabled 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 +63,7 @@ my ($param, $action);
while (@ARGV) {
$_ = shift(@ARGV);
- if (m/^--(get|origin)$/) {
+ if (m/^--(get|origin|features)$/) {
usageerr(_g("two commands specified: --%s and --%s"), $1, $action)
if defined($action);
$action = $1;
@@ -115,6 +116,22 @@ if ($action eq "get") {
print $build_flags->get_origin($param) . "\n";
exit(0);
}
+} elsif ($action eq "features") {
+ if ($build_flags->features($param)) {
+ my %features = $build_flags->get_features($param);
+ my @report;
+ foreach my $feature (sort keys %features) {
+ my $item = "Feature: $feature\nEnabled: ";
+ if ($features{$feature} == 1) {
+ $item .= "yes";
+ } else {
+ $item .= "no";
+ }
+ push(@report, $item);
+ }
+ print join("\n\n", @report), "\n";
+ exit(0);
+ }
} elsif ($action =~ m/^export-(.*)$/) {
my $export_type = $1;
foreach my $flag ($build_flags->list()) {
--
1.7.5.4
