Seccomp: initial small additions to the rules #2040

martinetd · 2024-06-05T04:01:26Z

This is the first part of the seccomp policy work described in #1988: the small things that probably won't be too controversial -- sorry for the delay.

I have finished the rest all the way to unshare/mount blocked as per docker policies in my seccomp-hardened branch, this one has a few more restrictions that are in place on docker so probably would not cause much trouble but still are more restrictive, so left out for now (that branch just modified the same filter, but it'll be a separate file when ready to open as a PR; I was just using it to test ; I've noticed a few more syscalls I'll want to look at closer and maybe block so there's a bit of work left first)

Each commit is small and self-contained, so please refer to commit messages for individual changes -- most of the new blockages are obsolete or never existed from what I can tell; the exceptions that might be worth discussing are:

reboot, the syscall itself should do the capability check anyway
pivot_root, it's not allowed even with CAP_SYS_ADMIN for docker so I don't think there are any users of it in containers, if someone thinks it might be a problem I'll move that commit for the restricted profile later PR
syscall, having this really makes the seccomp profile moot so I think we need to disable it; as far as I know there are no valid users of this syscall I've only seen it used in exploits (to be extra clear, it doesn't block the syscall assembly instruction (int 0x80) so syscall(__NR_foo, ..) will still work, it blocks syscall(__NR_syscall, __NR_foo, ..))

The rest should be strictly more permissive than currently unless I missed something.

These have been replaced by the rt_sigaction family, and have not been compiled in on most kernels since linux v3.9 (2013) Signed-off-by: Dominique Martinet <[email protected]>

timerfd as a syscall does not seem to have ever existed, remove it from allowed syscalls list. Signed-off-by: Dominique Martinet <[email protected]>

The following syscalls have been added in recent kernels and considered for this list: - cachestat, prints information about cache misses; it is less accurate than userfaultfd so probably safe but deny it until a clear need shows up - io_pgetevents_time64: io_pgetevents is already blocked, so block this variant as well. Note these are pretty close to io_getenvents, so we should probably block that as well, but since it is currently allowed keep that where it is. - map_shadow_stack: this allows creating a new shadow stack, required for user-space threading if shadow stack verification is enabled (prctl PR_SET_SHADOW_STACK_STATUS with PR_SHADOW_STACK_ENABLE); this might be required in the future but delay this decision until someone requests it - futex_* new interface is primarily intended for io_uring which we disallow, and does not have any known user yet so likewise block until someone requests it. - quotactl_fd: this is identical to quotactl, so only allow for SYS_ADMIN like quotactl. Signed-off-by: Dominique Martinet <[email protected]>

swapcontext seems to be used for coroutines in some languages (at least ruby), enough to have been added to other major engines by an actual user. Link: moby/moby#43092 Link: systemd/systemd#9487 Link: containerd/containerd#6411 Signed-off-by: Dominique Martinet <[email protected]>

apparently harmless and used Link: systemd/systemd#25018 Link: containerd/containerd#6882 Link: moby/moby#43553 Signed-off-by: Dominique Martinet <[email protected]>

Luap99 · 2024-06-05T09:31:21Z

@giuseppe PTAL

Luap99 · 2024-06-05T09:32:44Z

pkg/seccomp/default_linux.go

@@ -293,7 +294,6 @@ func DefaultProfile() *Seccomp {
 				"pidfd_send_signal",
 				"pipe",
 				"pipe2",
-				"pivot_root",


isn't pivot_root used by container runtimes so this breaks the nested container case I guess?

yes, we need this otherwise we break nested containers

Ugh, sorry I should have tested this, I was somehow convinced it wasn't used. I've removed that commit.

giuseppe · 2024-06-05T10:26:02Z

pkg/seccomp/default_linux.go

+			Names: []string{
+				"reboot",
+			},
+			Action: ActAllow,
+			Args:   []*Arg{},
+			Includes: Filter{
+				Caps: []string{"CAP_SYS_BOOT"},
+			},
+		},


I don't think we need this additional check in the profile since it is already in place in the kernel

I think the point of these is to stack layers so if the kernel gets a check wrong we had this safety, but I just had a look at the reboot syscall definition and checking the capability is literally the first thing done so there isn't much room for mistake here;
ok, removing this.

rhatdan · 2024-06-05T14:38:08Z

pkg/seccomp/default_linux.go

 				"io_pgetevents",
+				"io_pgetevents_time64",


Lets block io_getenvents,

If we block io_getevents too I think programs using libaio will stop working; this is just listing a syscall that was already blocked.
I agree the hardened profile can definitely block the io_{setup,destroy,cancel,getevents,submit} family of syscalls like we have io_uring blocked, but I'm not sure we should block them here.

Perhaps for podman 6.0 defaults?

rhatdan · 2024-06-05T14:43:03Z

Great work @martinetd Thanks for this effort
A couple of changes requested and then we can move forward.
Glad to see security increasing, even if it is minimal.

rhatdan · 2024-06-05T14:43:13Z

/approve

the readdir syscall hasn't existed forever (wasn't present in linux 2.6.12 initial import into git), remove it and don't even bother adding it to the list of EPERM syscalls Signed-off-by: Dominique Martinet <[email protected]>

This does not deny anything new (bpf is currently allowed for sys admin) Signed-off-by: Dominique Martinet <[email protected]>

This doesn't deny anything new (perf_event_open is currently allowed for SYS_ADMIN) Signed-off-by: Dominique Martinet <[email protected]>

