8000 Add /proc/acpi to masked paths by runcom · Pull Request #37404 · moby/moby · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add /proc/acpi to masked paths #37404

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 6, 2018
Merged

Add /proc/acpi to masked paths #37404

merged 1 commit into from
Jul 6, 2018

Conversation

runcom
Copy link
Member
@runcom runcom commented Jul 6, 2018 •
edited
Loading

The deafult OCI linux spec in oci/defaults{_linux}.go in Docker/Moby
from 1.11 to current upstream master does not block /proc/acpi pathnames
allowing attackers to modify host's hardware like enabling/disabling
bluetooth or turning up/down keyboard brightness and probably other stuff. SELinux prevents all
of this if enabled. Probably apparmor does prevent this as well but I haven't tested.

This is reproducible with just:

docker run --rm -ti alpine sh -c 'echo "2" >/proc/acpi/ibm/kbdlight' (turns up the brightness of the kbd)
docker run --rm -ti alpine sh -c 'echo "enable" >/proc/acpi/ibm/bluetooth' (enable bluetooth)

We've reserved CVE-2018-10892 for this.

@thaJeztah @cyphar et all, PTAL

Signed-off-by: Antonio Murdaca runcom@redhat.com

@runcom
Add /proc/acpi to masked paths
569b970 
The deafult OCI linux spec in oci/defaults{_linux}.go in Docker/Moby
from 1.11 to current upstream master does not block /proc/acpi pathnames
allowing attackers to modify host's hardware like enabling/disabling
bluetooth or turning up/down keyboard brightness. SELinux prevents all
of this if enabled.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom runcom requested a review from tonistiigi July 6, 2018 09:54
@cyphar
Copy link
Contributor
cyphar commented Jul 6, 2018 •
edited
Loading

Probably apparmor does prevent this as well but I haven't tested.

AppArmor does protect against this (it protects against all writes to /proc files unless they are inside /proc/sys/ or /proc/<pid>/). I've tested this on openSUSE Tumbleweed and /proc/acpi writes are disallowed inside containers.

LGTM.

@runcom
Copy link
Member Author
runcom commented Jul 6, 2018

failures seems to be unrelated

@n4ss
Copy link
n4ss commented Jul 6, 2018

LGTM!

This was referenced Jul 6, 2018
Merged
Closed
cpuguy83
cpuguy83 approved these changes Jul 6, 2018
View reviewed changes
Copy link
Member
@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah mentioned this pull request Jul 6, 2018
Open
andrewhsu
andrewhsu approved these changes Jul 6, 2018
View reviewed changes
Copy link
Contributor
@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM

thaJeztah
thaJeztah approved these changes Jul 6, 2018
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link
codecov bot commented Jul 6, 2018 •
edited
Loading

Codecov Report

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

@@            Coverage Diff             @@
##           master   #37404      +/-   ##
==========================================
+ Coverage   34.91%   34.93%   +0.01%     
==========================================
  Files         610      610              
  Lines       44884    44884              
==========================================
+ Hits        15672    15679       +7     
+ Misses      27092    27087       -5     
+ Partials     2120     2118       -2

@thaJeztah thaJeztah merged commit 86a41e4 into moby:master Jul 6, 2018
@runcom runcom deleted the no-acpi branch July 6, 2018 22:13
@n4ss
Copy link
n4ss commented Jul 7, 2018

Additional Information on CVE-2018-10892

Since this is linked from the CVE (which I'll make sure we amend and add additional documentation because the description shows extremely small understanding of what ACPI is and can be used for, as well as the use of the procfs interface for this purpose) and people will visit this issue, a few key points:

  1. Both SELinux and AppArmor with the default profiles protect against this (if you use an official Docker packaging and don't disable security features, you're safe).
  2. The /proc/acpi interface is either removed or deprecated since 2011/2012 of almost every kernel / drivers (if you use a kernel > 2.6 - current version is 4.17.4 - and no legacy/bad drivers, you're safe). [1]
  3. If your /proc/acpi is not empty (except button on some laptops for the LiD light), please report it to your distribution or driver provider support

[1] Please see the note on the kernel config option that gates all /proc/acpi population: https://github.com/torvalds/linux/blob/master/drivers/acpi/Kconfig#L100

@thaJeztah
Copy link
Member

Locking the conversation on this PR, as it got a bit of attention on Twitter, and I want to prevent a long discussion from hiding that summary.

Feel free to discuss on Slack, the forums, or open a new ticket it more discussion is needed

@moby moby locked and limited conversation to collaborators Jul 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@andrewhsu andrewhsu andrewhsu approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@n4ss n4ss n4ss approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi

Assignees
No one assigned
Labels
area/runtime area/security impact/changelog status/4-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@runcom @cyphar @n4ss @thaJeztah @andrewhsu @cpuguy83 @GordonTheTurtle
0