Re: Hardening patch

[Guillem Jover <guillem@debian.org>]

On Wed, 2011-09-07 at 11:55:19 +0200, Raphael Hertzog wrote:
> Here's what I'm going to push in case anyone feels like reviewing it
> quickly (I'm waiting some final feedback from Kees).

Here it is.

> >From 8f1c8a783b35486c70f48969679090d77278665c Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Rapha=C3=ABl=20Hertzog?= <hertzog@debian.org>
> Date: Wed, 27 Jul 2011 22:10:49 +0200
> Subject: [PATCH 2/3] dpkg-buildflags: emit hardening build flags by default
> 
> All the hardening build flags supported by hardening-includes
> are supported except that PIE is not enabled by default (just like
> the corresponding gcc patch doesn't enable it by default).
> 
> Inspired by the work of Kees Cook <kees@debian.org>.
> ---
>  scripts/Dpkg/BuildFlags.pm |   89 ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 89 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/Dpkg/BuildFlags.pm b/scripts/Dpkg/BuildFlags.pm
> index 9bc473a..5479af0 100644
> --- a/scripts/Dpkg/BuildFlags.pm
> +++ b/scripts/Dpkg/BuildFlags.pm
> @@ -84,9 +89,93 @@ sub load_vendor_defaults {
>  	FFLAGS   => 'vendor',
>  	LDFLAGS  => 'vendor',
>      };
> +    $self->add_hardening_flags();
>      run_vendor_hook("update-buildflags", $self);
>  }

This seems too compiler, system and distribution specific to be set by
default for all BuildFlags users, I think it would make more sense to
split it into a different module, and enable it from the Debian vendor
hook, so that Ubuntu inherits it too.

> +=item $bf->add_hardening_flags()
> +
> +Add hardening build flags to the current set of flags. It's always called
> +by load_vendor_defaults().
> +
> +=cut
> +
> +sub add_hardening_flags {
> +    my ($self) = @_;
> +    my $arch = get_host_arch();
> +    my ($abi, $os, $cpu) = debarch_to_debtriplet($arch);
> +
> +    # Decide what's enabled
> +    my %use_feature = (
> +	"pie" => 0,
> +	"stackprotector" => 1,
> +	"fortify" => 1,
> +	"format" => 1,
> +	"relro" => 1,
> +	"bindnow" => 1
> +    );
> +    my $opts = Dpkg::BuildOptions->new(envvar => "DEB_BUILD_MAINT_OPTIONS");
> +    foreach my $feature (split(",", $opts->get("hardening") // "")) {
> +	$feature = lc($feature);
> +	if ($feature =~ s/^([+-])//) {
> +	    my $value = ($1 eq "+") ? 1 : 0;
> +	    if ($feature eq "all") {
> +		$use_feature{$_} = $value foreach keys %use_feature;
> +	    } else {
> +		if (exists $use_feature{$feature}) {
> +		    $use_feature{$feature} = $value;
> +		} else {
> +		    warning(_g("unknown hardening feature: %s"), $feature);
> +		}
> +	    }
> +	} else {
> +	    warning(_g("incorrect value in hardening option of " .
> +	               "DEB_BUILD_MAINT_OPTIONS: %s"), $feature);
> +	}
> +    }

How were you envisioning vendors to change the defaults?

Also I'm not sure now if this has been brought up before, but the
bindnow option might have noticable startup speed impact depending
on the amount of symbols and shared objects to resolve and load.
The other options seem sane in general.

> >From ccc609c3ff08dc0b1bdaadc432a23493664dfa6e Mon Sep 17 00:00:00 2001
> From: Kees Cook <kees@debian.org>
> Date: Mon, 5 Sep 2011 23:34:49 -0700
> Subject: [PATCH 3/3] dpkg-buildflags(1): add initial hardening documentation
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Document the various hardening options that can be enabled/disabled
> via DEB_BUILD_MAINT_OPTIONS.
> 
> Improved-by: Raphaël Hertzog <hertzog@debian.org>
> Signed-off-by: Kees Cook <kees@debian.org>
> Signed-off-by: Raphaël Hertzog <hertzog@debian.org>

All minus signs that can end up being copy&pasted into a runnable
command, etc. need to be escaped as in \- so that man does not turn
them into hyphens.

reagards,
guillem