make call_usermodehelper a bit more "safe"

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.

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/macintosh/windfarm_core.c          | 2 +-
 drivers/net/hamradio/baycom_epp.c          | 2 +-
 drivers/pnp/pnpbios/core.c                 | 5 +++--
 drivers/staging/greybus/svc_watchdog.c     | 4 ++--
 drivers/staging/rtl8192e/rtl8192e/rtl_dm.c | 6 +++---
 fs/nfsd/nfs4layouts.c                      | 6 ++++--
 security/keys/request_key.c                | 7 ++++---
 7 files changed, 18 insertions(+), 14 deletions(-)

drivers/macintosh/windfarm_core.c +0 -0

index 465d770ab0bb..1b317cbb73cf 100644

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.

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/macintosh/windfarm_core.c          | 2 +-
 drivers/net/hamradio/baycom_epp.c          | 2 +-
 drivers/pnp/pnpbios/core.c                 | 5 +++--
 drivers/staging/greybus/svc_watchdog.c     | 4 ++--
 drivers/staging/rtl8192e/rtl8192e/rtl_dm.c | 6 +++---
 fs/nfsd/nfs4layouts.c                      | 6 ++++--
 security/keys/request_key.c                | 7 ++++---
 7 files changed, 18 insertions(+), 14 deletions(-)

drivers/macintosh/windfarm_core.c +0 -0

index 465d770ab0bb..1b317cbb73cf 100644

Quoted 4 lines · Greg KH

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.

You're not preventing change of where they point to, but rather preventing modification of the pointed-to data through these pointers...

Quoted 17 lines

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/macintosh/windfarm_core.c          | 2 +-
 drivers/net/hamradio/baycom_epp.c          | 2 +-
 drivers/pnp/pnpbios/core.c                 | 5 +++--
 drivers/staging/greybus/svc_watchdog.c     | 4 ++--
 drivers/staging/rtl8192e/rtl8192e/rtl_dm.c | 6 +++---
 fs/nfsd/nfs4layouts.c                      | 6 ++++--
 security/keys/request_key.c                | 7 ++++---
 7 files changed, 18 insertions(+), 14 deletions(-)

drivers/macintosh/windfarm_core.c +0 -0

index 465d770ab0bb..1b317cbb73cf 100644
@@ -74,7 +74,7 @@ static inline void wf_notify(int event, void *param)
 

drivers/macintosh/windfarm_core.c static inline void wf_notify(int event, void *param) · line 77

Quoted 4 lines

