seccomp: switch default to ENOSYS #573

giuseppe · 2021-05-24T11:48:34Z

add the currently blocked syscalls to a deny-list and switch the default to ENOSYS.

Signed-off-by: Giuseppe Scrivano [email protected]

rhatdan · 2021-05-24T12:26:36Z

Tests are not happy.

rhatdan · 2021-05-24T12:28:36Z

pkg/seccomp/seccomp.json

@@ -1,5 +1,6 @@
 {
 	"defaultAction": "SCMP_ACT_ERRNO",
+	"defaultErrnoRet": 38,


Should this be a magic number?

rhatdan · 2021-05-24T12:28:59Z

pkg/seccomp/seccomp.json

+				"vserver"
+			],
+			"action": "SCMP_ACT_ERRNO",
+			"errnoRet": 1


Should this be a magic number?

I think it is already too late for errnoRet as we already support it. Perhaps add a errnoRetString that can be used in place of errnoRet?

We could make defaultErrnoRet a string, but I don't see a way to convert from a name to an errno list in the std library, so we would need to do the conversion ourselves.

giuseppe · 2021-05-24T14:26:40Z

tests are happy now

rhatdan · 2021-05-24T14:40:02Z

LGTM
@containers/podman-maintainers PTAL

vrothberg

Can you open a test PR against Podman?

We've regressed a number of the times recently in c/common and caught them while vendoring the release into Podman.

giuseppe · 2021-05-25T08:21:05Z

Can you open a test PR against Podman?

good idea. Opened a PR here: containers/podman#10451

giuseppe · 2021-06-07T14:39:43Z

containers/podman#10451 failures don't seem related to the seccomp changes, but are related to the new runc release. Some tests need to be fixed

rhatdan · 2021-06-14T14:40:17Z

@giuseppe Still working on this?

fweimer-rh · 2021-06-14T14:50:26Z

pkg/seccomp/default_linux.go

