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

Add SCMP_ACT_LOG as a valid Seccomp action #1951

Merged
merged 2 commits into from
Sep 26, 2019

Conversation

blacktop
Copy link
Contributor

This enables logging of all system calls in a container. This could be useful for creating minimal seccomp profiles, and is the only Seccomp action runc doesn't support at present.

@blacktop
Copy link
Contributor Author

REF moby/moby#38333

@blacktop blacktop force-pushed the master branch 2 times, most recently from 190c60b to 1039b14 Compare December 18, 2018 02:58
@blacktop
Copy link
Contributor Author

This is failing because it requires seccomp/libseccomp-golang#29 to be merged 😢

@blacktop blacktop force-pushed the master branch 3 times, most recently from 673434d to 0ca09a8 Compare April 10, 2019 00:50
@blacktop
Copy link
Contributor Author

😁 🎉

@thaJeztah
Copy link
Member

@justincormack PTAL

@blacktop
Copy link
Contributor Author

@justincormack do you need me to do anything else?

@blacktop
Copy link
Contributor Author

ping @justincormack

1 similar comment
@blacktop
Copy link
Contributor Author

ping @justincormack

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.

found one unrelated change; could you also squash your commits?

Dockerfile Outdated
@@ -67,4 +67,4 @@ ENTRYPOINT ["/tmpmount"]
ADD . /go/src/github.com/opencontainers/runc

RUN . tests/integration/multi-arch.bash \
&& curl -o- -sSL `get_busybox` | tar xfJC - ${ROOTFS}
&& curl -o- -sSL `get_busybox` | tar xfJC - ${ROOTFS}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this inadvertently removed a newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed them and learned more about git squashing ;)

@blacktop blacktop force-pushed the master branch 2 times, most recently from fab554d to 0c82a88 Compare June 18, 2019 01:00
Signed-off-by: blacktop <blacktop@users.noreply.github.com>
@thaJeztah
Copy link
Member

Thanks for squashing 👍
I'm not a maintainer in this repository, but I noticed that you used your GitHub handle in your sign-off;

Signed-off-by: blacktop <blacktop@users.noreply.github.com>

Perhaps you can change to your real name? (see contributing.md;

using your real name (sorry, no pseudonyms or anonymous contributions.)

Changes themselves SGTM, but I'll try to get @justincormack and/or @crosbymichael to have a look

@crosbymichael
Copy link
Member

crosbymichael commented Jun 18, 2019

LGTM

Approved with PullApprove

@pjbgf
Copy link

pjbgf commented Sep 20, 2019

@crosbymichael @thaJeztah @justincormack is there anything else left before we can merge this?

@AkihiroSuda
Copy link
Member

@cyphar @mrunalp PTAL

@blacktop
Copy link
Contributor Author

@crosbymichael @thaJeztah @justincormack I have rebased so we should be good to go now?

@thaJeztah
Copy link
Member

Looks like there's a merge-commit in your branch; did you do a rebase, or a merge?

@mrunalp
Copy link
Contributor

mrunalp commented Sep 26, 2019

LGTM

Approved with PullApprove

1 similar comment
@crosbymichael
Copy link
Member

crosbymichael commented Sep 26, 2019

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit 84373aa into opencontainers:master Sep 26, 2019
@blacktop
Copy link
Contributor Author

We did it!!!!

👍 😎 👍

thaJeztah added a commit to thaJeztah/docker that referenced this pull request Nov 28, 2019
full diff: opencontainers/runc@3e425f8...v1.0.0-rc9

- opencontainers/runc#1951 Add SCMP_ACT_LOG as a valid Seccomp action
- opencontainers/runc#2130 *: verify operations on /proc/... are on procfs
  This is an additional mitigation for CVE-2019-16884. The primary problem
  is that Docker can be coerced into bind-mounting a file system on top of
  /proc (resulting in label-related writes to /proc no longer happening).

  While we are working on mitigations against permitting the mounts, this
  helps avoid our code from being tricked into writing to non-procfs
  files. This is not a perfect solution (after all, there might be a
  bind-mount of a different procfs file over the target) but in order to
  exploit that you would need to be able to tweak a config.json pretty
  specifically (which thankfully Docker doesn't allow).

  Specifically this stops AppArmor from not labeling a process silently
  due to /proc/self/attr/... being incorrectly set, and stops any
  accidental fd leaks because /proc/self/fd/... is not real.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
stefanberger pushed a commit to stefanberger/runc that referenced this pull request Dec 31, 2019
Signed-off-by: blacktop <blacktop@users.noreply.github.com>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Jan 16, 2020
full diff: opencontainers/runc@3e425f8...v1.0.0-rc9

- opencontainers/runc#1951 Add SCMP_ACT_LOG as a valid Seccomp action
- opencontainers/runc#2130 *: verify operations on /proc/... are on procfs
  This is an additional mitigation for CVE-2019-16884. The primary problem
  is that Docker can be coerced into bind-mounting a file system on top of
  /proc (resulting in label-related writes to /proc no longer happening).

  While we are working on mitigations against permitting the mounts, this
  helps avoid our code from being tricked into writing to non-procfs
  files. This is not a perfect solution (after all, there might be a
  bind-mount of a different procfs file over the target) but in order to
  exploit that you would need to be able to tweak a config.json pretty
  specifically (which thankfully Docker doesn't allow).

  Specifically this stops AppArmor from not labeling a process silently
  due to /proc/self/attr/... being incorrectly set, and stops any
  accidental fd leaks because /proc/self/fd/... is not real.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: efcd84e47c6bc3f5e52eb2cce518f55501d60ce7
Component: engine
adrianreber pushed a commit to adrianreber/runc that referenced this pull request Feb 10, 2020
Signed-off-by: blacktop <blacktop@users.noreply.github.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.

6 participants