Re: Enabling PKGSRC_MKPIE by default

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

			Hi Greg, tech-pkg@,

On 28/10/2017 16:05, Greg Troxel wrote:
> 
> Pierre Pronchery <khorben%defora.org@localhost> writes:
> 
>> The good news is that I just found a couple issues with PKGSRC_MKPIE in
>> the cwrappers, and could come up with a corresponding patch (attached).
>> While I let Joerg review it (as trivial as it seems to be), I would like
>> to ask if I can flip the switch once that patch committed, so that we
>> can find as much as possible of the remaining fallout soon, and 2017Q4
>> ships with PKGSRC_MKPIE enabled by default.
> 
> It may be approaching time (and definitely it's good to be away from the
> branch), but I think we need to pause for discussion and there are in my
> view too many loose ends (which I'd be very happy to see cleaned up).
> 
> So for now, I object.
> 
> With the variable abuse and documentation issues resolved, and a bit
> more information about testing, I expect to withdraw my objection.

What is the variable abuse? Too many knobs?

> A quick grep of PIE in pkgsrc/doc/pkgsrc.txt turns up nothing.  SSP and
> FORTIFY are similarly undocumented.  There was perhaps a notion that the

Sorry, I wasn't aware that these should be documented in there. I have
documented them in mk/defaults/mk.conf and on the wiki at
https://wiki.netbsd.org/pkgsrc/hardening/. I am still planning on
improving this wiki page, on which you gave me useful feedback already.

> documentation was coming in arrears, but I think we should have required
> that before enabling those by default.   Someone who really understands
> the details can explain this in not very many sentences, but the
> relationship of MKPIE and ASLR is not so obvious that "MKPIE turns on
> PIE!" would be adequate.

I have explained it briefly in the wiki page (I will add flesh) and in
mk/defaults/mk.conf, as well as in this e-mail (which you quoted away).

> My impression is that PKGSRC_MKPIE is a global user-settable variable to
> enable this, and you're talking about changing the value.  There doesn't
> seem to be a per-package variable to be set when enabling this breaks
> the package.  (I realize you may intend to fix all of those, but the
> history of pkgsrc is that some things get fixed and some don't; see
> MAKE_JOBS_SAFE for examples...)  I realize also that previous hardening

True, there is no such knob for PKGSRC_MKPIE, or for SSP or FORTIFY at
the moment. We just issued a full release without the need for this knob
for the last two though - with them enabled by default - and thus
avoiding a couple more variables.

> Would you be able to add this to the pkgsrc guide, explaining both the
> user-settable variable and the package-settable variable, including a
> few hints for packagers to tell when there's a problem caused by this?

Yes, I want to do that, and I am doing everything I can to please
everyone in the time that I have, but I cannot find every issue and do
and fix everything on my own either. Many of these tasks can be
distributed, and have been nicely distributed when SSP and FORTIFY were
enabled (special thanks to wiz@ there).

It is also fine to revert PIE if it happens to be too immature for the
release. But right now I honestly think it is ripe enough for a first shot.

Again, there is an important security risk to building with MKREPRO without

> I realize some find the xml unwieldy, but we have a history of someone
> trying to get it right and adding things, and others being happy to
> regen/fix as a team effort to improve our documentation.

Actually I do like DocBook XML - but everything takes time.

> Have you tested with and without cwrappers?  So far, both have to work.

I am trying to test it with and without cwrappers every time yes. I just
found one more issue with the cwrappers setup today, that I am letting
Joerg review as well (patch here for append_executables="-fPIC -pie" to
work).

> Do you mean that this is only active on NetBSD/{i386,amd64} >=8?  What
> happens on older versions of NetBSD?

To be more clear:

- Reproducible Builds are enabled by default in NetBSD 8 for the amd64
  and sparc64 architectures, thus making memory offsets the same for
  everyone in the base system there; (what old-school exploits rely on)
- thankfully, PaX ASLR is enabled by default in NetBSD 8 for the i386,
  amd64, sparc64, arm, landisk and pmax platforms, because it randomizes
  these memory offsets,
- ...but this can only work efficiently if applications are built
  position-independently (PIE), otherwise only the stack, the heap, and
  mmap() calls can be randomized;
- thankfully, pkgsrc knows how to enable PKGSRC_MKPIE, but only on
  NetBSD i386 and amd64 at the moment - but it is still ignored anywhere
  else.

So there is no fallout to be expected by enabling PKGSRC_MKPIE in the
current situation outside of NetBSD i386 and amd64. The next critical
architecture where to enable this is sparc64.

> How much testing has happened?  It sounds like you have built a lot of
> packages, and I've seen the commits.  Can you explain how many on i386
> and amd64?  On NetBSD 6, 7, 8, -current?  Did you run "make test" on
> those?

In my previous run on pkgsrc-2017Q1 I built 3000+ packages for
NetBSD/amd64. I did not do any bulk builds or any other architecture,
due to lack of spare powerful hardware, time, my electricity bill, and
pbulk not working for some reason. I just fixed quite a few issues on my
clone of pkgsrc-2017Q3, that I am committing on -current after review.

I admit that I usually do not run "make test", for the same reason that
I cannot build 17000+ packages on every platform after every tentative
change. I do use PKG_DEVELOPER=yes though, and I am also trying to add
more checks. I saw that jperkin@ contributed one for SSP (thanks!)

Cheers,
-- 
khorben

Index: files/bin/common.c
===================================================================
RCS file: /cvsroot/pkgsrc/pkgtools/cwrappers/files/bin/common.c,v
retrieving revision 1.7
diff -p -u -r1.7 common.c
--- files/bin/common.c	11 Jun 2017 19:34:43 -0000	1.7
+++ files/bin/common.c	28 Oct 2017 14:13:02 -0000
@@ -275,9 +275,14 @@ parse_config(const char *wrapper)
 			TAILQ_INSERT_TAIL(&prepend_executable_args, arg, link);
 		}
 		if (strncmp(line, "append_executable=", 18) == 0) {
+			char *last, *p;
+			unsigned int i;
 			struct argument *arg;
-			arg = argument_copy(line + 18);
-			TAILQ_INSERT_TAIL(&append_executable_args, arg, link);
+			for (p = strtok_r(line + 18, " ", &last), i = 0; p;
+					(p = strtok_r(NULL, " ", &last)), i++) {
+				arg = argument_copy(p);
+				TAILQ_INSERT_TAIL(&append_executable_args, arg, link);
+			}
 		}
 		if (strncmp(line, "prepend_shared=", 15) == 0) {
 			struct argument *arg;