Stefan Sperling <stsp@stsp.name> wrote:
> On Mon, Jul 04, 2022 at 10:27:13PM +0200, Omar Polo wrote:
> > 'got patch' is the only part of got that sends and receives messages
> > outside of privsep.c.  When i first implemented it I haven't realize
> > this.
> > 
> > I tried to minimize the changes as much as possible.  The only
> > difference is that recv_patch (now got_privsep_recv_patch) doesn't call
> > patch_free anymore, as i didn't want to export that function too.
> > Instead, we explicitly call it in the error path in patch.c, not a big
> > deal.  I flipped the order of the params for consistency with the other
> > got_privsep_* functions.
> > 
> > thoughts/ok?
> 
> There is an issue with the lib/privsep.c file. Its code is linked
> into all of our programs, including all of libexec. All this (machine)
> code could be mined for ROP gadgets when someone tries that particular
> avenue of writing an exploit.
> 
> At some point I did a sweep and moved code from privsep.c into individual
> libexec/ programs where only a single caller existed for a privsep function.
> What you are doing here is the inverse of that. Then again, right now there
> are more important problems to solve than making sure that privsep.c is as
> small as possible, and you might have reason to prefer the style that results
> from your refactoring. And I don't know if ROP on privsep.o is really a
> concern in practice. I have never bothered trying to run a ROP gadget miner
> on privsep.o to find out more (and results will vary by architecture...)

admittedly I haven't thought about it; when looking at zeroeing all the
structs sent via imsg I saw that all the imsg handling was in privsep.c
and so i thought of aligning patch.c too.

I don't mind keeping that stuff in patch.c if it's fine for you too, it
avoids an extra file for the struct definitions and keeps privsep.c
smaller.  It also reduces the build time when I have to tweak something
in the patch imsg handling :)

> So do we prefer a convenient place where all the imsg code is located, or
> should we try to trim the body of code that gets linked into libexec helpers
> down as much as possible? We probably cannot have both at the same time.
> I guess for security, a smaller attack surface in libexec is better. 

agreed.  if we want to have the libexec helpers even more small maybe we
could split privsep.c into multiple privsep_*.c files, so that the
libexec helpers can link only to the bits they actually need?

I thought that the linker would help with this, but at least by default
it doesn't.  I was surprised to find stuff like got_privsep_send_packfd
at first in got-read-patch :)

(well, probably for a good reason it can't help by default.)

I did a quick search and just for giggles tried to build got-read-patch
with LTO.  It significantly reduces the sizes of the helpers (patch went
from 229K to 54K, and once stripped down to 30K) and seemed to strip out
all the unused functions, but admittedly is something we can't use.  Was
fun to try thought, last time i played with -flto i had clang dying of
OOM :)

	% ls -lah got-read-patch
	-r-xr-xr-x  1 op  op  54.3K Jul  5 09:54 got-read-patch*
	% nm got-read-patch | grep got_privsep
	00006b90 t got_privsep_flush_imsg
	00006610 t got_privsep_recv_imsg
	000069e0 t got_privsep_send_error

> For just lib/patch.c itself, the difference doesn't matter. The main
> program has almost everything linked in anyway.

yup

I'm dropping the patch then, all it does is making the libexec worse by
providing unnecessary code to them.