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

Add /proc/scsi to masked paths #35399

Merged
merged 1 commit into from Nov 3, 2017

Conversation

Projects
None yet
@justincormack
Contributor

justincormack commented Nov 3, 2017

This is writeable, and can be used to remove devices. Containers do
not need to know about scsi devices.

Signed-off-by: Justin Cormack justin.cormack@docker.com

1411-masked-dog

Add /proc/scsi to masked paths
This is writeable, and can be used to remove devices. Containers do
not need to know about scsi devices.

Signed-off-by: Justin Cormack <justin.cormack@docker.com>
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Nov 3, 2017

Contributor

LGTM

Contributor

crosbymichael commented Nov 3, 2017

LGTM

@thaJeztah

LGTM

@yongtang yongtang merged commit a8cefcf into moby:master Nov 3, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37698 has succeeded
Details
janky Jenkins build Docker-PRs 46402 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6813 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17967 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6620 has succeeded
Details

@justincormack justincormack deleted the justincormack:mask-scsi branch Nov 4, 2017

@vielmetti

This comment has been minimized.

Show comment
Hide comment
@vielmetti

vielmetti Nov 4, 2017

https://twitter.com/ewindisch/status/926443182010916865

Is there a CVE, so that this gets properly handled upstream and downstream?

vielmetti commented Nov 4, 2017

https://twitter.com/ewindisch/status/926443182010916865

Is there a CVE, so that this gets properly handled upstream and downstream?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 4, 2017

Member

I'm not sure if a CVE was opened for the kernel

Member

thaJeztah commented Nov 4, 2017

I'm not sure if a CVE was opened for the kernel

AkihiroSuda added a commit to AkihiroSuda/containerd that referenced this pull request Nov 4, 2017

Add /proc/scsi to masked paths
Port over moby/moby#35399

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda added a commit to AkihiroSuda/runc that referenced this pull request Nov 4, 2017

specconv.Example(): add /proc/scsi to masked paths
Port over moby/moby#35399

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@vielmetti

This comment has been minimized.

Show comment
Hide comment
@vielmetti

vielmetti Nov 4, 2017

CVE-2017-16539

vielmetti commented Nov 4, 2017

CVE-2017-16539

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 4, 2017

Member

Thanks!

Member

thaJeztah commented Nov 4, 2017

Thanks!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 4, 2017

Member

I wonder if it's correct that he CVE is reported against Moby, as it's a kernel vulnerability; the patch here is just to work around that; http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-16539

Member

thaJeztah commented Nov 4, 2017

I wonder if it's correct that he CVE is reported against Moby, as it's a kernel vulnerability; the patch here is just to work around that; http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-16539

@vielmetti

This comment has been minimized.

Show comment
Hide comment
@vielmetti

vielmetti Nov 4, 2017

Yeah, probably the kernel gets the real blame, but I don't think there's a clear understanding yet of which piece of code would need to be fixed.

vielmetti commented Nov 4, 2017

Yeah, probably the kernel gets the real blame, but I don't think there's a clear understanding yet of which piece of code would need to be fixed.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Nov 4, 2017

Kernel patch was created by @cyphar here; https://marc.info/?l=linux-scsi&m=150982199728895&w=2

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 4, 2017

Contributor

@thaJeztah I think that Docker is the right component to be filed against (though it should be noted that the default AppArmor and SELinux setup actually protects against this attack -- so you'd have to misconfigure your system in order to make it exploitable) since we don't use user namespaces by default and we run images as root by default (with CAP_DAC_OVERRIDE enabled). If any of those things weren't true this attack couldn't work even with a misconfigured system.

Contributor

cyphar commented Nov 4, 2017

@thaJeztah I think that Docker is the right component to be filed against (though it should be noted that the default AppArmor and SELinux setup actually protects against this attack -- so you'd have to misconfigure your system in order to make it exploitable) since we don't use user namespaces by default and we run images as root by default (with CAP_DAC_OVERRIDE enabled). If any of those things weren't true this attack couldn't work even with a misconfigured system.

@vielmetti

This comment has been minimized.

Show comment
Hide comment
@vielmetti

vielmetti Nov 4, 2017

Based on this conversation

https://twitter.com/ewindisch/status/926888008015638530

there's a variant on this attack (known as #GroceryShoppingWithMyKids - and every bug gets an animated GIF) where the attacker writes into /proc/scsi/device_info and can also "write into this arbitrary data append only and DOS kernel via memory allocations".

I've send in a note to update the CVE to reference the patch from @cyphar which I believe addresses this variant as well.

vielmetti commented Nov 4, 2017

Based on this conversation

https://twitter.com/ewindisch/status/926888008015638530

there's a variant on this attack (known as #GroceryShoppingWithMyKids - and every bug gets an animated GIF) where the attacker writes into /proc/scsi/device_info and can also "write into this arbitrary data append only and DOS kernel via memory allocations".

I've send in a note to update the CVE to reference the patch from @cyphar which I believe addresses this variant as well.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 4, 2017

Contributor

@vielmetti That is also protected against by the default AppArmor and SELinux profiles (so the same "misconfigured" and "our defaults really should be better if it weren't for legacy reasons" caveats as above). And yes, my patch fixes that issue from the kernel-side as well.

Contributor

cyphar commented Nov 4, 2017

@vielmetti That is also protected against by the default AppArmor and SELinux profiles (so the same "misconfigured" and "our defaults really should be better if it weren't for legacy reasons" caveats as above). And yes, my patch fixes that issue from the kernel-side as well.

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Nov 8, 2017

Collaborator

@justincormack would it be realistic to write a test of this ?

Collaborator

vieux commented Nov 8, 2017

@justincormack would it be realistic to write a test of this ?

@mghazizadeh

This comment has been minimized.

Show comment
Hide comment
@mghazizadeh

mghazizadeh Nov 19, 2017

how's this tested?

mghazizadeh commented Nov 19, 2017

how's this tested?

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 19, 2017

Contributor

Well, you could start a container and check whether /proc/scsi is a tmpfs mount (or check that it's empty). Not sure it's worth it though -- we have tests in runc that make sure masked paths work properly and this just uses maskedPaths in the OCI configuration, so Docker isn't actually doing anything here.

Contributor

cyphar commented Nov 19, 2017

Well, you could start a container and check whether /proc/scsi is a tmpfs mount (or check that it's empty). Not sure it's worth it though -- we have tests in runc that make sure masked paths work properly and this just uses maskedPaths in the OCI configuration, so Docker isn't actually doing anything here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment