Hi,
On Thu, 15 Dec 2011, Kees Cook wrote:
> > $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.
I believe it's cleaner because we're doing this from Dpkg::Vendor::Debian
so we're not within Dpkg::BuildFlags and we should not have to access
the internals of the object except through a well defined API.
> 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.
A word about the structure of the output is needed.
> @@ -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.
This can easily be misread. I understood that enabling bindnow also
enables relro. When in fact it's really that bindnow requires relro as a
pre-requisite.
> +=item $bf->features($area)
> +
> +Returns true if the given area is a known, and false otherwise. The only
> +recognized area is "hardening".
Somehow I think that the method name is not clear enough. Maybe
"has_features"? At least it would more consistent in:
if ($bf->has_features()) {
$bf->get_features()
}
> + }
> +
> + # 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
You added empty lines before each feature test except here.
> if ($use_feature{"fortify"}) {
> $flags->append("CPPFLAGS", "-D_FORTIFY_SOURCE=2");
> }
> - # Format
> +
> + # Format Security
> --- 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.
It should be something more descriptive like "--query-features",
"--get-features" or "--print-features". Because otherwise it really
sounds like a command to ask whether <area> is supported (because
"features" is a verb).
> +} 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";
Put the first "\n" at the end of the block and you might want to use the
ternary operator to be more concise:
$item .= $features{$feature} ? "yes\n" : "no\n";
Cheers,
--
Raphaël Hertzog ◈ Debian Developer
Pre-order a copy of the Debian Administrator's Handbook and help
liberate it: http://debian-handbook.info/liberation/
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
