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

seccomp: switch default to ENOSYS #573

Merged
merged 2 commits into from
Jun 15, 2021
Merged

seccomp: switch default to ENOSYS #573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
adee333
seccomp: add support for defaultErrnoRet
giuseppe May 24, 2021
81cd342
seccomp: switch default to ENOSYS
giuseppe May 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/seccomp/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ func specToSeccomp(spec *specs.LinuxSeccomp) (*Seccomp, error) {
return nil, errors.Wrap(err, "convert default action")
}
res.DefaultAction = newDefaultAction
res.DefaultErrnoRet = spec.DefaultErrnoRet

// Loop through all syscall blocks and convert them to the internal format
for _, call := range spec.Syscalls {
Expand Down
184 changes: 181 additions & 3 deletions pkg/seccomp/default_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,54 @@ func arches() []Architecture {
// DefaultProfile defines the allowlist for the default seccomp profile.
func DefaultProfile() *Seccomp {
einval := uint(unix.EINVAL)
enosys := uint(unix.ENOSYS)
eperm := uint(unix.EPERM)

syscalls := []*Syscall{
{
Names: []string{
"bdflush",
"clone3",
Copy link

Choose a reason for hiding this comment

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

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.

"io_pgetevents",
"io_uring_enter",
"io_uring_register",
"io_uring_setup",
Copy link

Choose a reason for hiding this comment

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

io_uring_* should be ENOSYS, too.

"kexec_file_load",
"kexec_load",
"membarrier",
Copy link

Choose a reason for hiding this comment

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

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

"migrate_pages",
"move_pages",
"nfsservctl",
"nice",
"oldfstat",
"oldlstat",
"oldolduname",
"oldstat",
"olduname",
"pciconfig_iobase",
"pciconfig_read",
"pciconfig_write",
"pkey_alloc",
"pkey_free",
"pkey_mprotect",
Copy link

Choose a reason for hiding this comment

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

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

"rseq",
Copy link

Choose a reason for hiding this comment

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

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

"sgetmask",
"ssetmask",
"swapcontext",
"swapoff",
"swapon",
"sysfs",
"uselib",
"userfaultfd",
"ustat",
"vm86",
"vm86old",
"vmsplice",
},
Action: ActErrno,
ErrnoRet: &eperm,
Args: []*Arg{},
},
{
Names: []string{
"_llseek",
Expand Down Expand Up @@ -255,6 +301,7 @@ func DefaultProfile() *Seccomp {
"pwritev2",
"read",
"readahead",
"readdir",
"readlink",
"readlinkat",
"readv",
Expand Down Expand Up @@ -522,6 +569,17 @@ func DefaultProfile() *Seccomp {
Caps: []string{"CAP_DAC_READ_SEARCH"},
},
},
{
Names: []string{
"open_by_handle_at",
},
Action: ActErrno,
ErrnoRet: &eperm,
Args: []*Arg{},
Excludes: Filter{
Caps: []string{"CAP_DAC_READ_SEARCH"},
},
},
{
Names: []string{
"bpf",
Expand All @@ -539,6 +597,24 @@ func DefaultProfile() *Seccomp {
Caps: []string{"CAP_SYS_ADMIN"},
},
},
{
Names: []string{
"bpf",
"fanotify_init",
"lookup_dcookie",
"perf_event_open",
"quotactl",
"setdomainname",
"sethostname",
"setns",
},
Action: ActErrno,
ErrnoRet: &eperm,
Args: []*Arg{},
Excludes: Filter{
Caps: []string{"CAP_SYS_ADMIN"},
},
},
{
Names: []string{
"chroot",
Expand All @@ -549,6 +625,17 @@ func DefaultProfile() *Seccomp {
Caps: []string{"CAP_SYS_CHROOT"},
},
},
{
Names: []string{
"chroot",
},
Action: ActErrno,
ErrnoRet: &eperm,
Args: []*Arg{},
Excludes: Filter{
Caps: []string{"CAP_SYS_CHROOT"},
},
},
{
Names: []string{
"delete_module",
Expand All @@ -562,6 +649,20 @@ func DefaultProfile() *Seccomp {
Caps: []string{"CAP_SYS_MODULE"},
},
},
{
Names: []string{
"delete_module",
"init_module",
"finit_module",
"query_module",
},
Action: ActErrno,
ErrnoRet: &eperm,
Args: []*Arg{},
Excludes: Filter{
Caps: []string{"CAP_SYS_MODULE"},
},
},
{
Names: []string{
"get_mempolicy",
Expand All @@ -574,6 +675,19 @@ func DefaultProfile() *Seccomp {
Caps: []string{"CAP_SYS_NICE"},
},
},
{
Names: []string{
"get_mempolicy",
"mbind",
"set_mempolicy",
},
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

@fweimer-rh fweimer-rh Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

do you suggest enabling them in any case?

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks. Added a patch to always allow them

Action: ActErrno,
ErrnoRet: &eperm,
Args: []*Arg{},
Excludes: Filter{
Caps: []string{"CAP_SYS_NICE"},
},
},
{
Names: []string{
"acct",
Expand All @@ -584,6 +698,17 @@ func DefaultProfile() *Seccomp {
Caps: []string{"CAP_SYS_PACCT"},
},
},
{
Names: []string{
"acct",
},
Action: ActErrno,
ErrnoRet: &eperm,
Args: []*Arg{},
Excludes: Filter{
Caps: []string{"CAP_SYS_PACCT"},
},
},
{
Names: []string{
"kcmp",
Expand All @@ -598,6 +723,21 @@ func DefaultProfile() *Seccomp {
Caps: []string{"CAP_SYS_PTRACE"},
},
},
{
Names: []string{
"kcmp",
"process_madvise",
"process_vm_readv",
"process_vm_writev",
"ptrace",
},
Action: ActErrno,
ErrnoRet: &eperm,
Args: []*Arg{},
Excludes: Filter{
Caps: []string{"CAP_SYS_PTRACE"},
},
},
{
Names: []string{
"iopl",
Expand All @@ -609,6 +749,18 @@ func DefaultProfile() *Seccomp {
Caps: []string{"CAP_SYS_RAWIO"},
},
},
{
Names: []string{
"iopl",
"ioperm",
},
Action: ActErrno,
ErrnoRet: &eperm,
Args: []*Arg{},
Excludes: Filter{
Caps: []string{"CAP_SYS_RAWIO"},
},
},
{
Names: []string{
"settimeofday",
Expand All @@ -622,6 +774,20 @@ func DefaultProfile() *Seccomp {
Caps: []string{"CAP_SYS_TIME"},
},
},
{
Names: []string{
"settimeofday",
"stime",
"clock_settime",
"clock_settime64",
},
Action: ActErrno,
ErrnoRet: &eperm,
Args: []*Arg{},
Excludes: Filter{
Caps: []string{"CAP_SYS_TIME"},
},
},
{
Names: []string{
"vhangup",
Expand All @@ -632,6 +798,17 @@ func DefaultProfile() *Seccomp {
Caps: []string{"CAP_SYS_TTY_CONFIG"},
},
},
{
Names: []string{
"vhangup",
},
Action: ActErrno,
ErrnoRet: &eperm,
Args: []*Arg{},
Excludes: Filter{
Caps: []string{"CAP_SYS_TTY_CONFIG"},
},
},
{
Names: []string{
"socket",
Expand Down Expand Up @@ -714,8 +891,9 @@ func DefaultProfile() *Seccomp {
}

return &Seccomp{
DefaultAction: ActErrno,
ArchMap: arches(),
Syscalls: syscalls,
DefaultAction: ActErrno,
DefaultErrnoRet: &enosys,
ArchMap: arches(),
Syscalls: syscalls,
}
}
2 changes: 1 addition & 1 deletion pkg/seccomp/filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func BuildFilter(spec *specs.LinuxSeccomp) (*libseccomp.ScmpFilter, error) {
return nil, errors.Wrap(err, "convert spec to seccomp profile")
}

defaultAction, err := toAction(profile.DefaultAction, nil)
defaultAction, err := toAction(profile.DefaultAction, profile.DefaultErrnoRet)
if err != nil {
return nil, errors.Wrapf(err, "convert default action %s", profile.DefaultAction)
}
Expand Down
Loading