Re: Improving security for pkgsrc

[Pierre Pronchery <khorben%defora.org@localhost>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

			Hi tech-pkg@,

On 07/23/15 14:47, Greg Troxel wrote:
> Pierre Pronchery <khorben%defora.org@localhost> writes:
>> On 07/23/15 07:23, David Holland wrote:
>>> On Wed, Jul 22, 2015 at 07:25:08PM -0700, Alistair Crooks
>>> wrote:
>>>> It might be better, if this is just for pkgsrc, to call the
>>>> definition PKGSRC_USE_SSP.
> 
> I had the same reaction as agc (but without a specific name).

Ok, I have updated the patch with this name instead.

> That can be viewed as a bug; namespacing for config variables feels
> like collection of historical artifacts..  I have found it
> confusing meaning, had to actually go look things up) about which
> options are or base and which are for pkgsrc.

I agree here, especially when some of these options could have the
same name and meaning for both projects. With this said, then we
should probably also use USE_FORT here. If it makes more sense this
way, I will be glad to try to achieve this here.

> Generally, we declare each variable to be user-settable in mk.conf
> or pkg-settable in Makefile, and never both.  See the FETCH_USING 
> flamage...

Good point; I do think here it really makes sense to support both
though, just like in NetBSD's base system. And even then, packages
could set "PKGSRC_USE_SSP?=yes" and then the global setting would take
precedence always if set explicitly.

>> - it can easily be applied to pkgsrc only, in mk.conf: .ifdef
>> BSD_PKG_MK USE_SSP=yes .endif - unprivileged builds will use a
>> different mk.conf anyway.
> 
> That's true, but a) a person with USE_SSP in base will now get SSP
> in pgksrc, which may break a lot of things, and b) I think it's
> better in general to have separate names.

a)
.elif
USE_SSP=yes
.endif

But yes, it takes knowing about "BSD_PKG_MK" being specific to pkgsrc,
and although present in some of the documentation or sample
configuration files, it is not obvious.

b) My personal take on this is:
- - it finds bugs (which is a good thing)
- - breaking is fail-safe (likewise)

> On the patch, I agree with the concept, but have a few questions
> 
> I wonder if PKGSRC_USE_SSP should provoke an error if used with a 
> compiler that doesn't support it, rather than silently failing.
> Or perhaps a warning; an error seems too much.  But overall I think
> I agree with how you handled it.

My personal opinion is that it should trigger an error, and certainly
not silently ignore it. It may not be the most practical answer for
our users - but certainly the most secure.

We could proceed in steps:
1. introducing the feature (disabled by default)
2a. adventurous people/projects (EdgeBSD...) enable it by default and
    report/fix failures
2b. support gets added for more platforms
3. enabling by default on NetBSD/gcc (possibly also clang), possibly
   partially (like for base)
4. fail if enabled but not supported for the current platform

> The hunks in gcc.mk that add CFLAGS and remove LDFLAGS aren't 
> immediately understandable, but you didn't include the commit
> message. If this is fixing gcc.mk for an issue that's separate from
> adding SSP (and it seems it must be), I (with pmc hat OFF) would
> tend to want to see a separate commit for that.    If it's just
> that nothing sets _GCC_LDFLAGS now, I don't see why the LDFLAGS+=
> line should be removed.

The change about LDFLAGS was not related, and I have removed it
(thanks for noticing). I could not find anything that sets _GCC_CFLAGS
at the moment, but found prior art with _GCC_LDFLAGS and re-used the
same mechanism for this CFLAGS variable (being specific to NetBSD/gcc
at the moment).

I am attaching an updated version of the patch here.

HTH,
- -- 
khorben
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJVtBaqAAoJEDA4y9uYhpcDnsEP/3SdESuzhBvofaYr1oqKl9di
YGdiZwa9hKy2nfB+UdRQdG9ddK+9+aDo08/ssjKnh2nTFEWMbPGIsshhmkYrxYjq
V/kZR/LIzK4iOVGrmOVW023H/GAhXVJC7EWh4eGXod0DXCK/GmGN4QDz9tKflc7C
ywSmgAHlujdG6kHQUtYgTsNdJ4X/5CjQbvhY7sPbt3hrokT1k4oGU5JU5tQyZBDy
789zZb+ZP/eMi/KRv/JyhWeH2zB02B7A7fVxbzjpvh/jWpQmKNduXfQms1jKn/Tf
iJh/POpcpjI8gPjmiB3z3eMzNIBNhjCuYZT8tKq2+hW/bjRoXZOUXcEff27x3t9U
342VKt9Mf69y0GuGZt5iwUhYvQQlI6ednNXRFRtJ1zQlSSfG2UsqktCdSCkMt/7e
6GXCH1yafywQ7U1+LWgcIxjXwAYroadsZlQt9sY+tPgikCgmInQDg85obGdHrPkU
+omu5x1X1YJ6SbUFp7wkWpsFShKBKD4mGoSP4gFgsHtkMuBK4Yu/SuecxjerTs/H
qa1vqQWimpPS+xNQHOWirkalt+Ve9bpTckGvmY0fSUxYHKJEG8Aa8XyjIKlvKcQq
b7etaB/SHm7izh9RwRD7KzHUMfJrfKtbXBAbSxFE/10EyusBk+TyGrjrUzFtWvB1
kUAl+RinBvU6weP0MKyg
=enP8
-----END PGP SIGNATURE-----

