seccomp: allow ptrace(2) for 4.8+ kernels by tonistiigi · Pull Request #38137 · moby/moby

tonistiigi · 2018-11-04T18:04:18Z

4.8+ kernels have fixed the ptrace security issues
so we can allow ptrace(2) on the default seccomp
profile if we do the kernel version check.

torvalds/linux@93e35ef

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

tonistiigi · 2018-11-04T20:05:25Z

cc @justincormack @jessfraz

codecov · 2018-11-04T21:06:15Z

Codecov Report

Merging #38137 into master will decrease coverage by 0.43%.
The diff coverage is 30.76%.

@@            Coverage Diff             @@
##           master   #38137      +/-   ##
==========================================
- Coverage   36.55%   36.12%   -0.44%     
==========================================
  Files         610      610              
  Lines       45275    45242      -33     
==========================================
- Hits        16551    16342     -209     
- Misses      26394    26656     +262     
+ Partials     2330     2244      -86

4.8+ kernels have fixed the ptrace security issues so we can allow ptrace(2) on the default seccomp profile if we do the kernel version check. torvalds/linux@93e35ef Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

thaJeztah · 2018-11-05T14:30:33Z

+			"args": null,
+			"comment": "",
+			"includes": {
+				"minKernel": "4.8.0"


I know generally we tried to avoid kernel version checks; Will this be an issue if distros backport this fix? (i.e., would there be another approach so that the default could manually be overridden at compile time?)

If a distro backports they will just not get the ptrace support even if it may be secure for their kernel as well. User can still fix it in that case by providing a custom profile or upgrading the kernel. 4.8 is documented the switch point on the man page. Even if I would dynamically detect the behavior of the current system there is no way to mark that condition in the profile.

Yeah, not sure if there's a cleaner solution. Was just thinking if (e.g.) Red Hat decides to backport, and Docker wants to ship Docker EE packages for RHEL with a correct profile

You can ship a different default profile anyway, so I don't think this is an issue.

abudnik

In theory, we might want to specify a range or list of ranges of kernel versions, e.g. ["4.4.0", null], [null, "4.4.0"], [["3.16.0", "4.4.0"], ...]. It would be great to consider adding ranges into the current Seccomp profile format, so that it will be easier to implement ranges in the future in a backward-compatible way.

abudnik · 2018-11-26T23:07:26Z

+				"ptrace"
+			],
+			"action": "SCMP_ACT_ALLOW",
+			"args": null,


Consider using empty array [] instead of null for consistency.

tonistiigi · 2018-11-26T23:32:15Z

I can change it to any syntax that maintainers like to have. The current one came from discussing different variants (including the range one) with @justincormack

cpuguy83 · 2018-11-26T23:44:25Z

I'm fine with this.

abudnik · 2018-11-26T23:49:53Z

Ranges format makes sense if some severe bug appeared on some kernel version A and fixed later on some version B.

If adding ranges format in the future is not hard and is backward-compatible, then I have nothing against the current format : )

tonistiigi · 2018-12-10T13:34:42Z

ping @justincormack @tiborvass

tiborvass

Approving this after talking with @ncopa. He's also warning about abuse of the kernel version field.

justincormack · 2018-12-10T17:36:54Z

 	newConfig.DefaultAction = specs.LinuxSeccompAction(config.DefaultAction)

+	var currentKernelVersion *kernel.VersionInfo
+	kernelGreaterEqualThan := func(v string) (bool, error) {


not sure I like defining local closures like this, can you just make it a helper function external to this? (yes, Go should allow local functions...)

justincormack · 2018-12-10T17:39:12Z

 		}
