tech-pkg archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Improving security for pkgsrc

Hash: SHA1

			Hi tech-pkg@,

On 07/23/15 14:47, Greg Troxel wrote:
> Pierre Pronchery <> 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.


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 that add CFLAGS and remove LDFLAGS aren't 
> immediately understandable, but you didn't include the commit
> message. If this is fixing 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.

- -- 
Version: GnuPG v1

commit b20b89b465eb27c3458c8631ddff4ee1ec6f8fef
Author: Pierre Pronchery <>
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/ b/mk/compiler/
index eb7b925..adbadc2 100644
--- a/mk/compiler/
+++ b/mk/compiler/
@@ -67,7 +67,7 @@ _DEF_VARS.gcc=	\
@@ -336,6 +336,8 @@	-std=gnu99
 CFLAGS+=	-Wno-import
 .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
+# Set this to YES to enable stack-smashing protection (on supported platforms).
+# Possible: yes, no
+# Default: no
 .if (!empty(MACHINE_PLATFORM:MNetBSD-*-*) && \
diff --git a/mk/platform/ b/mk/platform/
index 91940de..4f37189 100644
--- a/mk/platform/
+++ b/mk/platform/
@@ -133,6 +133,16 @@ FFLAGS+=	-mieee
 PKG_HAVE_KQUEUE=	# defined
+.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
 _OPSYS_CAN_CHECK_SHLIBS=	yes # use readelf in check/
 # check for maximum command line length and set it in configure's environment,

Home | Main Index | Thread Index | Old Index