commit b20b89b465eb27c3458c8631ddff4ee1ec6f8fef
Author: Pierre Pronchery <khorben%defora.org@localhost>
Date:   Thu Jul 16 20:30:47 2015 +0200

    Add support for compiling with stack-smashing protection
    
    This is enabled with PKGSRC_USE_SSP in mk.conf(5), as documented here.
    Most NetBSD platforms are supported (when compiling with gcc).

diff --git a/mk/compiler/gcc.mk b/mk/compiler/gcc.mk
index eb7b925..adbadc2 100644
--- a/mk/compiler/gcc.mk
+++ b/mk/compiler/gcc.mk
@@ -67,7 +67,7 @@ _DEF_VARS.gcc=	\
 	PKG_CC PKG_CPP PKG_CXX PKG_FC \
 	PKG_ADA PKG_GMK PKG_GLK PKG_GDB PKG_CHP PKG_GLK PKG_GNT PKG_PRP \
 	_CC _COMPILER_RPATH_FLAG _COMPILER_STRIP_VARS \
-	_GCCBINDIR _GCC_ARCHDIR _GCC_BIN_PREFIX _GCC_CC \
+	_GCCBINDIR _GCC_ARCHDIR _GCC_BIN_PREFIX _GCC_CC _GCC_CFLAGS \
 	_GCC_CPP _GCC_CXX _GCC_DEPENDENCY _GCC_DEPENDS \
 	_GCC_FC _GCC_LDFLAGS _GCC_LIBDIRS _GCC_PKG \
 	_GCC_PKGBASE _GCC_PKGSRCDIR _GCC_PKG_SATISFIES_DEP \
@@ -336,6 +336,8 @@ CWRAPPERS_APPEND.cc+=	-std=gnu99
 CFLAGS+=	-Wno-import
 .endif
 
+CFLAGS+=	${_GCC_CFLAGS}
+
 .if !empty(_NEED_GCC2:M[yY][eE][sS])
 #
 # We require gcc-2.x in the lang/gcc directory.
diff --git a/mk/defaults/mk.conf b/mk/defaults/mk.conf
index 52ddc1b..d0162e3 100644
--- a/mk/defaults/mk.conf
+++ b/mk/defaults/mk.conf
@@ -215,6 +215,11 @@ PKGSRC_RUN_TEST?=	no
 # Possible: yes, no
 # Default: no
 
+PKGSRC_USE_SSP?= no
+# Set this to YES to enable stack-smashing protection (on supported platforms).
+# Possible: yes, no
+# Default: no
+
 .if (!empty(MACHINE_PLATFORM:MNetBSD-*-*) && \
      exists(/usr/X11R7/lib/libX11.so))
 PREFER_PKGSRC?=
diff --git a/mk/platform/NetBSD.mk b/mk/platform/NetBSD.mk
index 91940de..4f37189 100644
--- a/mk/platform/NetBSD.mk
+++ b/mk/platform/NetBSD.mk
@@ -133,6 +133,16 @@ FFLAGS+=	-mieee
 PKG_HAVE_KQUEUE=	# defined
 .endif
 
+.if (${MACHINE_ARCH} != "alpha") && \
+	(${MACHINE_ARCH} != "hppa") && \
+	(${MACHINE_ARCH} != "ia64") && \
+	(${MACHINE_ARCH} != "mips")
+. if ${PKGSRC_USE_SSP:Uno} != "no"
+# build with stack protection (with GCC)
+_GCC_CFLAGS+=	-fstack-protector
+. endif
+.endif
+
 _OPSYS_CAN_CHECK_SHLIBS=	yes # use readelf in check/bsd.check-vars.mk
 
 # check for maximum command line length and set it in configure's environment,