Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Interfaces: hardware-observe #1563
Conversation
cwayne18
added some commits
Jul 18, 2016
cwayne18
changed the title from
Hardware observe
to
Interfaces: hardware-observe
Jul 18, 2016
zyga
reviewed
Jul 18, 2016
| +/bin/udevadm ixr, | ||
| +/bin/lsblk ixr, | ||
| +/usr/sbin/dmidecode ixr, | ||
| +/usr/bin/lsusb ixr, |
zyga
Jul 18, 2016
Contributor
Is lsusb actually in the image? I would say that after having the permissions granted by this interface it would be better to ship the software that has no daemon component (not udev) as a snap itself.
zyga
reviewed
Jul 18, 2016
| + | ||
| +/bin/udevadm ixr, | ||
| +/bin/lsblk ixr, | ||
| +/usr/sbin/dmidecode ixr, |
zyga
Jul 18, 2016
Contributor
I know why we use it but this is legacy and I don't even know if it is in the image.
morphis
reviewed
Jul 18, 2016
| +// http://bazaar.launchpad.net/~ubuntu-security/ubuntu-core-security/trunk/view/head:/data/apparmor/policygroups/ubuntu-core/16.04/log-observe | ||
| +const hardwareObserveConnectedPlugAppArmor = ` | ||
| +# Description: This interface allows for getting hardware information | ||
| +# from the system, as is needed by checkbox on snappy. This is reserved for OS snap. |
morphis
reviewed
Jul 18, 2016
| +/var/lib/usbutils/usb.ids r, | ||
| +/sys/firmware/dmi/tables/DMI r, | ||
| +/sys/firmware/dmi/tables/smbios_entry_point r, | ||
| + |
|
Please patch |
cwayne18
added some commits
Jul 19, 2016
|
Looks good to me. Let's merge it after Jamie asks it. |
jdstrand
reviewed
Jul 25, 2016
| +#include <abstractions/base> | ||
| + | ||
| +/etc/udev/udev.conf r, | ||
| +@{PROC}/*/stat r, |
jdstrand
reviewed
Jul 25, 2016
| +/etc/udev/udev.conf r, | ||
| +@{PROC}/*/stat r, | ||
| +/run/udev/data/* r, | ||
| +/sys/bus/ r, |
jdstrand
reviewed
Jul 25, 2016
| +/run/udev/data/* r, | ||
| +/sys/bus/ r, | ||
| +/sys/bus/**/ r, | ||
| +/sys/class/ r, |
jdstrand
reviewed
Jul 25, 2016
| +/var/lib/usbutils/usb.ids r, | ||
| +/sys/firmware/dmi/tables/DMI r, | ||
| +/sys/firmware/dmi/tables/smbios_entry_point r, | ||
| +` |
jdstrand
Jul 25, 2016
Contributor
I suggest you clean this up to be:
#include <abstractions/base>
# files in /sys pertaining to hardware
/sys/{block,bus,class,devices}/{,**} r,
# USB IDs
/var/lib/usbutils/usb.ids r,
# DMI tables
/sys/firmware/dmi/tables/DMI r,
/sys/firmware/dmi/tables/smbios_entry_point r,
jdstrand
reviewed
Jul 25, 2016
| +/sys/class/ r, | ||
| +/sys/class/*/ r, | ||
| +/sys/devices/** r, | ||
| +@{PROC}/*/mountinfo r, |
jdstrand
reviewed
Jul 25, 2016
| +/sys/class/*/ r, | ||
| +/sys/devices/** r, | ||
| +@{PROC}/*/mountinfo r, | ||
| +@{PROC}/swaps r, |
jdstrand
Jul 25, 2016
Contributor
This should be in mount-observe but is currently not. I'll do a PR for that.
jdstrand
reviewed
Jul 25, 2016
| +/sys/block/ r, | ||
| +/sys/devices/** r, | ||
| +/dev/bus/usb/ r, | ||
| +/dev/bus/usb/** r, |
jdstrand
Jul 25, 2016
Contributor
These are tricky. Read access is potentially more than observe for these files. Reading https://www.kernel.org/doc/Documentation/hid/hiddev.txt I think a /dev/bus/usb/hiddev* r, rule would be ok, perhaps with an additional seccomp arg filtering on ioctl. You used /dev/bus/usb/** r, though, why?
cwayne18
Jul 25, 2016
Contributor
@jdstrand so the context for this is basically checkbox pulling in device info via lsusb, I got this by running aa-genprof for lsusb, if you think /dev/bus/usb/hiddev* would be better, I could try that out, though I'd have no idea how to do the additional seccomp filtering, do we have any docs for that?
|
@cwayne18 - can you comment on whether or not we should allow all ioctls in https://www.kernel.org/doc/Documentation/hid/hiddev.txt or a subset? |
jdstrand
reviewed
Jul 25, 2016
| +# files in /sys pertaining to hardware | ||
| +/sys/{block,bus,class,devices}/{,**} r, | ||
| + | ||
| +# USB IDs | ||
| /var/lib/usbutils/usb.ids r, |
jdstrand
Jul 25, 2016
Contributor
Thanks for the cleanup! It just occurred to me that /var/lib/usbutils/usb.ids isn't going to exist on an all-snaps image or within the runtime environment of the snap so I think it doesn't make sense to have this rule?
cwayne18
Jul 25, 2016
Contributor
Ah, you're right, and in fact we can use a usb.ids from within the snap as it is, I'll remove this line
jdstrand
reviewed
Jul 25, 2016
| +# from the system. This is reserved for OS snap. | ||
| +# Usage: reserved | ||
| + | ||
| +#include <abstractions/base> |
jdstrand
Jul 25, 2016
Contributor
I missed this before-- please drop the base abstraction-- it is part of the default template (see interfaces/apparmor/template.go).
jdstrand
reviewed
Jul 25, 2016
| +// http://bazaar.launchpad.net/~ubuntu-security/ubuntu-core-security/trunk/view/head:/data/apparmor/policygroups/ubuntu-core/16.04/log-observe | ||
| +const hardwareObserveConnectedPlugAppArmor = ` | ||
| +# Description: This interface allows for getting hardware information | ||
| +# from the system. This is reserved for OS snap. |
jdstrand
Jul 25, 2016
•
Edited 1 time
-
jdstrand
Jul 25, 2016
Contributor
-
Jul 25, 2016
Please change "This is reserved for the OS snap" to "this is reserved because it allows reading potentially sensitive information".
cwayne18
added some commits
Jul 25, 2016
|
Security policy LGTM after the recent changes. +1 |
|
LGTM then. Thank you for the in-depth review @jdstrand |
cwayne18 commentedJul 18, 2016
This adds an interface to pull some hardware information from the system, and is needed to be able to run checkbox on a snappy system without --devmode.