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

Update libseccomp-golang dependency for filter generation bugfix #1424

Merged
merged 3 commits into from
Oct 15, 2017

Conversation

mheon
Copy link
Contributor

@mheon mheon commented Apr 26, 2017

A bug in Seccomp filter handling was recently identified in Moby related to handling of syscall arguments in Seccomp filters (moby/moby#32714). The bug was in the libseccomp-golang bindings, and has been fixed there. This PR updates the bindings to include this fix, and provides integration tests to catch regressions in this behavior.

The minimum supported version of libseccomp is bumped from v2.1.0 to v2.2.0 by associated changes to the bindings. Support for v2.1.0 was never very good (some features had to be gated off because of a library bug), though it is still the version provided by a few major distributions. If this change is contentious, I can see about backing out the changes that require v2.2.0.

@dqminh
Copy link
Contributor

dqminh commented May 11, 2017

$ make BUILDTAGS="${BUILDTAGS}"
go build -i -ldflags "-X main.gitCommit="5b839ecdcc3fd3a7ff70d417a63afe6900c18f24" -X main.version=1.0.0-rc3" -tags "seccomp apparmor selinux ambient" -o runc .
# github.com/opencontainers/runc/vendor/github.com/seccomp/libseccomp-golang
vendor/github.com/seccomp/libseccomp-golang/seccomp_internal.go:25:2: error: #error Minimum supported version of Libseccomp is v2.2.0
 #error Minimum supported version of Libseccomp is v2.2.0
  ^
make: *** [runc] Error 2
The command "make BUILDTAGS="${BUILDTAGS}"" exited with 2.

@mheon CI is failing with this error. Looks like it doesnt have up to date libseccomp-dev in the host.

@mheon
Copy link
Contributor Author

mheon commented May 11, 2017

I'll look into it - probably an issue with Travis configs not being identical to what's in the Dockerfile used for integration tests.

@mheon
Copy link
Contributor Author

mheon commented May 12, 2017

@dqminh Adjusted the Travis config to pull in a newer libseccomp, tests are green now

@dqminh
Copy link
Contributor

dqminh commented May 12, 2017

Thanks, the change looks fine !

I'm not sure about the bump to libseccomp 2.2 though. I double checked and older distros like debian jessie still on 2.1 ( but backports has 2.2 ), so seccomp will not work there ?

@mheon
Copy link
Contributor Author

mheon commented May 12, 2017

Jessie and the like will need to either use backports or build without seccomp, yes. Support was dropped upstream due to missing features and a major filter generation bug in v2.1.x that required that we disable filters with syscall argument filtering. If it's a blocker to drop support for v2.1.0 I can see about adding a branch retaining v2.1.0 support in the upstream bindings.

@crosbymichael
Copy link
Member

Can you rebase?

mheon added 3 commits October 10, 2017 15:49
Syscall argument handling was bugged in previous releases.
Per-argument match rules were handled with OR logic when they
should have used AND logic. The updated version of the bindings
resolves this issue.

As a side effect, the minimum supported version of Libseccomp has
been raised from v2.1.0 to v2.2.0.

Signed-off-by: Matthew Heon <[email protected]>
@mheon mheon force-pushed the update-libseccomp-golang branch from 80ca9c2 to 472fa3d Compare October 10, 2017 19:56
@mheon
Copy link
Contributor Author

mheon commented Oct 10, 2017

@crosbymichael Rebased

@crosbymichael
Copy link
Member

crosbymichael commented Oct 10, 2017

LGTM

Approved with PullApprove

@cyphar
Copy link
Member

cyphar commented Oct 15, 2017

LGTM.

Approved with PullApprove

@cyphar cyphar merged commit 472fa3d into opencontainers:master Oct 15, 2017
cyphar added a commit that referenced this pull request Oct 15, 2017
  Update Travis config to use trusty-backports libseccomp
  Add integration tests for multi-argument Seccomp filters
  Vendor updated libseccomp-golang for bugfix

LGTMs: @crosbymichael @cyphar
Closes #1424
@mrunalp
Copy link
Contributor

mrunalp commented Oct 16, 2017

We saw breakage in cri-o CI from this one. @mheon is looking into fix which we probably should get in for 1.0.

@mheon
Copy link
Contributor Author

mheon commented Oct 16, 2017

Specifically, it looks like some behavior (rules which check the same syscall argument number more than once) now fail because of the bugfix. This behavior is still possible, but will require small rewrites to filters (splitting the checks into multiple rules). To prevent this from breaking existing filters, I'm working on a patch to ignore rules using the old behavior and warn users to rewrite them.

@cyphar
Copy link
Member

cyphar commented Oct 17, 2017

@mheon Can't we rewrite such rules ourselves, and then just output a warning? I imagine that rules which test syscall numbers multiple times effectively map to the same rule copied multiple times -- no?

@mheon
Copy link
Contributor Author

mheon commented Oct 17, 2017

Good point, @cyphar - it's a better option than ignoring them, certainly. Shouldn't be hard to implement, either.

@thaJeztah
Copy link
Member

This PR brings in the fix for CVE-2017-18367 (through 03a5a74#diff-c1eca12d097b318b217f891966083c8e). The fix in libseccomp is in commit seccomp/libseccomp-golang@06e7a29

Full diff of libseccomp-golang changes: seccomp/libseccomp-golang@32f571b...84e90a9

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