cmd/snap-confine: handle device cgroup before pivot #7049
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since the dawn of snapd, snap-confine would populate and join the device cgroup nearly just before executing the target application. This is done by interrogating udev for devices with appropriate tags and adding them to the control group. They way they are added to the control group, however, is pretty inefficient, as it involves executing a shell script that writes to /sys/fs/cgroup/devices/devices.allow. The shells script uses the interpreter #!/bin/sh forcing all base snaps to ship shell along with the minimal set of support libraries. Normally that was "okay" with the exception that we recently started adding new base snaps that are otherwise empty, raising the pressure on the need to host the spurious shell. While a better solution would be to write to the device cgroup directly, from snap-confine, ignoring the script, that work is somewhat larger. As a simple alternative, just run the script before we setup and switch to the mount namespace of the snap application. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
jdstrand
approved these changes
Jun 28, 2019
jdstrand
changed the title
|
Looks like this is going to be fun: |
|
Well, it was worth a try. I'll see if this is something obviously silly or more complex. |
|
Interestingly this only failed on fedora, debian, etc but not on any ubuntu system, perhaps it is just a interpreter problem in the script (that now executes on the host). I'll check. |
Since the snap-device-helper script now executes on the host we should not rewrite its interpreter path. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
When snap-confine executes snap-device-helper it does not have a prefix-derived path for it, instead it looks for several fixed locations. Adding /usr/libexec/snapd/snap-device-helper to it allows the code to work correctly on libexec-like systems, like Fedora. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
|
Still fails on opensuse: Bit confusing. IIRC up to the point where the code is executed, there's no changes to the mount ns yet? |
|
After fixing Fedora packaging we have only those errors left: ... and all of them fail on |
|
The reason it failed on Tumbleweed (than god) is because we have apparmor there: We either need to run the helper script unconfined or allow this to be used in the snap-confine template. |
When snap-confine runs snap-device-helper it now does so in the initial
host mount namespace. On openSUSE the shell is linked with libtinfo and
we get a denial, like this:
type=AVC msg=audit(1561974565.075:360): apparmor="DENIED"
operation="open" profile="/usr/lib/snapd/snap-confine"
name="/lib64/libtinfo.so.6.1" pid=7732 comm="snap-device-hel"
requested_mask="r" denied_mask="r" fsuid=0 ouid=0
This patch fixes that by allowing this library to be loaded. As an
alternative that was considered but discarded, udev could run before
apparmor is loaded. I think we will re-visit this once cgroup v2 work
forces changes to snap-device-helper and associated code.
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
jdstrand
approved these changes
Jul 1, 2019
| @@ -38,6 +38,8 @@ | |||
| /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libudev.so* mr, | |||
| /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libseccomp.so* mr, | |||
| /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libcap.so* mr, | |||
| # Needed to run /usr/bin/sh for snap-device-helper. | |||
| /{,usr/}lib{,32,64,x32}/{,@{multiarch}/}libtinfo.so* mr, | |||
There was a problem hiding this comment.
This rule is fine from a security POV, but I wouldn't normally be super excited about it from a maintenance POV, but we know that snap-confine is going to be adjusted to not shell out to snap-device-helper, so this is ok for now (at such time, we should remove the access that only the shell needs, like this one, but I suspect there are more).
bboozzoo
added a commit
to bboozzoo/snapd
that referenced
this pull request
Nov 27, 2019
Drop the workaround for rpmbuild rewriting /bin/sh to /usr/bin/sh. The snap-confine device cgroup setup would call snap-device-helper after pivoting to the snap mount namespace. This used to fail on Fedora because of the mangling done by rpm packaging that changes /bin/sh to /usr/bin/sh. In which, case we would try to run snap-device-helper using an interpreter at /usr/bin/sh that does not exist within the snap mount ns. The downstream packaging defined macros that prevented this scenario. Since canonical#7049, snap-confine calls snap-device-helper before pivoting to the snap mount namespace, thus the host interpreter is still visible at the correct path. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
bboozzoo
added a commit
to bboozzoo/snapd
that referenced
this pull request
Nov 28, 2019
Drop the workaround for rpmbuild rewriting /bin/sh to /usr/bin/sh. The snap-confine device cgroup setup would call snap-device-helper after pivoting to the snap mount namespace. This used to fail on Fedora because of the mangling done by rpm packaging that changes /bin/sh to /usr/bin/sh. In which, case we would try to run snap-device-helper using an interpreter at /usr/bin/sh that does not exist within the snap mount ns. The downstream packaging defined macros that prevented this scenario. Since canonical#7049, snap-confine calls snap-device-helper before pivoting to the snap mount namespace, thus the host interpreter is still visible at the correct path. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since the dawn of snapd, snap-confine would populate and join the device
cgroup nearly just before executing the target application. This is done
by interrogating udev for devices with appropriate tags and adding them
to the control group. They way they are added to the control group,
however, is pretty inefficient, as it involves executing a shell script
that writes to /sys/fs/cgroup/devices/devices.allow. The shells script
uses the interpreter #!/bin/sh forcing all base snaps to ship shell
along with the minimal set of support libraries.
Normally that was "okay" with the exception that we recently started
adding new base snaps that are otherwise empty, raising the pressure on
the need to host the spurious shell.
While a better solution would be to write to the device cgroup directly,
from snap-confine, ignoring the script, that work is somewhat larger.
As a simple alternative, just run the script before we setup and switch
to the mount namespace of the snap application.
Signed-off-by: Zygmunt Krynicki me@zygoon.pl