devtmpfs: mount with noexec and nosuid
PATCH v2 1/1 devtmpfs: mount with noexec and nosuid
Since devtmpfs is writable, make the default noexec,nosuid as well. This protects from the case of a privileged process having an arbitrary file write flaw and an argumentless arbitrary execution (i.e. it would lack the ability to run "mount -o remount,exec,suid /dev").
---
v2:
- use CONFIG_DEVTMPFS_SAFE to wrap the logic.
--- drivers/base/Kconfig | 12 ++++++++++++ drivers/base/devtmpfs.c | 12 ++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-)
drivers/base/Kconfig +12 -0
@@ -56,6 +56,18 @@ config DEVTMPFS_MOUNT rescue mode with init=/bin/sh, even when the /dev directory on the rootfs is completely empty. +config DEVTMPFS_SAFE + bool "Use nosuid,noexec mount options on devtmpfs" + depends on DEVTMPFS + help + This instructs the kernel to include the MS_NOEXEC and + MS_NOSUID mount flags when mounting devtmpfs. This prevents + certain kinds of code-execution attacks on embedded platforms. + + Notice: If enabled, things like /dev/mem cannot be mmapped + with the PROT_EXEC flag. This can break, for example, non-KMS + video drivers. + config STANDALONE bool "Select only drivers that don't need compile-time external firmware" if EXPERIMENTAL default y
drivers/base/devtmpfs.c +10 -2
@@ -340,6 +340,10 @@ static int handle_remove(const char *nodename, struct device *dev)int devtmpfs_mount (const char *mntdir) {int err; +int mflags =MS_SILENT ; +#ifdef CONFIG_DEVTMPFS_SAFE + mflags |=MS_NOEXEC |MS_NOSUID ; +#endif if (!mount_dev)return 0 ;
@@ -347,7 +351,7 @@ int devtmpfs_mount(const char *mntdir)if (!thread)return 0 ; - err =sys_mount ("devtmpfs" , (char *)mntdir,"devtmpfs" ,MS_SILENT ,NULL ); + err =sys_mount ("devtmpfs" , (char *)mntdir,"devtmpfs" , mflags,NULL );if (err)printk (KERN_INFO "devtmpfs: error mounting %i\n" , err);else
@@ -369,10 +373,14 @@ static int devtmpfsd(void *p) {char options[] ="mode=0755" ;int *err = p; +int mflags =MS_SILENT ; +#ifdef CONFIG_DEVTMPFS_SAFE + mflags |=MS_NOEXEC |MS_NOSUID ; +#endif *err =sys_unshare (CLONE_NEWNS );if (*err) goto out; - *err =sys_mount ("devtmpfs" ,"/" ,"devtmpfs" ,MS_SILENT , options); + *err =sys_mount ("devtmpfs" ,"/" ,"devtmpfs" , mflags, options);if (*err) goto out;sys_chdir ("/.." );/* will traverse into overmounted root */
Signature · 6 lines
1.7.9.5 -- Kees Cook Chrome OS Security
Greg Kroah-Hartman
maintainerQuoted 5 lines · Kees Cook
Since devtmpfs is writable, make the default noexec,nosuid as well. This protects from the case of a privileged process having an arbitrary file write flaw and an argumentless arbitrary execution (i.e. it would lack the ability to run "mount -o remount,exec,suid /dev").
Quoted 8 lines
Cc Kay Sievers <[email protected]>Cc Roland Eggner <[email protected]>Signed-off-by Kees Cook <[email protected]>---
v2:
- useCONFIG_DEVTMPFS_SAFEto wrap the logic.
That's better, thanks.
One tiny bikeshead request though:
drivers/base/devtmpfs.c static int handle_remove(const char *nodename, struct device *dev) · line 346
Quoted 10 lines
drivers/base/devtmpfs.c +4
-0@@ -340,6 +340,10 @@ static int handle_remove(const char *nodename, struct device *dev)int devtmpfs_mount (const char *mntdir) {int err; +int mflags =MS_SILENT ; +#ifdef CONFIG_DEVTMPFS_SAFE + mflags |=MS_NOEXEC |MS_NOSUID ; +#endif
You duplicate this in two places, which makes the c code harder to read. How about, at the top of the file, you do:
#ifdef CONFIG_DEVTMPFS_SAFE
#define DEVTMPFS_MFLAGS MS_SILENT | MS_NOEXEC | MS_NOSUID
#else
#define DEVTMPFS_MFLAGS MS_SILENT
#endif
And then just use DEVTMPFS_MFLAGS in both places?
That should make the patch smaller, which is always nice :)
thanks,
greg k-h
Quoted 39 lines · Greg Kroah-Hartman
Quoted 5 lines · Kees Cook
Since devtmpfs is writable, make the default noexec,nosuid as well. This protects from the case of a privileged process having an arbitrary file write flaw and an argumentless arbitrary execution (i.e. it would lack the ability to run "mount -o remount,exec,suid /dev").
Quoted 8 lines
Cc Kay Sievers <[email protected]>Cc Roland Eggner <[email protected]>Signed-off-by Kees Cook <[email protected]>---
v2:
- useCONFIG_DEVTMPFS_SAFEto wrap the logic.That's better, thanks.
One tiny bikeshead request though:
drivers/base/devtmpfs.c static int handle_remove(const char *nodename, struct device *dev) · line 346
Quoted 10 lines
drivers/base/devtmpfs.c +4
-0@@ -340,6 +340,10 @@ static int handle_remove(const char *nodename, struct device *dev)int devtmpfs_mount (const char *mntdir) {int err; +int mflags =MS_SILENT ; +#ifdef CONFIG_DEVTMPFS_SAFE + mflags |=MS_NOEXEC |MS_NOSUID ; +#endif You duplicate this in two places, which makes the c code harder to read. How about, at the top of the file, you do:
#ifdef
CONFIG_DEVTMPFS_SAFE
#defineDEVTMPFS_MFLAGSMS_SILENT|MS_NOEXEC|MS_NOSUID#else
#defineDEVTMPFS_MFLAGSMS_SILENT
#endif
Quoted 3 lines
And then just use
DEVTMPFS_MFLAGSin both places?That should make the patch smaller, which is always nice :)
Excellent idea.
-Kees
Signature · 2 lines
Kees Cook Chrome OS Security
Quoted 4 lines · On Tue, 20 Nov 2012 12:42:38 -0800 Kees Cook <[email protected]> wrote:
Since devtmpfs is writable, make the default noexec,nosuid as well. This protects from the case of a privileged process having an arbitrary file write flaw and an argumentless arbitrary execution (i.e. it would lack the ability to run "mount -o remount,exec,suid /dev").
Ok this looks crap on two levels.
1. Why not just have your userspace mount -o remount the file system this way already in early boot. (and if you trojanned boot that early then any supposed security gain is already lost)
2. If you want to do this right then you need to work out what you are trying to prevent. Your devtmpfs can force file permissions on the underlying device nodes by having its own operation handling for chmod.
At that point you can force permissions on anything that you want to avoid floating around that filesystem with other rights, while not touching it on device or directory nodes where the meaning is different.
In its current form however it appears to be a kernel implementation of "mount is too hard".
Alan
Quoted 8 lines · Alan Cox
Quoted 4 lines · On Tue, 20 Nov 2012 12:42:38 -0800 Kees Cook <[email protected]> wrote:
Since devtmpfs is writable, make the default noexec,nosuid as well. This protects from the case of a privileged process having an arbitrary file write flaw and an argumentless arbitrary execution (i.e. it would lack the ability to run "mount -o remount,exec,suid /dev").
Quoted 5 lines
Ok this looks crap on two levels.
1. Why not just have your userspace mount -o remount the file system this way already in early boot. (and if you trojanned boot that early then any supposed security gain is already lost)
That's certainly possible, but I am hoping to avoid adding any extra boot time. The kernel is responsible for this mount, so its flags should be configurable, resulting in no time penalty anywhere.
Quoted 10 lines
2. If you want to do this right then you need to work out what you are trying to prevent. Your devtmpfs can force file permissions on the underlying device nodes by having its own operation handling for chmod.
At that point you can force permissions on anything that you want to avoid floating around that filesystem with other rights, while not touching it on device or directory nodes where the meaning is different.
In its current form however it appears to be a kernel implementation of "mount is too hard".
This change also stops mmap() with PROT_EXEC which a chmod handler wouldn't be able to do. The noexec and nosuid mount options were designed for this sort of thing, so I think that's where it should be handled.
-Kees
Signature · 2 lines
Kees Cook Chrome OS Security
Quoted 7 lines
Quoted 3 lines
1. Why not just have your userspace mount -o remount the file system this way already in early boot. (and if you trojanned boot that early then any supposed security gain is already lost)
That's certainly possible, but I am hoping to avoid adding any extra boot time. The kernel is responsible for this mount, so its flags should be configurable, resulting in no time penalty anywhere.
You just broke my bullshitometer
It's a single syscall from your init binary, its microseconds.
Quoted 8 lines
Quoted 8 lines
2. If you want to do this right then you need to work out what you are trying to prevent. Your devtmpfs can force file permissions on the underlying device nodes by having its own operation handling for chmod.
At that point you can force permissions on anything that you want to avoid floating around that filesystem with other rights, while not touching it on device or directory nodes where the meaning is different.
Quoted 5 lines
Quoted 2 lines
In its current form however it appears to be a kernel implementation of "mount is too hard".
This change also stops
mmap()withPROT_EXECwhich a chmod handler wouldn't be able to do.
You don't want to stop mmap with PROT_EXEC on /dev/mem as that breaks a load of stuff, you want to stop people adding stuff to that file system and executing it.
Think about it more carefully - if I've got access to /dev/mem you already lost.
The patch is as is nonsense. It's doable in all kernels (including existing legacy ones) as a single syscall in your init code. That's portable back compatible and works all over the place. Your patch introduces a pointless config option to make things less compatible than before. It's bogus.
Alan
Quoted 11 lines · Alan Cox
Quoted 7 lines
Quoted 3 lines
1. Why not just have your userspace mount -o remount the file system this way already in early boot. (and if you trojanned boot that early then any supposed security gain is already lost)
That's certainly possible, but I am hoping to avoid adding any extra boot time. The kernel is responsible for this mount, so its flags should be configurable, resulting in no time penalty anywhere.
You just broke my bullshitometer
It's a single syscall from your init binary, its microseconds.
Whatever, I still see it as a needless inefficiency.
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
2. If you want to do this right then you need to work out what you are trying to prevent. Your devtmpfs can force file permissions on the underlying device nodes by having its own operation handling for chmod.
At that point you can force permissions on anything that you want to avoid floating around that filesystem with other rights, while not touching it on device or directory nodes where the meaning is different.
In its current form however it appears to be a kernel implementation of "mount is too hard".
Quoted 6 lines
Quoted 2 lines
This change also stops
mmap()withPROT_EXECwhich a chmod handler wouldn't be able to do.You don't want to stop mmap with
PROT_EXECon/dev/memas that breaks a load of stuff, you want to stop people adding stuff to that file system and executing it.
Well, initially the latter, yes. But as it turns out, setting noexec also stops PROT_EXEC on /dev/mem. Since the systems I'm building for all use KMS, there's no need to execute regions of /dev/mem (e.g. VESA BIOS init, etc).
-Kees
Signature · 2 lines
Kees Cook Chrome OS Security
Alan Cox
NAKQuoted 5 lines
Quoted 3 lines
You just broke my bullshitometer
It's a single syscall from your init binary, its microseconds.
Whatever, I still see it as a needless inefficiency.
As opposed to adding permanent kernel code and having to change the setting by recompilation, and not being able to deploy this with older kernels.
Right now you can add one call to your user space and it just works, with old kernels, with new kernels, with Android etc. You think its *more efficient* to add permanently loaded kernel hacks.
Wrong. Big time wrong. A page is 4096 bytes, your change is probably about 8. So 1 in 512 builds will see an extra page of kernel space used assuming an even distribution. For each of those users the moment they've had a few page faults your solution is *less* efficient, and the moment they've paged one page because of having less memory your solution has become vastly less efficient.
So can we put the crap about efficiency away please. This basically reads like
"Mummy, the mount syscall looks complicated let me hack everyones kernel
with a crass extra Kconfig option because I'm crap"
Quoted 8 lines
Quoted 3 lines
You don't want to stop mmap with
PROT_EXECon/dev/memas that breaks a load of stuff, you want to stop people adding stuff to that file system and executing it.Well, initially the latter, yes. But as it turns out, setting noexec also stops
PROT_EXECon/dev/mem. Since the systems I'm building for all use KMS, there's no need to execute regions of/dev/mem(e.g. VESA BIOS init, etc).
I have news for you: this is the upstream kernel, you specific personal needs should not define what is good design - this isn't GNOME.
If you block exec on anything but devices then all is happy. You could even block it on anything but specific devices. At that point you could get rid of the config option and make it more useful.
We don't need extra confusion Kconfig options, we don't need extra kernel code combinations to fail to maintain. Your patch offers *NO* feature advantages over the existing kernel.
NAK
Alan
Quoted 9 lines · Alan Cox
Quoted 5 lines
Quoted 3 lines
You just broke my bullshitometer
It's a single syscall from your init binary, its microseconds.
Whatever, I still see it as a needless inefficiency.
As opposed to adding permanent kernel code and having to change the setting by recompilation, and not being able to deploy this with older kernels.
Some organizations only support their single "current" kernel, and always move forward.
Quoted 3 lines
Right now you can add one call to your user space and it just works, with old kernels, with new kernels, with Android etc. You think its *more efficient* to add permanently loaded kernel hacks.
I'm not interested in those things. That said, if it genuinely agreed that this clear lack of security best-practice (W^X) in the kernel isn't the place to fix it, I'll move on. There are still legacy video drivers and things that can't operate with this configuration, but the kernel mounting devtmpfs shouldn't _require_ userspace to clean up after a buggy kernel.
Quoted 6 lines
Wrong. Big time wrong. A page is 4096 bytes, your change is probably about 8. So 1 in 512 builds will see an extra page of kernel space used assuming an even distribution. For each of those users the moment they've had a few page faults your solution is *less* efficient, and the moment they've paged one page because of having less memory your solution has become vastly less efficient.
My patch doesn't change the code size there after the most recent change to use a constant value again, so the only change I can see would be if someone built with /proc/config.gz which would show the new config.
Quoted 5 lines
So can we put the crap about efficiency away please. This basically reads like
"Mummy, the mount syscall looks complicated let me hack everyones kernel
with a crass extraKconfigoption because I'm crap"
I think you just switched from calling my code crap to calling me crap.
Quoted 11 lines
Quoted 8 lines
Quoted 3 lines
You don't want to stop mmap with
PROT_EXECon/dev/memas that breaks a load of stuff, you want to stop people adding stuff to that file system and executing it.Well, initially the latter, yes. But as it turns out, setting noexec also stops
PROT_EXECon/dev/mem. Since the systems I'm building for all use KMS, there's no need to execute regions of/dev/mem(e.g. VESA BIOS init, etc).I have news for you: this is the upstream kernel, you specific personal needs should not define what is good design - this isn't GNOME.
Heh, I support your dig at GNOME, but refute this being a "specific personal need". Best-practices are clear here. The only reason the kernel hasn't been doing it is fear of breaking legacy video drivers.
Quoted 3 lines
If you block exec on anything but devices then all is happy. You could even block it on anything but specific devices. At that point you could get rid of the config option and make it more useful.
Security is layers, and is never perfect. We already do a fair bit of process confinement, but this change seemed to be clear low-hanging fruit.
Quoted 3 lines
We don't need extra confusion
Kconfigoptions, we don't need extra kernel code combinations to fail to maintain. Your patch offers *NO* feature advantages over the existing kernel.
I disagree. It allows a system builder to opt-in to correcting a kernel flaw introduced by the need to support legacy devices in the case where an initramfs isn't used.
Quoted 1 line
NAK
I don't understand why you're so violently opposed to letting an arguably flawed constant get fixed. I'm not even asking for this to be on by default.
-Kees
Signature · 2 lines
Kees Cook Chrome OS Security
Quoted 5 lines · On Tue, 20 Nov 2012 16:41:27 -0800 Kees Cook <[email protected]> wrote:
Quoted 4 lines · Alan Cox
Quoted 4 lines
Quoted 3 lines
You just broke my bullshitometer
It's a single syscall from your init binary, its microseconds.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Whatever, I still see it as a needless inefficiency.
As opposed to adding permanent kernel code and having to change the setting by recompilation, and not being able to deploy this with older kernels.
Some organizations only support their single "current" kernel, and always move forward.
This is not GNOME. Your specific organisational choices are not the sole decider.
Quoted 1 line
I'm not interested in those things.
This is not GNOME.
Quoted 5 lines
that this clear lack of security best-practice (W^X) in the kernel isn't the place to fix it, I'll move on. There are still legacy video drivers and things that can't operate with this configuration, but the kernel mounting devtmpfs shouldn't _require_ userspace to clean up after a buggy kernel.
The kernel is not "buggy". It merely has defaults you don't personally like. They are configurable via userspace. Policy belongs in userspace.
Quoted 11 lines
Quoted 6 lines
Wrong. Big time wrong. A page is 4096 bytes, your change is probably about 8. So 1 in 512 builds will see an extra page of kernel space used assuming an even distribution. For each of those users the moment they've had a few page faults your solution is *less* efficient, and the moment they've paged one page because of having less memory your solution has become vastly less efficient.
My patch doesn't change the code size there after the most recent change to use a constant value again, so the only change I can see would be if someone built with
/proc/config.gzwhich would show the new config.
So in actual fact going by distro choices its worse than I thought 8)
Quoted 3 lines
Heh, I support your dig at GNOME, but refute this being a "specific personal need". Best-practices are clear here. The only reason the kernel hasn't been doing it is fear of breaking legacy video drivers.
Thats not true. Nor is it a case of "Not doing". This feature has been supported for over ten years. We also support more flexible alternatives like using SELinux, Smack or AppArmor for it. Right now I can do no exec for /dev except for specific files entirely by security rules.
Quoted 3 lines
Security is layers, and is never perfect. We already do a fair bit of process confinement, but this change seemed to be clear low-hanging fruit.
It's not a bad idea - but its a single userspace syscall in your init code. Has been for over a decade.
Quoted 7 lines
Quoted 3 lines
We don't need extra confusion
Kconfigoptions, we don't need extra kernel code combinations to fail to maintain. Your patch offers *NO* feature advantages over the existing kernel.I disagree. It allows a system builder to opt-in to correcting a kernel flaw introduced by the need to support legacy devices in the case where an initramfs isn't used.
Thats a fiction you invented. It's completely irrelevant to most systems and its already long supported. I think doing that mount in Chrome is sensible. I think doing it in userspace as we've supported for over a decade is rather smarter than kernel hacks. You don't have to pee in other people's ponds that way or achieve consensus you can just go do what you need.
Quoted 3 lines
I don't understand why you're so violently opposed to letting an arguably flawed constant get fixed. I'm not even asking for this to be on by default.
Because its a pointless option. It adds nothing but an extra Kconfig option for everyone. It's a complete waste. Policy belongs in user space.
Fix your userspace. It can't be *that* hard to add a mount call to a chromium init. It's hardly rocket science. That gives you what you want, avoids more configuration options, and doesn't mess up compatibility or add confusing interactions for those users already using more flexible security options than your mount.
Alan
Quoted 4 lines · Kees Cook
Since devtmpfs is writable, make the default noexec,nosuid as well. This protects from the case of a privileged process having an arbitrary file write flaw and an argumentless arbitrary execution (i.e. it would lack the ability to run "mount -o remount,exec,suid /dev").
This only really applies to systems without an initramfs when the kernel mounts /dev over the rootfs it has mounted; with an initramfs, /dev is always mounted by user code.
Just checking, that is the use case you are doing that for?
Kay
Quoted 11 lines · Kay Sievers
Quoted 4 lines · Kees Cook
Since devtmpfs is writable, make the default noexec,nosuid as well. This protects from the case of a privileged process having an arbitrary file write flaw and an argumentless arbitrary execution (i.e. it would lack the ability to run "mount -o remount,exec,suid /dev").
This only really applies to systems without an initramfs when the kernel mounts /dev over the rootfs it has mounted; with an initramfs, /dev is always mounted by user code.
Just checking, that is the use case you are doing that for?
Correct. We're using this in Chrome OS, which does not use an initramfs.
-Kees
Signature · 2 lines
Kees Cook Chrome OS Security
Quoted 7 lines · On Tue, 20 Nov 2012 16:18:33 -0800 Kees Cook <[email protected]> wrote:
Quoted 6 lines · Kay Sievers
Quoted 4 lines · Kees Cook
Since devtmpfs is writable, make the default noexec,nosuid as well. This protects from the case of a privileged process having an arbitrary file write flaw and an argumentless arbitrary execution (i.e. it would lack the ability to run "mount -o remount,exec,suid /dev").
Quoted 7 lines
Quoted 5 lines
This only really applies to systems without an initramfs when the kernel mounts /dev over the rootfs it has mounted; with an initramfs, /dev is always mounted by user code.
Just checking, that is the use case you are doing that for?
Correct. We're using this in Chrome OS, which does not use an initramfs.
But which has a perfectly good init process of its own, so it's just fine.
If you fix your init to do the work then you can deploy it to all your Google partners and onto existing devices in an update even with old kernels. Far better security practise.
Alan