drivers/macintosh/windfarm_core.c +1 -1

 static int wf_critical_overtemp(void)
 {
-	static char * critical_overtemp_path = "/sbin/critical_overtemp";
+	static const char * critical_overtemp_path = "/sbin/critical_overtemp";

Should be static char *const critical_overtemp_path, or if you prefer static const char *const critical_overtemp_path (since the pointed-to data is not modifiable either). Likewise elsewhere.

Quoted 27 lines · Rich Felker

Quoted 4 lines · Greg KH

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.

You're not preventing change of where they point to, but rather preventing modification of the pointed-to data through these pointers...

Quoted 17 lines

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/macintosh/windfarm_core.c          | 2 +-
 drivers/net/hamradio/baycom_epp.c          | 2 +-
 drivers/pnp/pnpbios/core.c                 | 5 +++--
 drivers/staging/greybus/svc_watchdog.c     | 4 ++--
 drivers/staging/rtl8192e/rtl8192e/rtl_dm.c | 6 +++---
 fs/nfsd/nfs4layouts.c                      | 6 ++++--
 security/keys/request_key.c                | 7 ++++---
 7 files changed, 18 insertions(+), 14 deletions(-)

drivers/macintosh/windfarm_core.c +0 -0

index 465d770ab0bb..1b317cbb73cf 100644
@@ -74,7 +74,7 @@ static inline void wf_notify(int event, void *param)
 

Quoted 8 lines

drivers/macintosh/windfarm_core.c static inline void wf_notify(int event, void *param) · line 77

Quoted 4 lines

drivers/macintosh/windfarm_core.c +1 -1

 static int wf_critical_overtemp(void)
 {
-	static char * critical_overtemp_path = "/sbin/critical_overtemp";
+	static const char * critical_overtemp_path = "/sbin/critical_overtemp";

Should be static char *const critical_overtemp_path, or if you prefer static const char *const critical_overtemp_path (since the pointed-to data is not modifiable either). Likewise elsewhere.

argh, ok, I failed here, thanks for that, that's what I get for writing code on an airplane...

let me rework this, I also want to make argv and env static too, just for good measure.

To follow up on this, and after staring at too many outputs of the compiler, I think what this really should be is:

	static char const critical_overtemp_path[] =

"/sbin/critical_overtemp";
right?

That way both the variable, and the data, end up in read-only memory from what I can tell.

But, if I do:

	static char const char critical_overtemp_path[] =

"/sbin/critical_overtemp";
then sparse complains to me about:
warning: duplicate const

Is that just sparse being dense, or is the latter one really better here?  It seems that both of the above put the data and variable into the same segment (.rodata).

thanks,

greg k-h

Quoted 15 lines

To follow up on this, and after staring at too many outputs of the compiler, I think what this really should be is:

	static char const critical_overtemp_path[] =

"/sbin/critical_overtemp";
right?

That way both the variable, and the data, end up in read-only memory from what I can tell.

But, if I do:

	static char const char critical_overtemp_path[] =

"/sbin/critical_overtemp";
then sparse complains to me about:
warning: duplicate const

Quoted 7 lines

Is that just sparse being dense, or is the latter one really better here?  It seems that both of the above put the data and variable into the same segment (.rodata).

thanks,

greg k-h

Either 'char *const foo = "bar"' or 'const char *const foo = "bar" will also be a string constant in rodata with a pointer in rodata referring to them. Duplicate string constants get merged without any analysis as there's no guarantee of a unique address for the data itself since it's not a variable. 'const char foo[] = "bar"' goes into rodata too, but is the toolchain can't assume it can't safely merge strings + sizeof works but gcc/clang know how to optimize constant strlen anyway.

The 'const' qualifier for pointers doesn't really do anything, it's when it's used on the variable (after the pointer) that it can do more than acting as a programming guide.

Thanks for the explanation.  I don't think we need to worry about merging these strings, but I'll keep it in mind.

However, the "folklore" of the kernel was to never do:

	char *foo = "bar";
but instead do:
	char foo[] = "bar";

to save on the extra variable that the former creates.  Is that no longer the case and we really should be using '*' to allow gcc to be smarter about optimizations?

Quoted 5 lines

The 'const' qualifier for pointers doesn't really do anything, it's when
it's used on the variable (after the pointer) that it can do more than
acting as a programming guide.

Many thanks for the explanations,

greg k-h

So for statics, I think static const char * wins due to allowing merging (although it doesn't matter here). For non-statics, you end up with extra pointer constants. Those could get removed, but Linux doesn't
have -fvisibility=hidden and I'm not sure how clever linkers are. Maybe
setting up -fvisibility=hidden to work with monolithic non-module- enabled builds could actually be realistic. Expect it'd remove a fair bit of bloat but not sure how much would need to be marked as non- hidden
other than the userspace ABI.

If you can write to kernel memory, an "easy" way to get the kernel to run any application is to change the pointer of one of the usermode helper program names. To try to mitigate this, create a new config option, CONFIG_READONLY_USERMODEHELPER.

This option only allows "predefined" binaries to be called. A number of drivers and subsystems allow for the name of the binary to be changed, and this config option disables that capability, so be aware of that.

Note: Still a proof-of-concept at this point in time, doesn't cover all of the call_usermodehelper() calls just yet, including the "fun" of coredumps, it's still a work in progress.

Not-Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c | 12 ++++++++----
 drivers/block/drbd/drbd_int.h    |  6 +++++-
 drivers/block/drbd/drbd_main.c   |  5 +++++
 drivers/video/fbdev/uvesafb.c    | 19 ++++++++++++++-----
 fs/nfs/cache_lib.c               | 12 ++++++++++--
 include/linux/reboot.h           |  2 ++
 kernel/ksysfs.c                  |  6 +++++-
 kernel/reboot.c                  |  3 +++
 kernel/sysctl.c                  |  4 ++++
 lib/kobject_uevent.c             |  3 +++
 security/Kconfig                 | 17 +++++++++++++++++
 11 files changed, 76 insertions(+), 13 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +0 -0

index 00ef43233e03..92a2ef8ffe3e 100644

arch/x86/kernel/cpu/mcheck/mce.c +7 -3

-static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
-                               const char *buf, size_t siz)
+#ifndef CONFIG_READONLY_USERMODEHELPER
+static ssize_t trigger_store(struct device *s, struct device_attribute *attr,
+                            const char *buf, size_t siz)
 {
        char *p;
 

@@ -2358,6 +2359,10 @@ static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
 
        return strlen(mce_helper) + !!p;
 }
+static DEVICE_ATTR_RW(trigger);
+#else
+static DEVICE_ATTR_RO(trigger);
+#endif
 
 static ssize_t set_ignore_ce(struct device *s,
                             struct device_attribute *attr,

@@ -2415,7 +2420,6 @@ static ssize_t store_int_with_restart(struct device *s,
        return ret;
 }
 
-static DEVICE_ATTR(trigger, 0644, show_trigger, set_trigger);
 static DEVICE_INT_ATTR(tolerant, 0644, mca_cfg.tolerant);
 static DEVICE_INT_ATTR(monarch_timeout, 0644, mca_cfg.monarch_timeout);
 static DEVICE_BOOL_ATTR(dont_log_ce, 0644, mca_cfg.dont_log_ce);

drivers/block/drbd/drbd_int.h +0 -0

index a139a34f1f1e..e21ab2bcc482 100644

Quoted 35 lines · Greg KH

If you can write to kernel memory, an "easy" way to get the kernel to run any application is to change the pointer of one of the usermode helper program names. To try to mitigate this, create a new config option, CONFIG_READONLY_USERMODEHELPER.

This option only allows "predefined" binaries to be called. A number of drivers and subsystems allow for the name of the binary to be changed, and this config option disables that capability, so be aware of that.

Note: Still a proof-of-concept at this point in time, doesn't cover all of the call_usermodehelper() calls just yet, including the "fun" of coredumps, it's still a work in progress.

Not-Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c | 12 ++++++++----
 drivers/block/drbd/drbd_int.h    |  6 +++++-
 drivers/block/drbd/drbd_main.c   |  5 +++++
 drivers/video/fbdev/uvesafb.c    | 19 ++++++++++++++-----
 fs/nfs/cache_lib.c               | 12 ++++++++++--
 include/linux/reboot.h           |  2 ++
 kernel/ksysfs.c                  |  6 +++++-
 kernel/reboot.c                  |  3 +++
 kernel/sysctl.c                  |  4 ++++
 lib/kobject_uevent.c             |  3 +++
 security/Kconfig                 | 17 +++++++++++++++++
 11 files changed, 76 insertions(+), 13 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +0 -0

index 00ef43233e03..92a2ef8ffe3e 100644
@@ -2337,15 +2337,16 @@ static ssize_t set_bank(struct device *s, struct device_attribute *attr,
 }
 

arch/x86/kernel/cpu/mcheck/mce.c static ssize_t set_bank(struct device *s, struct device_attribute *attr, · line 2344

Quoted 7 lines

arch/x86/kernel/cpu/mcheck/mce.c +1 -1

 static ssize_t
-show_trigger(struct device *s, struct device_attribute *attr, char *buf)
+trigger_show(struct device *s, struct device_attribute *attr, char *buf)
 {
        strcpy(buf, mce_helper);
        strcat(buf, "\n");
        return strlen(mce_helper) + 1;

The +1 is wrong, AFAICT. Also, is speed really needed here?

    return scnprintf(buf, PAGE_SIZE, "%s\n", mce_helper);

is more readable...

Quoted 32 lines

arch/x86/kernel/cpu/mcheck/mce.c +7 -3

-static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
-                               const char *buf, size_t siz)
+#ifndef CONFIG_READONLY_USERMODEHELPER
+static ssize_t trigger_store(struct device *s, struct device_attribute *attr,
+                            const char *buf, size_t siz)
 {
        char *p;
 

@@ -2358,6 +2359,10 @@ static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
 
        return strlen(mce_helper) + !!p;
 }
+static DEVICE_ATTR_RW(trigger);
+#else
+static DEVICE_ATTR_RO(trigger);
+#endif
 
 static ssize_t set_ignore_ce(struct device *s,
                             struct device_attribute *attr,

@@ -2415,7 +2420,6 @@ static ssize_t store_int_with_restart(struct device *s,
        return ret;
 }
 
-static DEVICE_ATTR(trigger, 0644, show_trigger, set_trigger);
 static DEVICE_INT_ATTR(tolerant, 0644, mca_cfg.tolerant);
 static DEVICE_INT_ATTR(monarch_timeout, 0644, mca_cfg.monarch_timeout);
 static DEVICE_BOOL_ATTR(dont_log_ce, 0644, mca_cfg.dont_log_ce);

drivers/block/drbd/drbd_int.h +0 -0

index a139a34f1f1e..e21ab2bcc482 100644

drivers/block/drbd/drbd_int.h extern int fault_rate; · line 82

Quoted 9 lines

drivers/block/drbd/drbd_int.h +5 -1

@@ -75,7 +75,11 @@ extern int fault_rate;
 extern int fault_devs;
 #endif
 
-extern char drbd_usermode_helper[];
+extern
+#ifdef CONFIG_READONLY_USERMODEHELPER
+       const
+#endif
+             char drbd_usermode_helper[];

This #ifdef; const; #endif is repeated a few times. Perhaps better to create a separate macro:

#ifdef CONFIG_READONLY_USERMODEHELPER

# define __ro_umh const
#else
# define __ro_umh /**/
#endif

...

extern __ro_umh char drbd_usermode_helper[];

The issue is that if you end up getting write access to kernel memory, if you change the string '/sbin/hotplug' to point to '/home/hacked/my_binary', then the next uevent that the system makes will call this binary instead of the "trusted" one.

It does this by moving the location of the binary to be in read-only memory. This works for a number of call_usermodehelper strings, as they are specified at build or configuration time. But, some subsystems have the option to let userspace change the value at runtime, so those values can't live in read-only memory.

So, anyone have any better ideas? Is this approach worth it? Or should we just go down the "whitelist" path?

Hi,

Quoted 10 lines · Greg KH

The issue is that if you end up getting write access to kernel memory, if you change the string '/sbin/hotplug' to point to '/home/hacked/my_binary', then the next uevent that the system makes will call this binary instead of the "trusted" one.

It does this by moving the location of the binary to be in read-only memory. This works for a number of call_usermodehelper strings, as they are specified at build or configuration time. But, some subsystems have the option to let userspace change the value at runtime, so those values can't live in read-only memory.

Quoted 2 lines

So, anyone have any better ideas? Is this approach worth it? Or should we just go down the "whitelist" path?

As a general note, I believe the write-rarely / mostly-ro [1] stuff is meant to cater for this case, but I haven't heard anything on that front recently (and there doesn't appear to be anything on the KSPP TODO page).

If that does cater for this case, and if we're able to implement that generically, that might be nicer than locking down the set of binaries at build time.

Chen, are you still looking at implementing write-rarely support?

Thanks,
Mark.

[1] http://www.openwall.com/lists/kernel-hardening/2016/11/16/3

So, anyone have any better ideas? Is this approach worth it? Or should we just go down the "whitelist" path?

Hi Greg,

Quoted 2 lines · Greg KH

So, anyone have any better ideas? Is this approach worth it? Or should we just go down the "whitelist" path?

I think your approach is generally better than the whitelist path. But maybe there's yet a third approach that involves futzing with page permissions at runtime. I think grsec does something similar with read_mostly function pointer structs. Namely, they make them read-only const, and then temporarily twiddle the page permissions if it needs to be changed while disabling preemption. There could be a particular class of data that needs to be "opened" and "closed" in order to modify. Seems like these strings would be a good use of that.

Hi all,

Here's a proof-of-concept patch series that tries to work to address the issue of call_usermodehelper being abused to have the kernel call any userspace binary with full root permissions.

The issue is that if you end up getting write access to kernel memory, if you change the string '/sbin/hotplug' to point to '/home/hacked/my_binary', then the next uevent that the system makes will call this binary instead of the "trusted" one.

Quoted 6 lines · Greg KH

Hi all,

Here's a proof-of-concept patch series that tries to work to address the issue of call_usermodehelper being abused to have the kernel call any userspace binary with full root permissions.

Quoted 4 lines

The issue is that if you end up getting write access to kernel memory, if you change the string '/sbin/hotplug' to point to '/home/hacked/my_binary', then the next uevent that the system makes will call this binary instead of the "trusted" one.

You seem to be targeting a situation where the kernel memory can be easily changed, but filesystem content cannot (if it could - the attacker would simply replace /sbin/hotplug).

If that is a credible threat scenario, it seems to me that the simplest mitigation is to have call_usermodehelper always call a single compiled-in path - e.g. /sbin/usermode-helper - and rely on that program to validate argv[0] and call it if it is deemed safe.

i.e. get the policy out of the kernel.

Quoted 3 lines

You seem to be targeting a situation where the kernel memory can be easily changed, but filesystem content cannot (if it could - the attacker would simply replace /sbin/hotplug).

Correct, like an embedded system with a read-only system partition, or for when some kernel bug allows for random memory writes, yet privilege escalation is hard to achieve for your process.

Quoted 4 lines · Greg KH

Quoted 3 lines

You seem to be targeting a situation where the kernel memory can be easily changed, but filesystem content cannot (if it could - the attacker would simply replace /sbin/hotplug).

Quoted 3 lines

Correct, like an embedded system with a read-only system partition, or for when some kernel bug allows for random memory writes, yet privilege escalation is hard to achieve for your process.

Sorry, I really don't get this.

If kernel memory can be easily changed (which is assumed here), why bother with all this? I'll just set current->uid to 0 and be done.

Quoted 4 lines

Sorry, I really don't get this.

If kernel memory can be easily changed (which is assumed here), why bother with all this? I'll just set current->uid to 0 and be done.

Because you don't want your current process to uid 0, you want some other program to run as root. It's quite common for exploits to work this way, take a look at how the p0wn-to-own "contests" usually break out of sandboxed systems like browsers.

I stay totally unconvinced that such kind of countermeasure brings any value whatsoever. Could you please bring up a particular usecase, where you have complete control over kernel memory, and still the only possible exploit factor is redirecting usermodhelper? It feels like rather random shot into darkness.

Quoted 5 lines · Jiri Kosina

I stay totally unconvinced that such kind of countermeasure brings any value whatsoever. Could you please bring up a particular usecase, where you have complete control over kernel memory, and still the only possible exploit factor is redirecting usermodhelper? It feels like rather random shot into darkness.

If we want to make usermod helper really secure, perhaps the best way to go would be to completely nuke it and handle everyhting in udev; that'd be quite some work though, especially so that we don't break all the corner cases of module autoloading (request_module() and such).