+		if call.Excludes.MinKernel != "" {
+			if ok, err := kernelGreaterEqualThan(call.Excludes.MinKernel); err != nil {
+				return nil, err


in the error case should we not just include the rule anyway? ie fail closed not error.

This should fail only in the case where profile had a version that couldn't be parsed. I think that case is similar to the other validation, eg. the call.Names check.

justincormack · 2018-12-10T17:39:56Z

+			Names:  []string{"ptrace"},
+			Action: types.ActAllow,
+			Includes: types.Filter{
+				MinKernel: "4.8.0",


Technically the kernel version is 4.8, there is not a 4.8.0 release, ir goes 4.8, 4.8.1...

justincormack · 2018-12-10T17:41:46Z

This should have a test.

justincormack · 2018-12-10T17:42:59Z

Some comments but LGTM generally. Note that some uses for ptrace will still be blocked by CAP_PTRACE and maybe Yama kernel module, but basic debug etc should work.

tonistiigi · 2018-12-10T17:50:06Z

@justincormack For the test, what kind are you expecting. A unit test of some component? Integration seems tricky as this behavior is very much host dependent.

tonistiigi · 2018-12-10T18:00:43Z

@justincormack And just to confirm, no ranges like #38137 (comment) , right? I'd be fine with adding it. It could be useful in the case where a syscall becomes broken in a future version, and a seccomp profile could block that without upgrading the daemon.

tonistiigi · 2019-02-05T01:03:18Z

ping @justincormack

abudnik · 2019-02-26T17:20:49Z

@tonistiigi Does the current default Docker Seccomp profile allows ptrace syscall on kernel 4.8+ even thought a container user does not have CAP_SYS_PTRACE capability?

		{
			"names": [
				"ptrace"
			],
			"action": "SCMP_ACT_ALLOW",
			"args": null,
			"comment": "",
			"includes": {
				"minKernel": "4.8"
			},
			"excludes": {}
		},
...
		{
			"names": [
				"kcmp",
				"process_vm_readv",
				"process_vm_writev",
				"ptrace"
			],
			"action": "SCMP_ACT_ALLOW",
			"args": [],
			"comment": "",
			"includes": {
				"caps": [
					"CAP_SYS_PTRACE"
				]
			},
			"excludes": {}
		},

tonistiigi · 2019-02-26T18:25:37Z

@abudnik This PR is not in any release version of Docker yet. Next version 19.03 should allow it.

abudnik · 2019-02-26T19:11:41Z

@tonistiigi Previously, a default profile allowed ptrace syscall only when CAP_SYS_PTRACE capability was enabled for a process. After this change, it seems that ptrace is allowed for a non-privileged containers on Linux 4.8+. Can this lead to a security issue?

tonistiigi · 2019-02-26T19:37:27Z

@abudnik No it shouldn't. ptrace was blocked in seccomp because kernels before 4.8 allowed to use it for insecure things. But these have been fixed now (see linked commit) so if we know we are on the updated kernel we can allow it. ptrace is still protected by the regular kernel checks for specific arguments(eg. it will not give you same level of controls as CAP_SYS_PTRACE). If you think we might have missed something, let us know.

AkihiroSuda · 2019-03-19T02:57:02Z

Wondering this should be disabled when --pid=host
WDYT?

tonistiigi · 2019-03-19T03:01:15Z

I guess pid=host is already considered insecure as far as visibility into host processes is concerned. (cc @justincormack )

edit: and actually I'm not sure if this exposes anything very bad. You still don't have CAP_PTRACE .

GordonTheTurtle added the status/0-triage label Nov 4, 2018

tonistiigi force-pushed the seccomp-ptrace branch from 5a1f5e0 to fb7b390 Compare November 4, 2018 21:06

seccomp: allow ptrace for 4.8+ kernels

1124543

4.8+ kernels have fixed the ptrace security issues so we can allow ptrace(2) on the default seccomp profile if we do the kernel version check. torvalds/linux@93e35ef Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the seccomp-ptrace branch from fb7b390 to 1124543 Compare November 4, 2018 21:06

thaJeztah added status/2-code-review impact/changelog area/security/seccomp and removed status/0-triage labels Nov 5, 2018

thaJeztah reviewed Nov 5, 2018

View reviewed changes

GordonTheTurtle assigned runcom Nov 19, 2018

abudnik reviewed Nov 26, 2018

View reviewed changes

tiborvass approved these changes Dec 10, 2018

View reviewed changes

justincormack reviewed Dec 10, 2018

View reviewed changes

derek bot added the rebuild/* label Dec 19, 2018

GordonTheTurtle removed the rebuild/* label Dec 19, 2018

derek bot added the rebuild/powerpc label Dec 19, 2018

GordonTheTurtle removed the rebuild/powerpc label Dec 19, 2018

justincormack approved these changes Feb 5, 2019

View reviewed changes

justincormack merged commit 1603af9 into moby:master Feb 5, 2019

tonistiigi mentioned this pull request Feb 5, 2019

seccomp: review update for ptrace support #38683

Merged

AkihiroSuda mentioned this pull request Jul 9, 2019

Update to Kernel 5.2 + Debian 9.9 + Docker 18.09 weber-software/diuid#5

Merged

thaJeztah mentioned this pull request Oct 10, 2020

Allow access to /proc/PID/exe by default #40713

Open

thaJeztah mentioned this pull request Mar 9, 2021

Enable process_vm_readv and process_vm_writev for kernel >= 4.8 #42083

Merged

Conversation

tonistiigi commented Nov 4, 2018

Uh oh!

tonistiigi commented Nov 4, 2018

Uh oh!

codecov bot commented Nov 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abudnik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Nov 26, 2018

Uh oh!

cpuguy83 commented Nov 26, 2018

Uh oh!

abudnik commented Nov 26, 2018

Uh oh!

tonistiigi commented Dec 10, 2018

Uh oh!

tiborvass left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justincormack commented Dec 10, 2018

Uh oh!

justincormack commented Dec 10, 2018

Uh oh!

tonistiigi commented Dec 10, 2018

Uh oh!

tonistiigi commented Dec 10, 2018

Uh oh!

tonistiigi commented Feb 5, 2019

Uh oh!

abudnik commented Feb 26, 2019

Uh oh!

tonistiigi commented Feb 26, 2019

Uh oh!

abudnik commented Feb 26, 2019

Uh oh!

tonistiigi commented Feb 26, 2019

Uh oh!

AkihiroSuda commented Mar 19, 2019

Uh oh!

tonistiigi commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov bot commented Nov 4, 2018 •

edited

Loading

tonistiigi commented Mar 19, 2019 •

edited

Loading