Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

interfaces/builtin: add add exec "/" to docker-support #6610

Merged

Conversation

anonymouse64
Copy link
Contributor

This access is needed to allow recent versions of runC inside docker
18.06.3 and 18.09.3 to run properly. Note that the snap is not affected
by CVE-2019-5736 directly, but this access is needed so that the
mitigation works properly.

Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This weird access has to do with memfd sealing. There is a kernel issue where the path is not visible to AppArmor and (we believe) because we're using attach_disconnected, it gets mapped to /. The same access was needed for greengrass. For now this is fine to add, but can you create a bug against the apparmor project and reference LP: #... here?

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, agree with jdstrand about the bug report.

@jdstrand is there a kernel bug tracking this? Should we file one?

@mvo5 mvo5 added this to the 2.38 milestone Mar 15, 2019
@jdstrand
Copy link

+1, agree with jdstrand about the bug report.

@jdstrand is there a kernel bug tracking this? Should we file one?

With @anonymouse64 filing a bug against apparmor, that is enough for jj to look into and escalate if it is a larger kernel/LSM issue as needed (it might be apparmor-specific, don't know yet).

This access is needed to allow recent versions of runC inside docker
18.06.3 and 18.09.3 to run properly. Note that the snap is not affected
by CVE-2019-5736 directly, but this access is needed so that the
mitigation works properly.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
@anonymouse64 anonymouse64 force-pushed the bugfix/docker-support-runc-cve-2019-5736 branch from 42f60f9 to 616333e Compare March 15, 2019 19:41
@anonymouse64
Copy link
Contributor Author

I filed apparmor bug https://bugs.launchpad.net/apparmor/+bug/1820344 for this

@codecov-io
Copy link

Codecov Report

Merging #6610 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6610      +/-   ##
==========================================
+ Coverage   79.16%   79.22%   +0.05%     
==========================================
  Files         592      592              
  Lines       45461    45464       +3     
==========================================
+ Hits        35991    36017      +26     
+ Misses       6559     6532      -27     
- Partials     2911     2915       +4
Impacted Files Coverage Δ
interfaces/builtin/docker_support.go 74.28% <ø> (ø) ⬆️
overlord/ifacestate/handlers.go 63.98% <0%> (-0.22%) ⬇️
overlord/hookstate/hookmgr.go 74.51% <0%> (+0.96%) ⬆️
cmd/snap/cmd_snapshot.go 68.34% <0%> (+16.13%) ⬆️
cmd/snap/times.go 75% <0%> (+25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0aee9e...36296d3. Read the comment docs.

@mvo5 mvo5 merged commit d4c4994 into canonical:master Mar 20, 2019
tianon added a commit to infosiftr/snapd that referenced this pull request Jul 10, 2019
Newer runC applied further improvements to their CVE-2019-5736 mitigation in opencontainers/runc#1984 which change the nature of our apparmor denial from `/` to `/bin/runc` (which I have also commented on https://bugs.launchpad.net/apparmor/+bug/1820344 about).

See also canonical#6610.

Signed-off-by: Tianon Gravi <tianon@debian.org>
@anonymouse64 anonymouse64 deleted the bugfix/docker-support-runc-cve-2019-5736 branch July 11, 2019 13:14
tianon added a commit to infosiftr/snapd that referenced this pull request Jul 11, 2019
Newer runC applied further improvements to their CVE-2019-5736 mitigation in opencontainers/runc#1984 which change the nature of our apparmor denial from `/` to `/bin/runc` (which I have also commented on https://bugs.launchpad.net/apparmor/+bug/1820344 about).

See also canonical#6610.

Signed-off-by: Tianon Gravi <tianon@debian.org>
tianon added a commit to infosiftr/snapd that referenced this pull request Jul 12, 2019
Newer runC applied further improvements to their CVE-2019-5736 mitigation in opencontainers/runc#1984 which change the nature of our apparmor denial from `/` to `/bin/runc` (which I have also commented on https://bugs.launchpad.net/apparmor/+bug/1820344 about).

See also canonical#6610.

Signed-off-by: Tianon Gravi <tianon@debian.org>
anonymouse64 added a commit to anonymouse64/snapd that referenced this pull request Jul 12, 2019
Newer runC applied further improvements to their CVE-2019-5736 mitigation in opencontainers/runc#1984 which change the nature of our apparmor denial from `/` to `/bin/runc` (which I have also commented on https://bugs.launchpad.net/apparmor/+bug/1820344 about).

See also canonical#6610.

(originally from Tianon Gravi, but re-committed due to CLA issues with the PR checks)

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
mvo5 pushed a commit that referenced this pull request Jul 15, 2019
Newer runC applied further improvements to their CVE-2019-5736 mitigation in opencontainers/runc#1984 which change the nature of our apparmor denial from `/` to `/bin/runc` (which I have also commented on https://bugs.launchpad.net/apparmor/+bug/1820344 about).

See also #6610.

(originally from Tianon Gravi, but re-committed due to CLA issues with the PR checks)

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants