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...)

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. 

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