diff --git a/pkg/seccomp/seccomp.go b/pkg/seccomp/seccomp.go index 50a3574f6..45c8e5d43 100644 --- a/pkg/seccomp/seccomp.go +++ b/pkg/seccomp/seccomp.go @@ -38,13 +38,6 @@ const ( vsyscallPageIPMask = 1 << 31 ) -// NonNegativeFDCheck ensures an FD argument is a non-negative int. -func NonNegativeFDCheck() LessThanOrEqual { - // Negative int32 has the MSB (31st bit) set. So the raw uint FD value must - // be less than or equal to 0x7fffffff. - return LessThanOrEqual(0x7fffffff) -} - // Install generates BPF code based on the set of syscalls provided. It only // allows syscalls that conform to the specification. Syscalls that violate the // specification will trigger RET_KILL_PROCESS. If RET_KILL_PROCESS is not diff --git a/pkg/seccomp/seccomp_rules.go b/pkg/seccomp/seccomp_rules.go index 0c9d9bec1..97588d13c 100644 --- a/pkg/seccomp/seccomp_rules.go +++ b/pkg/seccomp/seccomp_rules.go @@ -267,6 +267,31 @@ func (le LessThanOrEqual) Render(program *syscallProgram, labelSet *labelSet, va program.JumpTo(labelSet.Matched()) } +// NonNegativeFD ensures that an FD argument is a non-negative int32. +type NonNegativeFD struct{} + +// String implements `ValueMatcher.String`. +func (NonNegativeFD) String() string { + return fmt.Sprintf("NonNegativeFD") +} + +// Repr implements `ValueMatcher.Repr`. +func (NonNegativeFD) Repr() string { + return NonNegativeFD{}.String() +} + +// Render implements `ValueMatcher.Render`. +func (NonNegativeFD) Render(program *syscallProgram, labelSet *labelSet, value matchedValue) { + // FDs are 32 bits, so the high 32 bits must all be zero. + value.LoadHigh32Bits() + program.IfNot(bpf.Jmp|bpf.Jeq|bpf.K, 0, labelSet.Mismatched()) + // Negative int32 has the MSB (31st bit) set. + // So the raw uint FD value must not have the 31st bit set. + value.LoadLow32Bits() + program.If(bpf.Jmp|bpf.Jset|bpf.K, 1<<31, labelSet.Mismatched()) + program.JumpTo(labelSet.Matched()) +} + // MaskedEqual specifies a value that matches the input after the input is // masked (bitwise &) against the given mask. It implements `ValueMatcher`. type maskedEqual struct { diff --git a/pkg/seccomp/seccomp_test.go b/pkg/seccomp/seccomp_test.go index d10c2a55f..ae18af546 100644 --- a/pkg/seccomp/seccomp_test.go +++ b/pkg/seccomp/seccomp_test.go @@ -901,6 +901,68 @@ func TestBasic(t *testing.T) { }, }, }, + { + name: "NonNegativeFD", + ruleSets: []RuleSet{ + { + Rules: MakeSyscallRules(map[uintptr]SyscallRule{ + 1: PerArg{ + NonNegativeFD{}, + }, + }), + Action: linux.SECCOMP_RET_ALLOW, + }, + }, + defaultAction: linux.SECCOMP_RET_TRAP, + badArchAction: linux.SECCOMP_RET_KILL_THREAD, + specs: []spec{ + { + desc: "zero allowed", + data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x0}}, + want: linux.SECCOMP_RET_ALLOW, + }, + { + desc: "one allowed", + data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x0}}, + want: linux.SECCOMP_RET_ALLOW, + }, + { + desc: "seven allowed", + data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x7}}, + want: linux.SECCOMP_RET_ALLOW, + }, + { + desc: "largest int32 allowed", + data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x7fffffff}}, + want: linux.SECCOMP_RET_ALLOW, + }, + { + desc: "negative 1 not allowed", + data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x80000000}}, + want: linux.SECCOMP_RET_TRAP, + }, + { + desc: "largest uint32 not allowed", + data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0xffffffff}}, + want: linux.SECCOMP_RET_TRAP, + }, + { + desc: "a positive int64 larger than max uint32 is not allowed", + data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x100000000}}, + want: linux.SECCOMP_RET_TRAP, + }, + { + desc: "largest int64 not allowed", + data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0x7fffffffffffffff}}, + want: linux.SECCOMP_RET_TRAP, + }, + { + desc: "largest uint64 not allowed", + data: linux.SeccompData{Nr: 1, Arch: LINUX_AUDIT_ARCH, Args: [6]uint64{0xffffffffffffffff}}, + want: linux.SECCOMP_RET_TRAP, + }, + }, + }, { name: "Instruction Pointer", ruleSets: []RuleSet{ diff --git a/pkg/sentry/devices/accel/seccomp_filters.go b/pkg/sentry/devices/accel/seccomp_filters.go index 489d92cbd..5ee6cdcbc 100644 --- a/pkg/sentry/devices/accel/seccomp_filters.go +++ b/pkg/sentry/devices/accel/seccomp_filters.go @@ -23,7 +23,6 @@ import ( // Filters returns seccomp-bpf filters for this package. func Filters() seccomp.SyscallRules { - nonNegativeFD := seccomp.NonNegativeFDCheck() return seccomp.MakeSyscallRules(map[uintptr]seccomp.SyscallRule{ unix.SYS_OPENAT: seccomp.PerArg{ // All paths that we openat() are absolute, so we pass a dirfd @@ -38,27 +37,27 @@ func Filters() seccomp.SyscallRules { unix.SYS_GETDENTS64: seccomp.MatchAll{}, unix.SYS_IOCTL: seccomp.Or{ seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(gasket.GASKET_IOCTL_RESET), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(gasket.GASKET_IOCTL_MAP_BUFFER), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(gasket.GASKET_IOCTL_UNMAP_BUFFER), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(gasket.GASKET_IOCTL_CLEAR_INTERRUPT_COUNTS), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(gasket.GASKET_IOCTL_REGISTER_INTERRUPT), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(gasket.GASKET_IOCTL_UNREGISTER_INTERRUPT), }, }, diff --git a/pkg/sentry/devices/nvproxy/seccomp_filters.go b/pkg/sentry/devices/nvproxy/seccomp_filters.go index 377bb1727..88eb40dd9 100644 --- a/pkg/sentry/devices/nvproxy/seccomp_filters.go +++ b/pkg/sentry/devices/nvproxy/seccomp_filters.go @@ -23,7 +23,6 @@ import ( // Filters returns seccomp-bpf filters for this package. func Filters() seccomp.SyscallRules { - nonNegativeFD := seccomp.NonNegativeFDCheck() notIocSizeMask := ^(((uintptr(1) << linux.IOC_SIZEBITS) - 1) << linux.IOC_SIZESHIFT) // for ioctls taking arbitrary size return seccomp.MakeSyscallRules(map[uintptr]seccomp.SyscallRule{ unix.SYS_OPENAT: seccomp.PerArg{ @@ -38,153 +37,153 @@ func Filters() seccomp.SyscallRules { }, unix.SYS_IOCTL: seccomp.Or{ seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.MaskedEqual(notIocSizeMask, frontendIoctlCmd(nvgpu.NV_ESC_CARD_INFO, 0)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_CHECK_VERSION_STR, nvgpu.SizeofRMAPIVersion)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_REGISTER_FD, nvgpu.SizeofIoctlRegisterFD)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_ALLOC_OS_EVENT, nvgpu.SizeofIoctlAllocOSEvent)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_FREE_OS_EVENT, nvgpu.SizeofIoctlFreeOSEvent)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_SYS_PARAMS, nvgpu.SizeofIoctlSysParams)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_ALLOC_MEMORY, nvgpu.SizeofIoctlNVOS02ParametersWithFD)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_FREE, nvgpu.SizeofNVOS00Parameters)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_CONTROL, nvgpu.SizeofNVOS54Parameters)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_ALLOC, nvgpu.SizeofNVOS21Parameters)), }, // Note that we don't need to add one for NVOS21ParametersV535, because // SizeofNVOS21ParametersV535 == SizeofNVOS21Parameters. We test this. seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_ALLOC, nvgpu.SizeofNVOS64Parameters)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_ALLOC, nvgpu.SizeofNVOS64ParametersV535)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_DUP_OBJECT, nvgpu.SizeofNVOS55Parameters)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_SHARE, nvgpu.SizeofNVOS57Parameters)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_VID_HEAP_CONTROL, nvgpu.SizeofNVOS32Parameters)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_MAP_MEMORY, nvgpu.SizeofIoctlNVOS33ParametersWithFD)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_UNMAP_MEMORY, nvgpu.SizeofNVOS34Parameters)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(frontendIoctlCmd(nvgpu.NV_ESC_RM_UPDATE_DEVICE_MAPPING_INFO, nvgpu.SizeofNVOS56Parameters)), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_INITIALIZE), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_MM_INITIALIZE), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_DEINITIALIZE), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_CREATE_RANGE_GROUP), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_DESTROY_RANGE_GROUP), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_REGISTER_GPU_VASPACE), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_UNREGISTER_GPU_VASPACE), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_REGISTER_CHANNEL), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_UNREGISTER_CHANNEL), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_MAP_EXTERNAL_ALLOCATION), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_FREE), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_REGISTER_GPU), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_UNREGISTER_GPU), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_PAGEABLE_MEM_ACCESS), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_DISABLE_READ_DUPLICATION), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_MAP_DYNAMIC_PARALLELISM_REGION), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_ALLOC_SEMAPHORE_POOL), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_VALIDATE_VA_RANGE), }, seccomp.PerArg{ - nonNegativeFD, + seccomp.NonNegativeFD{}, seccomp.EqualTo(nvgpu.UVM_CREATE_EXTERNAL_RANGE), }, }, diff --git a/runsc/boot/filter/config.go b/runsc/boot/filter/config.go index 100c62171..07c674369 100644 --- a/runsc/boot/filter/config.go +++ b/runsc/boot/filter/config.go @@ -340,83 +340,82 @@ func hostFilesystemFilters() seccomp.SyscallRules { // negative FD values (like AT_FDCWD or invalid FD numbers). We try to be as // restrictive as possible because any restriction here improves security. We // don't know what set of arguments will trigger a future vulnerability. - validFDCheck := seccomp.NonNegativeFDCheck() return seccomp.MakeSyscallRules(map[uintptr]seccomp.SyscallRule{ unix.SYS_FCHOWNAT: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, seccomp.AnyValue{}, seccomp.AnyValue{}, seccomp.EqualTo(unix.AT_EMPTY_PATH | unix.AT_SYMLINK_NOFOLLOW), }, unix.SYS_FCHMODAT: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, seccomp.AnyValue{}, }, unix.SYS_UNLINKAT: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, seccomp.AnyValue{}, }, unix.SYS_GETDENTS64: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, seccomp.AnyValue{}, }, unix.SYS_OPENAT: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, seccomp.MaskedEqual(unix.O_NOFOLLOW, unix.O_NOFOLLOW), seccomp.AnyValue{}, }, unix.SYS_LINKAT: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, seccomp.EqualTo(0), }, unix.SYS_MKDIRAT: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, seccomp.AnyValue{}, }, unix.SYS_MKNODAT: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, seccomp.AnyValue{}, seccomp.AnyValue{}, }, unix.SYS_SYMLINKAT: seccomp.PerArg{ seccomp.AnyValue{}, - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, }, unix.SYS_FSTATFS: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, }, unix.SYS_READLINKAT: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, seccomp.AnyValue{}, seccomp.AnyValue{}, }, unix.SYS_UTIMENSAT: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, seccomp.AnyValue{}, seccomp.AnyValue{}, }, unix.SYS_RENAMEAT: seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, }, archFstatAtSysNo(): seccomp.PerArg{ - validFDCheck, + seccomp.NonNegativeFD{}, seccomp.AnyValue{}, seccomp.AnyValue{}, seccomp.AnyValue{},