+		{
+			Names: []string{
+				"bdflush",
+				"clone3",


I think this has to be treated the same way as clone. Ideally, we would allow this system call because it is required for supporting certain forms of security hardening. But we definitely will need ENOSYS for clone3.

fweimer-rh · 2021-06-14T14:51:04Z

pkg/seccomp/default_linux.go

+				"io_pgetevents",
+				"io_uring_enter",
+				"io_uring_register",
+				"io_uring_setup",


io_uring_* should be ENOSYS, too.

fweimer-rh · 2021-06-14T14:51:42Z

pkg/seccomp/default_linux.go

+				"pciconfig_write",
+				"pkey_alloc",
+				"pkey_free",
+				"pkey_mprotect",


The pkey_* functions should really be permitted because they enable security hardening. Otherwise, ENOSYS, not EPERM is needed for them.

fweimer-rh · 2021-06-14T14:52:14Z

pkg/seccomp/default_linux.go

+				"pkey_alloc",
+				"pkey_free",
+				"pkey_mprotect",
+				"rseq",


I do not see any harm in permitting rseq. In the future, it will be an important performance enhancement for some workloads.

fweimer-rh · 2021-06-14T14:52:45Z

pkg/seccomp/default_linux.go

+				"io_uring_setup",
+				"kexec_file_load",
+				"kexec_load",
+				"membarrier",


membarrier should default to ENOSYS if it can't be permitted.

fweimer-rh · 2021-06-14T14:53:53Z

pkg/seccomp/default_linux.go

+				"get_mempolicy",
+				"mbind",
+				"set_mempolicy",
+			},


Not sure why those are separate from move_pages and migrate_pages above?

these fails with EPERM only when the container is not granted CAP_SYS_NICE. Otherwise seccomp won't block them

Hmm. move_pages and migrate_pages seem to have similar constraints. I also believe that these system calls do not require CAP_SYS_NICE in all cases. See the discussion of the MPOL_MF_MOVE_ALL flag in the manual pages.

do you suggest enabling them in any case?

As far as I know, they are similar to madvise and not really privileged, so I think they can be enabled.

thanks. Added a patch to always allow them

Add support to specify the default errno return value. The OCI runtime specs already have support for it, and both crun (>= 0.19) and runc (>= 1.0-rc95) have support for it. Signed-off-by: Giuseppe Scrivano <[email protected]>

add the currently blocked syscalls to a deny-list and switch the default to ENOSYS. Signed-off-by: Giuseppe Scrivano <[email protected]>

openshift-ci · 2021-06-14T17:10:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

giuseppe · 2021-06-15T07:39:17Z

@fweimer-rh thanks for the suggestions. I agree with them and I'll implement them as a follow-up PR.

I am still struggling to get this in as some Podman CI tests are failing with the new ubuntu/fedora images needed for having runc-1.0-rc95. Progress here: containers/podman#10451

As soon as the tests pass, I will polish the Podman PR to update the images and we can merge this one

giuseppe · 2021-06-15T11:37:05Z

Podman tests are finally green. If we merge the PR, I will cleanup the Podman PR to vendor a release of containers/common and we can then target the new changes to the seccomp policy

rhatdan · 2021-06-15T13:06:05Z

/lgtm

giuseppe · 2021-06-16T10:20:59Z

@fweimer-rh addressed your comments in #627, except for mbind, set_mempolicy, get_mempolicy, but I can add another patch to address it

seccomp: tweak default profile (followup for #573)

@mstopa-splunk

Commit 491daf4 ("iv_fd_epoll: Add support for epoll_pwait2().") added support for epoll_pwait2(), with a fallback to epoll_wait() in case epoll_pwait2() is not supported by the kernel we are running on, which would be indicated by epoll_pwait2() returning -ENOSYS. Some reports (e.g. axoflow/axosyslog#85 , #33 (comment) ) suggest that some container technologies can cause -EPERM to be returned for epoll_pwait2(), independently of whether or not epoll_pwait2() is actually supported by the kernel we are running on, and this trips us up because we don't currently handle -EPERM gracefully, as we did not expect that we would have to do so. Making system calls return -EPERM to indicate that they were filtered out by a security policy framework seems somewhat dubious, especially when considering the amount of application and user confusion generated by system calls that are not documented as being able to fail with -EPERM now suddenly being able to fail with -EPERM, but there is not much we can do about this. I would be against adding EPERM-as-ENOSYS fallbacks for every current or future case where we handle ENOSYS, but: 1. it seems that this is the only case where this triggers; 2. upstream seems to agree that this EPERM behavior is a bug (see e.g. these links dug up by László Várady: containers/common#573 , containers/podman#10337 , opencontainers/runtime-spec#1087 ), so there will hopefully be no new cases of this in the future; 3. there's at least one container technology release (podman on CentOS 7) where this bug triggers and where the platform is sufficiently old to no longer be receiving updates, as pointed out by Balazs Scheidler, so this issue can't be fixed by users updating their container software. Under these circumstances, adding a workaround on our end seems reasonable, and this commit does so. This issue was originally reported by @mstopa-splunk on GitHub. Workaround originally by Balazs Scheidler. Signed-off-by: Lennert Buytenhek <[email protected]>

openshift-ci bot added the approved label May 24, 2021

giuseppe mentioned this pull request May 24, 2021

Permission denied when container process executes close_range syscall containers/podman#10337

Closed

giuseppe force-pushed the default-errno-ENOSYS branch from 5f489b3 to 40d6e52 Compare May 24, 2021 12:10

rhatdan reviewed May 24, 2021

View reviewed changes

pkg/seccomp/seccomp.json

@@ -1,5 +1,6 @@

{

"defaultAction": "SCMP_ACT_ERRNO",

"defaultErrnoRet": 38,

Copy link

Member

rhatdan May 24, 2021

Choose a reason for hiding this comment

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

Should this be a magic number?

rhatdan reviewed May 24, 2021

View reviewed changes

giuseppe force-pushed the default-errno-ENOSYS branch from 40d6e52 to dd4bab0 Compare May 24, 2021 13:16

vrothberg reviewed May 25, 2021

View reviewed changes

giuseppe force-pushed the default-errno-ENOSYS branch 12 times, most recently from 5ab4b05 to fcaaf65 Compare June 4, 2021 13:56

fweimer-rh reviewed Jun 14, 2021

View reviewed changes

giuseppe added 2 commits June 14, 2021 19:08

seccomp: add support for defaultErrnoRet

adee333

Add support to specify the default errno return value. The OCI runtime specs already have support for it, and both crun (>= 0.19) and runc (>= 1.0-rc95) have support for it. Signed-off-by: Giuseppe Scrivano <[email protected]>

seccomp: switch default to ENOSYS

81cd342

add the currently blocked syscalls to a deny-list and switch the default to ENOSYS. Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the default-errno-ENOSYS branch from fcaaf65 to 81cd342 Compare June 14, 2021 17:09

openshift-ci bot assigned rhatdan Jun 15, 2021

openshift-ci bot added the lgtm label Jun 15, 2021

openshift-merge-robot merged commit b115fab into containers:main Jun 15, 2021

This was referenced Jun 15, 2021

tests: update CI images containers/podman#10451

Merged

seccomp: tweak default profile (followup for #573) #627

Merged

cevich mentioned this pull request Jun 17, 2021

error adding seccomp filter rule for syscall bdflush: permission denied #631

Closed

openshift-merge-robot added a commit that referenced this pull request Jun 18, 2021

Merge pull request #627 from giuseppe/fix-seccomp-profile

5a0821b

seccomp: tweak default profile (followup for #573)

MrAnno mentioned this pull request May 9, 2024

iv_fd_poll: epoll_pwait2() returns EPERM on CentOS7 with unprivileged… buytenh/ivykis#33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

seccomp: switch default to ENOSYS #573

seccomp: switch default to ENOSYS #573

giuseppe commented May 24, 2021

rhatdan commented May 24, 2021

rhatdan May 24, 2021

rhatdan May 24, 2021

giuseppe May 24, 2021

giuseppe commented May 24, 2021

rhatdan commented May 24, 2021

vrothberg left a comment

giuseppe commented May 25, 2021

giuseppe commented Jun 7, 2021 •

edited

Loading

rhatdan commented Jun 14, 2021

fweimer-rh Jun 14, 2021

fweimer-rh Jun 14, 2021

fweimer-rh Jun 14, 2021

fweimer-rh Jun 14, 2021

fweimer-rh Jun 14, 2021

fweimer-rh Jun 14, 2021

giuseppe Jun 15, 2021

fweimer-rh Jun 15, 2021 •

edited

Loading

giuseppe Jun 16, 2021

fweimer-rh Jun 16, 2021

giuseppe Jun 16, 2021

openshift-ci bot commented Jun 14, 2021

giuseppe commented Jun 15, 2021

giuseppe commented Jun 15, 2021

rhatdan commented Jun 15, 2021

giuseppe commented Jun 16, 2021

seccomp: switch default to ENOSYS #573

seccomp: switch default to ENOSYS #573

Conversation

giuseppe commented May 24, 2021

rhatdan commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented May 24, 2021

rhatdan commented May 24, 2021

vrothberg left a comment

Choose a reason for hiding this comment

giuseppe commented May 25, 2021

giuseppe commented Jun 7, 2021 • edited Loading

rhatdan commented Jun 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fweimer-rh Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 14, 2021

giuseppe commented Jun 15, 2021

giuseppe commented Jun 15, 2021

rhatdan commented Jun 15, 2021

giuseppe commented Jun 16, 2021

giuseppe commented Jun 7, 2021 •

edited

Loading

fweimer-rh Jun 15, 2021 •

edited

Loading