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.

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

Filter seccomp profile path from malicious .. and / #27036

Merged
merged 3 commits into from
Jun 18, 2016
Merged

Filter seccomp profile path from malicious .. and / #27036

merged 3 commits into from
Jun 18, 2016

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Jun 8, 2016 •
edited
Loading

Without this patch with localhost/<some-releative-path> as seccomp profile one can load any file on the host, e.g. localhost/../../../../dev/mem which is not healthy for the kubelet.

/cc @jfrazelle

Unit tests depend on #26710.

@sttts sttts added this to the v1.3 milestone Jun 8, 2016
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jun 8, 2016
@sttts
Copy link
Contributor Author

sttts commented Jun 8, 2016

Added unit tests for profile loading.

ncdc
ncdc reviewed Jun 8, 2016
View reviewed changes
pkg/kubelet/dockertools/manager_test.go
},
{
annotations: map[string]string{
"seccomp.security.alpha.kubernetes.io/pod": "localhost/../..//foo/../sub/subtest",
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be considered an invalid value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@ncdc
Copy link
Member

ncdc commented Jun 8, 2016

@pmorie

ncdc
ncdc reviewed Jun 8, 2016
View reviewed changes
pkg/kubelet/dockertools/manager_test.go

for _, test := range tests {
dm, fakeDocker := newTestDockerManagerWithVersion("1.10.1", "1.22")
_, filename, _, _ := goruntime.Caller(0)
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 is having issues in jenkins:

--- FAIL: TestSeccompLocalhostProfileIsLoaded (0.04s)
    manager_test.go:1915: dm.seccompProfileRoot=/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/dockertools/fixtures/seccomp
    <autogenerated>:31: 

    Error Trace:    manager_test.go:1952

    Error:      "[seccomp:unconfined]" does not contain "{"foo":"bar"}"

    Messages:   The compacted seccomp json profile should be loaded.

    manager_test.go:1915: dm.seccompProfileRoot=/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/dockertools/fixtures/seccomp
    <autogenerated>:31: 

    Error Trace:    manager_test.go:1952

    Error:      "[seccomp:unconfined]" does not contain "{"abc":"def"}"

    Messages:   The compacted seccomp json profile should be loaded.

    manager_test.go:1915: dm.seccompProfileRoot=/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/dockertools/fixtures/seccomp
    <autogenerated>:31: 

    Error Trace:    manager_test.go:1952

    Error:      "[seccomp:unconfined]" does not contain "{"abc":"def"}"

    Messages:   The compacted seccomp json profile should be loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 8, 2016
@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 9, 2016
@sttts sttts force-pushed the sttts-secure-seccomp-path branch from a03e515 to ded3dca Compare June 9, 2016 07:49
@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 9, 2016
ncdc
ncdc reviewed Jun 9, 2016
View reviewed changes
pkg/kubelet/dockertools/manager.go
name := strings.TrimPrefix(profile, "localhost/")
cleanName := strings.TrimPrefix(path.Clean("/"+name), "/")
if name != cleanName {
return nil, fmt.Errorf("invalid seccomp profile name: %s", name)
Copy link
Member

Choose a reason for hiding this comment

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

I talked with @pmorie on irc an we think ideally we'd have a helper function to do this, as it's needed in multiple places. There's already private validation code at

kubernetes/pkg/api/validation/validation.go

Line 742 in 5288a25

func validateSubPath(targetPath string, fldPath *field.Path) field.ErrorList {
to check for attempts to escape a base directory.

Given that we probably shouldn't change the validation code during the 1.3 code freeze, it's probably best to copy the logic from validateSubPath into here and put a TODO to create a helper and unify this code and the other places we need this logic after 1.3 is out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, the validation logic belongs into api/validation. I have added a commit implementing that. It's purely additional validation code, nothing old changed. I hope that's fine for 1.3. If not, I can split it and leave the upper code as it is, with an addition PR for post-1.3.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2016
@sttts sttts force-pushed the sttts-secure-seccomp-path branch from 054d320 to e771f76 Compare June 13, 2016 12:45
@sttts
Copy link
Contributor Author

sttts commented Jun 13, 2016

@ncdc ptal

@k8s-github-robot k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/design Categorizes issue or PR as related to design. labels Jun 13, 2016
@ncdc
Copy link
Member

ncdc commented Jun 13, 2016

I won't be able to look soon. Could you please find another reviewer?
Thanks.

On Monday, June 13, 2016, Dr. Stefan Schimanski notifications@github.com
wrote:

@ncdc https://github.com/ncdc ptal

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27036 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAABYkSShhb47dbLPcnQ38aLCSYfZcjrks5qLVFFgaJpZM4Iw2YN
.

@sttts sttts assigned pmorie and unassigned ncdc Jun 13, 2016
@sttts
Copy link
Contributor Author

sttts commented Jun 13, 2016

@pmorie ptal for a final review

pmorie
pmorie reviewed Jun 13, 2016
View reviewed changes
pkg/api/validation/validation_test.go
@@ -2711,6 +2767,62 @@ func TestValidatePod(t *testing.T) {
DNSPolicy: api.DNSClusterFirst,
},
},
"must be a valid pod seccomp profile": {
Copy link
Member

Choose a reason for hiding this comment

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

how about a test for the container annotation without a container name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@sttts sttts force-pushed the sttts-secure-seccomp-path branch from e771f76 to 87fa159 Compare June 14, 2016 05:59
@sttts
Copy link
Contributor Author

sttts commented Jun 14, 2016

@pmorie ptal

@sttts sttts force-pushed the sttts-secure-seccomp-path branch from 87fa159 to 3826d25 Compare June 14, 2016 12:58
@pmorie pmorie added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2016
@pmorie
Copy link
Member

pmorie commented Jun 14, 2016

Thanks for the patch @sttts!

@sttts
Copy link
Contributor Author

sttts commented Jun 17, 2016

@k8s-bot test this issue: #IGNORE

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jun 18, 2016

GCE e2e build/test passed for commit 3826d25.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 7ee4189 into kubernetes:master Jun 18, 2016
@sttts sttts mentioned this pull request Jun 20, 2016
Closed
@timstclair timstclair mentioned this pull request Jul 26, 2016
Closed
@pweil- pweil- mentioned this pull request Oct 25, 2016
Closed
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pmorie pmorie

Labels
area/kubelet area/security lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
7 participants
@sttts @ncdc @pmorie @k8s-github-robot @k8s-bot @googlebot @eparis