| Message ID | 1497286620-15027-4-git-send-email-s.mesoraca16@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On 06/12/2017 06:56 PM, Salvatore Mesoraca wrote: > Creation of a new LSM hook that can be used to authorize or deauthorize > new USB devices via the usb authorization interface. > The same hook can also prevent the authorization of a USB device via > "/sys/bus/usb/devices/DEVICE/authorized". > Using this hook an LSM could provide an higher level of granularity > than the current authorization interface. > Could you please explain me why we need LSM for this? There are tools like usbguard[1] and as far as I can tell it looks like they can do everything for you... Without kernel modification... without matching and storing rules inside kernel.. just pure userspace which uses device/interface authorization Footnote: 1 - https://dkopecek.github.io/usbguard/ Best regards,
On Mon, Jun 12, 2017 at 06:56:52PM +0200, Salvatore Mesoraca wrote: > Creation of a new LSM hook that can be used to authorize or deauthorize > new USB devices via the usb authorization interface. > The same hook can also prevent the authorization of a USB device via > "/sys/bus/usb/devices/DEVICE/authorized". > Using this hook an LSM could provide an higher level of granularity > than the current authorization interface. > > Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com> > Cc: linux-usb@vger.kernel.org > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> No, like Krzysztof said, you can already do this today, just fine, from userspace. I think that support has been there for over a decade now, why are you not taking advantage of this already? No need to add extra stuff to the kernel at all to do this, sorry you implemented all of this for no reason :( greg k-h
On 6/12/2017 9:56 AM, Salvatore Mesoraca wrote: > Creation of a new LSM hook that can be used to authorize or deauthorize > new USB devices via the usb authorization interface. > The same hook can also prevent the authorization of a USB device via > "/sys/bus/usb/devices/DEVICE/authorized". > Using this hook an LSM could provide an higher level of granularity > than the current authorization interface. > > Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com> > Cc: linux-usb@vger.kernel.org > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/usb/core/hub.c | 4 ++++ > drivers/usb/core/sysfs.c | 6 +++++- > include/linux/lsm_hooks.h | 6 ++++++ > include/linux/security.h | 7 +++++++ > security/security.c | 5 +++++ > 5 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index b8bb20d..58be4f0 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -28,6 +28,7 @@ > #include <linux/mutex.h> > #include <linux/random.h> > #include <linux/pm_qos.h> > +#include <linux/security.h> > > #include <linux/uaccess.h> > #include <asm/byteorder.h> > @@ -4831,6 +4832,9 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, > if (udev->quirks & USB_QUIRK_DELAY_INIT) > msleep(1000); > > + if (security_usb_device_auth(udev)) > + usb_deauthorize_device(udev); > + > /* consecutive bus-powered hubs aren't reliable; they can > * violate the voltage drop budget. if the new child has > * a "powered" LED, users should notice we didn't enable it > diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c > index dfc68ed..fce9d39 100644 > --- a/drivers/usb/core/sysfs.c > +++ b/drivers/usb/core/sysfs.c > @@ -17,6 +17,7 @@ > #include <linux/usb.h> > #include <linux/usb/quirks.h> > #include <linux/of.h> > +#include <linux/security.h> > #include "usb.h" > > /* Active configuration fields */ > @@ -742,8 +743,11 @@ static ssize_t authorized_store(struct device *dev, > result = -EINVAL; > else if (val == 0) > result = usb_deauthorize_device(usb_dev); > - else > + else { > + if (security_usb_device_auth(usb_dev)) > + return -EPERM; Return the error reported by the hook rather than -EPERM. > result = usb_authorize_device(usb_dev); > + } > return result < 0 ? result : size; > } > static DEVICE_ATTR_IGNORE_LOCKDEP(authorized, S_IRUGO | S_IWUSR, > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index bd274db..cc0937e 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -1189,6 +1189,10 @@ > * to the @parent process for tracing. > * @parent contains the task_struct structure for debugger process. > * Return 0 if permission is granted. > + * @usb_device_auth: > + * Check if @udev device should be authorized or not. > + * @udev contains the usb_device structure for the USB device. > + * Return 0 if the device is allowed. > * @capget: > * Get the @effective, @inheritable, and @permitted capability sets for > * the @target process. The hook may also perform permission checking to > @@ -1352,6 +1356,7 @@ > int (*ptrace_access_check)(struct task_struct *child, > unsigned int mode); > int (*ptrace_traceme)(struct task_struct *parent); > + int (*usb_device_auth)(const struct usb_device *udev); > int (*capget)(struct task_struct *target, kernel_cap_t *effective, > kernel_cap_t *inheritable, kernel_cap_t *permitted); > int (*capset)(struct cred *new, const struct cred *old, > @@ -1670,6 +1675,7 @@ struct security_hook_heads { > struct list_head binder_transfer_file; > struct list_head ptrace_access_check; > struct list_head ptrace_traceme; > + struct list_head usb_device_auth; > struct list_head capget; > struct list_head capset; > struct list_head capable; > diff --git a/include/linux/security.h b/include/linux/security.h > index af675b5..19bc364 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -30,6 +30,7 @@ > #include <linux/string.h> > #include <linux/mm.h> > #include <linux/fs.h> > +#include <linux/usb.h> > > struct linux_binprm; > struct cred; > @@ -196,6 +197,7 @@ int security_binder_transfer_file(struct task_struct *from, > struct task_struct *to, struct file *file); > int security_ptrace_access_check(struct task_struct *child, unsigned int mode); > int security_ptrace_traceme(struct task_struct *parent); > +int security_usb_device_auth(const struct usb_device *udev); > int security_capget(struct task_struct *target, > kernel_cap_t *effective, > kernel_cap_t *inheritable, > @@ -434,6 +436,11 @@ static inline int security_ptrace_traceme(struct task_struct *parent) > return cap_ptrace_traceme(parent); > } > > +static inline int security_usb_device_auth(const struct usb_device *udev) > +{ > + return 0; > +} > + > static inline int security_capget(struct task_struct *target, > kernel_cap_t *effective, > kernel_cap_t *inheritable, > diff --git a/security/security.c b/security/security.c > index 42c8028..e390f99 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -214,6 +214,11 @@ int security_ptrace_traceme(struct task_struct *parent) > return call_int_hook(ptrace_traceme, 0, parent); > } > > +int security_usb_device_auth(const struct usb_device *udev) > +{ > + return call_int_hook(usb_device_auth, 0, udev); > +} > + > int security_capget(struct task_struct *target, > kernel_cap_t *effective, > kernel_cap_t *inheritable,
Hi Salvatore,
[auto build test WARNING on security/next]
[also build test WARNING on v4.12-rc5]
[cannot apply to next-20170609]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Salvatore-Mesoraca/S-A-R-A-Documentation/20170613-050631
base: https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next
config: cris-allyesconfig (attached as .config)
compiler: cris-linux-gcc (GCC) 6.2.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=cris
All warnings (new ones prefixed by >>):
>> security/sara/securityfs.c:24:0: warning: "STR" redefined
#define STR(x) STR_HELPER(x)
In file included from arch/cris/include/asm/irq.h:4:0,
from include/linux/irq.h:26,
from include/asm-generic/hardirq.h:12,
from ./arch/cris/include/generated/asm/hardirq.h:1,
from include/linux/hardirq.h:8,
from include/linux/interrupt.h:12,
from include/linux/usb.h:15,
from include/linux/security.h:33,
from security/sara/securityfs.c:17:
arch/cris/include/arch-v10/arch/irq.h:81:0: note: this is the location of the previous definition
#define STR(x) __STR(x)
vim +/STR +24 security/sara/securityfs.c
4c272b9a Salvatore Mesoraca 2017-06-12 8 * published by the Free Software Foundation.
4c272b9a Salvatore Mesoraca 2017-06-12 9 *
4c272b9a Salvatore Mesoraca 2017-06-12 10 */
4c272b9a Salvatore Mesoraca 2017-06-12 11
4c272b9a Salvatore Mesoraca 2017-06-12 12 #include <linux/ctype.h>
4c272b9a Salvatore Mesoraca 2017-06-12 13 #include <linux/seq_file.h>
4c272b9a Salvatore Mesoraca 2017-06-12 14 #include <linux/uaccess.h>
4c272b9a Salvatore Mesoraca 2017-06-12 15 #include <linux/mm.h>
4c272b9a Salvatore Mesoraca 2017-06-12 16 #include <linux/spinlock.h>
4c272b9a Salvatore Mesoraca 2017-06-12 17 #include <linux/security.h>
4c272b9a Salvatore Mesoraca 2017-06-12 18
4c272b9a Salvatore Mesoraca 2017-06-12 19 #include "include/sara.h"
4c272b9a Salvatore Mesoraca 2017-06-12 20 #include "include/utils.h"
4c272b9a Salvatore Mesoraca 2017-06-12 21 #include "include/securityfs.h"
4c272b9a Salvatore Mesoraca 2017-06-12 22
4c272b9a Salvatore Mesoraca 2017-06-12 23 #define STR_HELPER(x) #x
4c272b9a Salvatore Mesoraca 2017-06-12 @24 #define STR(x) STR_HELPER(x)
4c272b9a Salvatore Mesoraca 2017-06-12 25
4c272b9a Salvatore Mesoraca 2017-06-12 26 static struct dentry *fs_root;
4c272b9a Salvatore Mesoraca 2017-06-12 27
4c272b9a Salvatore Mesoraca 2017-06-12 28 static inline bool check_config_write_access(void)
4c272b9a Salvatore Mesoraca 2017-06-12 29 {
4c272b9a Salvatore Mesoraca 2017-06-12 30 if (unlikely(sara_config_locked && !capable(CAP_MAC_ADMIN))) {
4c272b9a Salvatore Mesoraca 2017-06-12 31 pr_warn("config write access blocked.\n");
4c272b9a Salvatore Mesoraca 2017-06-12 32 return false;
:::::: The code at line 24 was first introduced by commit
:::::: 4c272b9af7b3b969359f7c0dc41fd2a8a03be9fc S.A.R.A. framework creation
:::::: TO: Salvatore Mesoraca <s.mesoraca16@gmail.com>
:::::: CC: 0day robot <fengguang.wu@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Salvatore,
[auto build test ERROR on security/next]
[also build test ERROR on v4.12-rc5]
[cannot apply to next-20170609]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Salvatore-Mesoraca/S-A-R-A-Documentation/20170613-050631
base: https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64
All errors (new ones prefixed by >>):
>> ERROR: "security_usb_device_auth" [drivers/usb/core/usbcore.ko] undefined!
ERROR: "ia64_delay_loop" [drivers/spi/spi-thunderx.ko] undefined!
ERROR: "ia64_delay_loop" [drivers/net/phy/mdio-cavium.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
2017-06-12 19:35 GMT+02:00 Krzysztof Opasiak <k.opasiak@samsung.com>: > Could you please explain me why we need LSM for this? > > There are tools like usbguard[1] and as far as I can tell it looks like they > can do everything for you... I have to admit that this is the first time I read about usbguard and it made me realize that this feature can be implemented in userspace, so no need to modify the kernel. :) You are right. Thank you very much for taking the time to answer me.
2017-06-12 21:38 GMT+02:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > No, like Krzysztof said, you can already do this today, just fine, from > userspace. I think that support has been there for over a decade now, > why are you not taking advantage of this already? > No need to add extra stuff to the kernel at all to do this Honesty, now that you mention it, I agree with you. This feature can be implemented in userspace and I'm the first one to think that what can be implemented in userspace should stay in userspace. > sorry you > implemented all of this for no reason :( Don't worry, usb filtering is just a small part of this patchset. Let's hope the rest will end up being more useful. :) Thank you very much for taking the time to answer me.
2017-06-12 23:31 GMT+02:00 Casey Schaufler <casey@schaufler-ca.com>:
> Return the error reported by the hook rather than -EPERM.
Agreed, anyway this part will be, probably, dropped in
the next version (read Greg and Krzysztof answers).
I'm sorry :(
Thank you very much for the time you spent on this.
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index b8bb20d..58be4f0 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -28,6 +28,7 @@ #include <linux/mutex.h> #include <linux/random.h> #include <linux/pm_qos.h> +#include <linux/security.h> #include <linux/uaccess.h> #include <asm/byteorder.h> @@ -4831,6 +4832,9 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, if (udev->quirks & USB_QUIRK_DELAY_INIT) msleep(1000); + if (security_usb_device_auth(udev)) + usb_deauthorize_device(udev); + /* consecutive bus-powered hubs aren't reliable; they can * violate the voltage drop budget. if the new child has * a "powered" LED, users should notice we didn't enable it diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c index dfc68ed..fce9d39 100644 --- a/drivers/usb/core/sysfs.c +++ b/drivers/usb/core/sysfs.c @@ -17,6 +17,7 @@ #include <linux/usb.h> #include <linux/usb/quirks.h> #include <linux/of.h> +#include <linux/security.h> #include "usb.h" /* Active configuration fields */ @@ -742,8 +743,11 @@ static ssize_t authorized_store(struct device *dev, result = -EINVAL; else if (val == 0) result = usb_deauthorize_device(usb_dev); - else + else { + if (security_usb_device_auth(usb_dev)) + return -EPERM; result = usb_authorize_device(usb_dev); + } return result < 0 ? result : size; } static DEVICE_ATTR_IGNORE_LOCKDEP(authorized, S_IRUGO | S_IWUSR, diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index bd274db..cc0937e 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1189,6 +1189,10 @@ * to the @parent process for tracing. * @parent contains the task_struct structure for debugger process. * Return 0 if permission is granted. + * @usb_device_auth: + * Check if @udev device should be authorized or not. + * @udev contains the usb_device structure for the USB device. + * Return 0 if the device is allowed. * @capget: * Get the @effective, @inheritable, and @permitted capability sets for * the @target process. The hook may also perform permission checking to @@ -1352,6 +1356,7 @@ int (*ptrace_access_check)(struct task_struct *child, unsigned int mode); int (*ptrace_traceme)(struct task_struct *parent); + int (*usb_device_auth)(const struct usb_device *udev); int (*capget)(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted); int (*capset)(struct cred *new, const struct cred *old, @@ -1670,6 +1675,7 @@ struct security_hook_heads { struct list_head binder_transfer_file; struct list_head ptrace_access_check; struct list_head ptrace_traceme; + struct list_head usb_device_auth; struct list_head capget; struct list_head capset; struct list_head capable; diff --git a/include/linux/security.h b/include/linux/security.h index af675b5..19bc364 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -30,6 +30,7 @@ #include <linux/string.h> #include <linux/mm.h> #include <linux/fs.h> +#include <linux/usb.h> struct linux_binprm; struct cred; @@ -196,6 +197,7 @@ int security_binder_transfer_file(struct task_struct *from, struct task_struct *to, struct file *file); int security_ptrace_access_check(struct task_struct *child, unsigned int mode); int security_ptrace_traceme(struct task_struct *parent); +int security_usb_device_auth(const struct usb_device *udev); int security_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, @@ -434,6 +436,11 @@ static inline int security_ptrace_traceme(struct task_struct *parent) return cap_ptrace_traceme(parent); } +static inline int security_usb_device_auth(const struct usb_device *udev) +{ + return 0; +} + static inline int security_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, diff --git a/security/security.c b/security/security.c index 42c8028..e390f99 100644 --- a/security/security.c +++ b/security/security.c @@ -214,6 +214,11 @@ int security_ptrace_traceme(struct task_struct *parent) return call_int_hook(ptrace_traceme, 0, parent); } +int security_usb_device_auth(const struct usb_device *udev) +{ + return call_int_hook(usb_device_auth, 0, udev); +} + int security_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable,
Creation of a new LSM hook that can be used to authorize or deauthorize new USB devices via the usb authorization interface. The same hook can also prevent the authorization of a USB device via "/sys/bus/usb/devices/DEVICE/authorized". Using this hook an LSM could provide an higher level of granularity than the current authorization interface. Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com> Cc: linux-usb@vger.kernel.org Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/usb/core/hub.c | 4 ++++ drivers/usb/core/sysfs.c | 6 +++++- include/linux/lsm_hooks.h | 6 ++++++ include/linux/security.h | 7 +++++++ security/security.c | 5 +++++ 5 files changed, 27 insertions(+), 1 deletion(-)