Re: [PATCH 2/3] Make static usermode helper binaries constant

[J. Bruce Fields]

On Tue, Jan 17, 2017 at 08:13:47AM +0100, Greg KH wrote:
> On Mon, Jan 16, 2017 at 04:25:55PM -0500, J. Bruce Fields wrote:
> > On Mon, Jan 16, 2017 at 05:50:31PM +0100, Greg KH wrote:
> > > From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > 
> > > There are a number of usermode helper binaries that are "hard coded" in
> > > the kernel today, so mark them as "const" to make it harder for someone
> > > to change where the variables point to.
> > > 
> > ...
> > > --- a/drivers/pnp/pnpbios/core.c
> > > +++ b/drivers/pnp/pnpbios/core.c
> > > @@ -98,6 +98,7 @@ static struct completion unload_sem;
> > >   */
> > >  static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> > >  {
> > > +	static char const sbin_pnpbios[] = "/sbin/pnpbios";
> > >  	char *argv[3], **envp, *buf, *scratch;
> > >  	int i = 0, value;
> > >  
> > > @@ -112,7 +113,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> > >  	 * integrated into the driver core and use the usual infrastructure
> > >  	 * like sysfs and uevents
> > >  	 */
> > > -	argv[0] = "/sbin/pnpbios";
> > > +	argv[0] = (char *)sbin_pnpbios;
> > 
> > So here and elsewhere, can attackers write to argv[0] instead of to the
> > memory where the string lives?
> 
> Yes, they could, it would be a very "tight" race to do that (have to
> write after the assignment and before the call_usermodehelper_exec()
> runs).  However, the kernel does not run argv[0], it just passes it to
> the binary you specify in path, so for this example, the correct program
> would still be run by the kernel.

In this case it's argv[0] that will be passed to call_usermodehelper as
path, but.... OK, this argv array and the various function call
arguments are all just data on the stack, so I guess it's all about
equivalent.

So we're assuming an attacker that can write to a static location in
memory but can't write to the right part of the stack at the right time.
I'm no expert at this kind of thing but it seems plausible that
assumption could apply in cases that matter.

> But, if you do worry about this type of attack, then enable the option I
> created in patch 3/3 here, which will funnel all calls into a single
> userspace binary where you can then filter on argv[0] to see if you want
> to run the binary or not to prevent this type of attack.
> 
> > Apologies if I'm rehashing earlier discussion, I did a quick search of
> > archives but could easily have missed something.
> 
> No problem at all, hopefully I've explained it better now.

Thanks!

--b.