Skip to content

Mask /sys/firmware using read-only tmpfs#26689

Closed
AkihiroSuda wants to merge 1 commit into
moby:masterfrom
AkihiroSuda:mask-sys-firmware
Closed

Mask /sys/firmware using read-only tmpfs#26689
AkihiroSuda wants to merge 1 commit into
moby:masterfrom
AkihiroSuda:mask-sys-firmware

Conversation

@AkihiroSuda

@AkihiroSuda AkihiroSuda commented Sep 18, 2016

Copy link
Copy Markdown
Member

- What I did
TestBuildApiDockerFileRemote has been consistently failing (EPERM) on the host with #26618, which prohibits /sys/firmware from being accessed using apparmor.
https://github.com/docker/docker/blob/8a46c18dd4e4f8091b9e02ef5b476e8b8aa77c47/integration-cli/docker_api_build_test.go#L19

RUN find / -name ba*
find: /sys/firmware/dmi: Permission denied
find: /sys/firmware/acpi: Permission denied
find: /sys/firmware/memmap: Permission denied

This PR masks /sys/firmware using a read-only tmpfs so that TestBuildApiDockerFileRemote passes without a change.

- How I did it
Masked /sys/firmware using a read-only tmpfs.

- How to verify it
Make sure the latest apparmor profile is installed to the host:

$ cat /etc/apparmor.d/docker
...
profile docker-default flags=(attach_disconnected,mediate_deleted) {
...
  deny /sys/firmware/** rwklx,
...
}

Then execute tests

$ TESTFLAGS='-check.f TestBuildApiDockerFileRemote' make test-integration-cli
$ TESTFLAGS='-check.f TestRunContainerWithSysFirmwareMountRO' make test-integration-cli

Or just make sure that docker run -it --rm busybox sh -c 'find / > /dev/null' passes without any error.

- Description for the changelog

Mask /sys/firmware using read-only tmpfs

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Akihiro Suda [email protected]

@AkihiroSuda
Mask /sys/firmware using read-only tmpfs
d02da45 
Since 693b4ac which prohibits /sys/firmware from being accessed,
`find /` has been failing due to EPERM.

Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda AkihiroSuda mentioned this pull request Sep 18, 2016
Closed
@AkihiroSuda

AkihiroSuda commented Sep 18, 2016

Copy link
Copy Markdown
Member Author

This PR depends on opencontainers/runc#1054

@AkihiroSuda

AkihiroSuda commented Sep 18, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

I fear this approach would need many tmpfs if we mask other /sys/foobar or /proc/foobar as well in the future.

So maybe there is a chance to accept EPERM as a breaking change, but I don't like to break the compatibility. Not sure how many folks can hit such EPERM...

@justincormack

justincormack commented Sep 19, 2016

Copy link
Copy Markdown
Contributor

The test should be using find -xdev I would think - it is fine for these system directories to have inaccessible files.

@AkihiroSuda AkihiroSuda mentioned this pull request Sep 19, 2016
Merged
@AkihiroSuda

AkihiroSuda commented Sep 19, 2016

Copy link
Copy Markdown
Member Author

@justincormack OK, opened #26720
I'm closing this PR.

@justincormack

justincormack commented Sep 19, 2016

Copy link
Copy Markdown
Contributor

There is still an issue that if users do not have apparmor these paths are not being blocked, and this is one solution, but it would need to be applied more completely. There may be other solutions though, looking at the options.

@AkihiroSuda

AkihiroSuda commented Sep 20, 2016

Copy link
Copy Markdown
Member Author

@justincormack
Yes, and I was thinking of adding maskedDirPath (equivalent of maskedPath for directories) in the OCI spec, as a much more generic approach.
https://github.com/opencontainers/runtime-spec/blob/9c0fcee0b8d620f1fe7d5032e5f674fe2feda09f/config-linux.md#masked-paths

Some questions:

  • Should we mask other /{sys,proc}/foobar as well? I feel currently too many entries under /sys and /proc are exposed, but not sure they can be a vulnerability.
  • Is it technically right approach to use read-only tmpfs for masking directories? Is there any impact on performance?

@AkihiroSuda

AkihiroSuda commented Sep 23, 2016

Copy link
Copy Markdown
Member Author

@justincormack
Opened PRs in OCI, please look into them?
opencontainers/runtime-spec#582
opencontainers/runc#1068

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

Reviewers

No reviews

Assignees

No one assigned

Labels

area/security status/needs-vendoring status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AkihiroSuda @justincormack @thaJeztah @GordonTheTurtle