Skip to content

Move the syslog syscall to be gated by CAP_SYS_ADMIN or CAP_SYSLOG #37929

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
Merged
thaJeztah merged 1 commit into from
Oct 2, 2018
Merged

Move the syslog syscall to be gated by CAP_SYS_ADMIN or CAP_SYSLOG #37929

thaJeztah merged 1 commit into from
Oct 2, 2018

Conversation

@justincormack
Copy link
Contributor

@justincormack justincormack commented Sep 27, 2018

This call is what is used to implement dmesg to get kernel messages
about the host. This can leak substantial information about the host.
It is normally available to unprivileged users on the host, unless
the sysctl kernel.dmesg_restrict = 1 is set, but this is not set
by standard on the majority of distributions. Blocking this to restrict
leaks about the configuration seems correct.

Fix #37897

See also https://googleprojectzero.blogspot.com/2018/09/a-cache-invalidation-bug-in-linux.html

Signed-off-by: Justin Cormack [email protected]

beagle-dog-on-a-log-6475

@justincormack
Move the syslog syscall to be gated by CAP_SYS_ADMIN or CAP_SYSLOG
ccd22ff 
This call is what is used to implement `dmesg` to get kernel messages
about the host. This can leak substantial information about the host.
It is normally available to unprivileged users on the host, unless
the sysctl `kernel.dmesg_restrict = 1` is set, but this is not set
by standard on the majority of distributions. Blocking this to restrict
leaks about the configuration seems correct.

Fix moby#37897

See also https://googleprojectzero.blogspot.com/2018/09/a-cache-invalidation-bug-in-linux.html

Signed-off-by: Justin Cormack <[email protected]>
@justincormack justincormack mentioned this pull request Sep 27, 2018
Closed
@thaJeztah
Copy link
Member

thaJeztah commented Sep 28, 2018

Flaky test on windows? https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS1/22431/console

looks like it was marked as flaky a while back, but closed; #29641

22:55:53 FAIL: check_test.go:86: DockerSuite.OnTimeout
22:55:53 
22:55:53 check_test.go:93:
22:55:53     c.Fatalf("Failed to get daemon PID from %s\n", path)
22:55:53 ... Error: Failed to get daemon PID from docker.pid
22:55:53 
22:55:53 
22:55:53 --- FAIL: Test (4355.77s)
22:55:53 panic: DockerSuite.TestRestartAutoRemoveContainer test timed out after 10m0s [recovered]
22:55:53 	panic: DockerSuite.TestRestartAutoRemoveContainer test timed out after 10m0s
22:55:53 
22:55:53 goroutine 51 [running]:
22:55:53 testing.tRunner.func1(0xc000650300)
22:55:53 	d:/CI/CI-ccd22ffcc/go/src/testing/testing.go:792 +0x38e
22:55:53 panic(0xc34860, 0xc0006315d0)
22:55:53 	d:/CI/CI-ccd22ffcc/go/src/runtime/panic.go:513 +0x1c7
22:55:53 github.com/docker/docker/vendor/github.com/go-check/check.(*suiteRunner).runTest(0xc000670990, 0xc0006a9810, 0xc0002b4a50)
22:55:53 	c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/check.go:875 +0x29d
22:55:53 github.com/docker/docker/vendor/github.com/go-check/check.(*suiteRunner).run(0xc000670990, 0x133eaf0)
22:55:53 	c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/check.go:621 +0xfe
22:55:53 github.com/docker/docker/vendor/github.com/go-check/check.Run(0xcb52a0, 0x133eaf0, 0xc000096730, 0x1)
22:55:53 	c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/run.go:100 +0x54
22:55:53 github.com/docker/docker/vendor/github.com/go-check/check.RunAll(0xc000096730, 0x3)
22:55:53 	c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/run.go:92 +0x9d
22:55:53 github.com/docker/docker/vendor/github.com/go-check/check.TestingT(0xc000650300)
22:55:53 	c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/run.go:80 +0x384
22:55:53 github.com/docker/docker/integration-cli.Test(0xc000650300)
22:55:53 	c:/gopath/src/github.com/docker/docker/integration-cli/check_test.go:76 +0x8e
22:55:53 testing.tRunner(0xc000650300, 0xd04400)
22:55:53 	d:/CI/CI-ccd22ffcc/go/src/testing/testing.go:827 +0xc6
22:55:53 created by testing.(*T).Run
22:55:53 	d:/CI/CI-ccd22ffcc/go/src/testing/testing.go:878 +0x35a
22:55:53 exit status 2
22:55:53 FAIL	github.com/docker/docker/integration-cli	4355.851s
22:55:54 
22:55:54 
22:55:54 ERROR: Failed 'ERROR: Integration tests failed at 09/27/2018 22:55:54. Duration:01:12:41.6722393' at 09/27/2018 22:55:54
22:55:54 At C:\Users\jenkins\jenkins8451405105974784355.ps1:864 char:17
22:55:54 + ...             Throw "ERROR: Integration tests failed at $(Get-Date). Du ...
22:55:54 +                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@thaJeztah thaJeztah mentioned this pull request Sep 28, 2018
Open
@codecov
Copy link

codecov bot commented Sep 28, 2018

Codecov Report

Merging #37929 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #37929      +/-   ##
==========================================
+ Coverage   36.08%   36.09%   +<.01%     
==========================================
  Files         610      610              
  Lines       45126    45135       +9     
==========================================
+ Hits        16284    16291       +7     
- Misses      26602    26609       +7     
+ Partials     2240     2235       -5

thaJeztah
thaJeztah approved these changes Sep 28, 2018
View reviewed changes
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.

SGTM

@jessfraz
Copy link
Contributor

jessfraz commented Sep 30, 2018

i was about to open a PR from the issue but you already did 😍

@thaJeztah
Copy link
Member

thaJeztah commented Oct 1, 2018

@jessfraz that's a LGTM? 🙏 😇 😊

@jessfraz
Copy link
Contributor

jessfraz commented Oct 1, 2018

Yes it is :)

cpuguy83
cpuguy83 approved these changes Oct 1, 2018
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83
Copy link
Member

cpuguy83 commented Oct 1, 2018

I'm curious, shouldn't this already be blocked the kernel if the user doesn't have CAP_SYSLOG or CAP_SYS_ADMIN?

@cpuguy83
Copy link
Member

cpuguy83 commented Oct 1, 2018

Or is that only true if that kernel option is set to true and we want to make sure to block it here?

@justincormack
Copy link
Contributor Author

justincormack commented Oct 1, 2018

@cpuguy83 that is only true if the sysctl is set, which it is not by default in most distros.

@thaJeztah
Copy link
Member

thaJeztah commented Oct 2, 2018

Looks like we have enough LGTM's; let's merge 🎉

@thaJeztah thaJeztah merged commit 9267f34 into moby:master Oct 2, 2018
@justincormack justincormack deleted the syslog branch October 2, 2018 18:15
@thaJeztah thaJeztah mentioned this pull request Oct 2, 2018
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 24, 2020
Merged
@tomberek tomberek mentioned this pull request Aug 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

area/security/seccomp area/security impact/changelog status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@justincormack @thaJeztah @jessfraz @cpuguy83 @GordonTheTurtle