Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560075 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/3] Make static usermode helper binaries constant "J. Bruce Fields" <bfields@fieldses.org> - 2017-01-16 22:30 +0100
    Re: [PATCH 2/3] Make static usermode helper binaries constant Greg KH <gregkh@linuxfoundation.org> - 2017-01-17 08:20 +0100
      Re: [PATCH 2/3] Make static usermode helper binaries constant "J. Bruce Fields" <bfields@fieldses.org> - 2017-01-17 16:20 +0100
        Re: [PATCH 2/3] Make static usermode helper binaries constant Greg KH <gregkh@linuxfoundation.org> - 2017-01-17 16:30 +0100
          Re: [kernel-hardening] Re: [PATCH 2/3] Make static usermode helper  binaries constant Greg KH <gregkh@linuxfoundation.org> - 2017-01-19 13:10 +0100
            Re: [kernel-hardening] Re: [PATCH 2/3] Make static usermode helper  binaries constant "J. Bruce Fields" <bfields@fieldses.org> - 2017-01-19 17:30 +0100

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

On Mon, Jan 16, 2017 at 05:50:31PM +0100, Greg KH wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> 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?

Apologies if I'm rehashing earlier discussion, I did a quick search of
archives but could easily have missed something.

--b.

[toc] | [next] | [standalone]

#1560289

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@linuxfoundation.org>
> > 
> > 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.

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,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1560692

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@linuxfoundation.org>
> > > 
> > > 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.

[toc] | [prev] | [next] | [standalone]

#1560711

On Tue, Jan 17, 2017 at 10:19:11AM -0500, J. Bruce Fields wrote:
> 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@linuxfoundation.org>
> > > > 
> > > > 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.

Kind of, nice catch, I'll change the call to usermodehelper to use
sbin_pnpbios here, as that's the right thing to do.

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

And again, if you really are worried about this, just use the new
kconfig option that allows you to filter all of this in userspace :)

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1562663 — Re: [kernel-hardening] Re: [PATCH 2/3] Make static usermode helper binaries constant

On Tue, Jan 17, 2017 at 04:29:19PM +0100, Greg KH wrote:
> On Tue, Jan 17, 2017 at 10:19:11AM -0500, J. Bruce Fields wrote:
> > 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@linuxfoundation.org>
> > > > > 
> > > > > 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.
> 
> Kind of, nice catch, I'll change the call to usermodehelper to use
> sbin_pnpbios here, as that's the right thing to do.

Oops, no, the patch was doing the right thing here, you missed the next
chunk of the patch:


@@ -139,7 +140,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
                           info->location_id, info->serial, info->capabilities);
        envp[i] = NULL;

-       value = call_usermodehelper(argv [0], argv, envp, UMH_WAIT_EXEC);
+       value = call_usermodehelper(sbin_pnpbios, argv, envp, UMH_WAIT_EXEC);
        kfree(buf);
        kfree(envp);
        return 0;


So it's ok.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1562873 — Re: [kernel-hardening] Re: [PATCH 2/3] Make static usermode helper binaries constant

On Thu, Jan 19, 2017 at 01:03:21PM +0100, Greg KH wrote:
> On Tue, Jan 17, 2017 at 04:29:19PM +0100, Greg KH wrote:
> > On Tue, Jan 17, 2017 at 10:19:11AM -0500, J. Bruce Fields wrote:
> > > 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@linuxfoundation.org>
> > > > > > 
> > > > > > 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.
> > 
> > Kind of, nice catch, I'll change the call to usermodehelper to use
> > sbin_pnpbios here, as that's the right thing to do.
> 
> Oops, no, the patch was doing the right thing here, you missed the next
> chunk of the patch:

Oh, got it, thanks.

--b.

> 
> 
> @@ -139,7 +140,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
>                            info->location_id, info->serial, info->capabilities);
>         envp[i] = NULL;
> 
> -       value = call_usermodehelper(argv [0], argv, envp, UMH_WAIT_EXEC);
> +       value = call_usermodehelper(sbin_pnpbios, argv, envp, UMH_WAIT_EXEC);
>         kfree(buf);
>         kfree(envp);
>         return 0;
> 
> 
> So it's ok.
> 
> thanks,
> 
> greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web