Hi, On Wed, 26 Jan 2022, Kees Cook wrote:
On Wed, Jan 26, 2022 at 03:13:10PM -0600, Ariadne Conill wrote:Looks good to me, but I wonder if we shouldn't set an argv of {bprm->filename, NULL} instead of {"", NULL}. Discussion in IRC led to the realization that multicall programs will try to use argv[0] and might crash in this scenario. If we're going to fake an argv, I guess we should try to do it right.They're crashing currently, though, yes? I think the goal is to move toward making execve(..., NULL, NULL) just not work at all. Using the {"", NULL} injection just gets us closer to protecting a bad userspace program. I think things _should_ crash if they try to start depending on this work-around.
Is there a reason to spawn a program, just to have it crash, rather than just denying it to begin with, though?
I mean, it all seems fine enough, and perhaps I'm just a bit picky on the colors and flavors of my bikesheds, so if you want to go with this patch, I'll be glad to carry it in the Alpine security update I am doing to make sure the *other* GLib-using SUID programs people find don't get exploited in the same way.
Ariadne