syscall() emulates all other syscalls, so having this allowed makes no sense as far as seccomp filters go. This is a breaking change, but this probably will not break much. Signed-off-by: Dominique Martinet <[email protected]>

martinetd

Thanks for the reviews, I've replied/updated as pointed out.
I'll do some more tests (including podman in podman!) with this later today, but I think it should be good to go.

martinetd · 2024-06-06T01:17:09Z

pkg/seccomp/default_linux.go

@@ -293,7 +294,6 @@ func DefaultProfile() *Seccomp {
 				"pidfd_send_signal",
 				"pipe",
 				"pipe2",
-				"pivot_root",


Ugh, sorry I should have tested this, I was somehow convinced it wasn't used. I've removed that commit.

martinetd · 2024-06-06T01:53:20Z

pkg/seccomp/default_linux.go

+			Names: []string{
+				"reboot",
+			},
+			Action: ActAllow,
+			Args:   []*Arg{},
+			Includes: Filter{
+				Caps: []string{"CAP_SYS_BOOT"},
+			},
+		},


I think the point of these is to stack layers so if the kernel gets a check wrong we had this safety, but I just had a look at the reboot syscall definition and checking the capability is literally the first thing done so there isn't much room for mistake here;
ok, removing this.

martinetd · 2024-06-06T02:21:41Z

pkg/seccomp/default_linux.go

 				"io_pgetevents",
+				"io_pgetevents_time64",


If we block io_getevents too I think programs using libaio will stop working; this is just listing a syscall that was already blocked.
I agree the hardened profile can definitely block the io_{setup,destroy,cancel,getevents,submit} family of syscalls like we have io_uring blocked, but I'm not sure we should block them here.

Perhaps for podman 6.0 defaults?

rhatdan · 2024-06-06T10:37:01Z

LGTM

giuseppe

/lgtm

openshift-ci · 2024-06-07T09:04:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, martinetd, rhatdan

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,rhatdan]

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

martinetd added 5 commits June 5, 2024 12:42

seccomp: remove obsolete sigaction family syscalls

fd7d4a3

These have been replaced by the rt_sigaction family, and have not been compiled in on most kernels since linux v3.9 (2013) Signed-off-by: Dominique Martinet <[email protected]>

seccomp: remove 'timerfd'

aa9e144

timerfd as a syscall does not seem to have ever existed, remove it from allowed syscalls list. Signed-off-by: Dominique Martinet <[email protected]>

seccomp: riscv: add riscv_flush_icache

b12e6be

apparently harmless and used Link: systemd/systemd#25018 Link: containerd/containerd#6882 Link: moby/moby#43553 Signed-off-by: Dominique Martinet <[email protected]>

martinetd force-pushed the seccomp branch from 193f8b1 to 3604031 Compare June 5, 2024 04:47

Luap99 reviewed Jun 5, 2024

View reviewed changes

giuseppe reviewed Jun 5, 2024

View reviewed changes

rhatdan reviewed Jun 5, 2024

View reviewed changes

openshift-ci bot added the approved label Jun 5, 2024

martinetd added 4 commits June 6, 2024 11:23

seccomp: deny readdir()

fa18956

the readdir syscall hasn't existed forever (wasn't present in linux 2.6.12 initial import into git), remove it and don't even bother adding it to the list of EPERM syscalls Signed-off-by: Dominique Martinet <[email protected]>

seccomp: allow bpf() if CAP_BPF

6dd328a

This does not deny anything new (bpf is currently allowed for sys admin) Signed-off-by: Dominique Martinet <[email protected]>

seccomp: allow perf_event_open if CAP_PERFMON

daa81f1

This doesn't deny anything new (perf_event_open is currently allowed for SYS_ADMIN) Signed-off-by: Dominique Martinet <[email protected]>

seccomp: block syscall()

d3283f8

syscall() emulates all other syscalls, so having this allowed makes no sense as far as seccomp filters go. This is a breaking change, but this probably will not break much. Signed-off-by: Dominique Martinet <[email protected]>

martinetd force-pushed the seccomp branch from 3604031 to d3283f8 Compare June 6, 2024 02:25

martinetd commented Jun 6, 2024

View reviewed changes

giuseppe approved these changes Jun 7, 2024

View reviewed changes

openshift-ci bot assigned giuseppe Jun 7, 2024

openshift-ci bot added the lgtm label Jun 7, 2024

openshift-merge-bot bot merged commit 9331830 into containers:main Jun 7, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Seccomp: initial small additions to the rules #2040

Seccomp: initial small additions to the rules #2040

martinetd commented Jun 5, 2024

Luap99 commented Jun 5, 2024

Luap99 Jun 5, 2024

giuseppe Jun 5, 2024

martinetd Jun 6, 2024

giuseppe Jun 5, 2024

martinetd Jun 6, 2024

rhatdan Jun 5, 2024

martinetd Jun 6, 2024

rhatdan commented Jun 5, 2024

rhatdan commented Jun 5, 2024

martinetd left a comment

martinetd Jun 6, 2024

martinetd Jun 6, 2024

martinetd Jun 6, 2024

rhatdan commented Jun 6, 2024

giuseppe left a comment

openshift-ci bot commented Jun 7, 2024

Seccomp: initial small additions to the rules #2040

Seccomp: initial small additions to the rules #2040

Conversation

martinetd commented Jun 5, 2024

Luap99 commented Jun 5, 2024

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

rhatdan commented Jun 5, 2024

rhatdan commented Jun 5, 2024

martinetd left a 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

rhatdan commented Jun 6, 2024

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 7, 2024