Add seccomp feature #529

keloyang · 2015-04-14T01:22:15Z

add seccomp feature which is not use third-party
add multi arch surport
add test case
all code use golang

this pr is relate to #511 because I close it and find it can not be reopen

Signed-off-by: Yang Shukui [email protected]

hqhq · 2015-04-14T11:51:53Z

@pcmoore Can we discuss it in it's own thread?
commends from pcmoore:

Interesting, it handles the seccomp BPF filter generation completely in Golang. I'm obviously a little biased so I won't comment on which approach is better, but I will mention that just a quick glance at the code in PR #529 revealed a few issues/concerns:

It doesn't appear that x32 is handled correctly, resulting in a gap in protection on some systems

The syscall tables are wrong for ARM, e.g. OABI_SYSCALL_BASE (?)
The code doesn't abstract away architecture specifics, e.g. socketcall()
The generated BPF filter code looks like it might be twice as large as necessary resulting in unnecessary per-syscall performance overhead
The generated BPF filter code has a trap/signal return action, yet I don't see a signal handler defined (?) ... possible this may be misunderstanding how Golang handles signals

I suspect if I look closer there are more issues as well, not to mention the long term maintenence issues (who will keep the syscall tables updated?).

hqhq · 2015-04-14T12:04:01Z

Code review is a bit earlier, surely there would be lots of improvement for the code, I think we can have a design review first, have a comparison between these two approaches before we merge any of them.

I think maintaining syscall tables is acceptable, it's quite stable now, and we already maintain other kernel related info, like rlimit, capability etc, so that won't be the problem.

ping @crosbymichael

pcmoore · 2015-04-14T12:23:28Z

I'll leave the code review to you guys as you are much more knowledgeable than I am about Golang, but I will advise you to carefully examine the generated BPF code from both approaches as well as consider how you would support a single Docker filter configuration across multiple platforms (e.g. the x86 socketcall() problem).

keloyang · 2015-04-15T03:51:28Z

@pcmoore
thanks for your review.

The syscall tables are wrong for ARM, e.g. OABI_SYSCALL_BASE (?)

for arm, we don't use SYS_OABI_SYSCALL_BASE and SYS_SYSCALL_BASE, just start from SYS_RESTART_SYSCALL. and for multible arch, we can just relate to golang syscall,and relate to golang source code, e.g. syscall zsysnum_linux_arm.go/zsysnum_linux_386.go etc.

The code doesn't abstract away architecture specifics, e.g. socketcall()

I misunderstand, you means uniform interface for 386/arm/amd64 etc. or other ?

The generated BPF filter code looks like it might be twice as large as necessary resulting in unnecessary per-syscall performance overhead

It is a problem ? BPF is complex, but can be used simply, we can have a reference for kernel/example/seccomp

The generated BPF filter code has a trap/signal return action, yet I don't see a signal handler defined (?) ... possible this may be misunderstanding how Golang handles signals

SECCOMP_RET_KILL has a precedence for SECCOMP_RET_TRAP, we can relate to https://www.kernel.org/doc/Documentation/prctl/seccomp_filter.txt. SECCOMP_RET_KILL: Results in the task exiting immediately without executing the system call.
SECCOMP_RET_TRAP: Results in the kernel sending a SIGSYS signal to the triggeringtask without executing the system call. for SECCOMP_RET_KILL, I don't think a signal action is necessary. WDYT?

@hqhq +1 I think maintaining syscall tables is acceptable

pcmoore · 2015-04-16T23:11:52Z

A few follow-ups:

Regarding OABI_SYSCALL_BASE: I think there is a misunderstanding, let me try again - why do you define OABI_SYSCALL_BASE as a syscall in the ARM syscall table? That seems to be incorrect to me.
Regarding arch/ABI abstractions: How do you handle a configuration that involves socket() on x86? How do you handle socketcall() on ARM? Abstracting away the arch/ABI specifics is important to keeping a consistent configuration.
Yes, larger wasteful BPF programs are large and wasteful, especially when it is called on each syscall invocation.
I still don't understand why you are returning SECCOMP_RET_TRAP without a signal handler, that just doesn't make sense.

keloyang · 2015-04-17T02:05:22Z

Regarding OABI_SYSCALL_BASE: I think there is a misunderstanding, let me try again - why do you define OABI_SYSCALL_BASE as a syscall in the ARM syscall table? That seems to be incorrect to me.
OABI_SYSCALL_BASE may bring misunderstanding, I will remote it.
Regarding arch/ABI abstractions: How do you handle a configuration that involves socket() on x86? How do you handle socketcall() on ARM? Abstracting away the arch/ABI specifics is important to keeping a consistent configuration.
It's really a big problem. for i386, socket() is arg for socketcall(), adding the args surport can resolve these problem. Regrettably, it not surport now. I will add arg surporting later.
Yes, larger wasteful BPF programs are large and wasteful, especially when it is called on each syscall invocation.
I still don't understand why you are returning SECCOMP_RET_TRAP without a signal handler, that just doesn't make sense.
SECCOMP_RET_TRAP doesn't make sense, remote it.

thanks for your follow-ups!

crosbymichael · 2015-04-23T23:49:35Z

@keloyang do you have the code that generates these tables?

crosbymichael · 2015-04-23T23:52:07Z

Can you disable certain flags passed to syscalls with this implementation. Like preventing the CLONE_NEW* flags to clone()?

keloyang · 2015-04-30T12:44:54Z

@crosbymichael
sorry for my late reply.

I have change ScmpAdd(ctx *ScmpCtx, call sting ... to ScmpAdd(ctx *ScmpCtx, call int ... , we can use lxc seccomp config file now( refer to TestSeccompNotStat), and seccomp enable flags is added for easy control.

though the local test of libcontainner is past, but docker can't use it except adding this to default config(refer to newTemplateConfig):
Seccomps: configs.SeccompConf{
Enable: false,
SysCalls: make([]int, 0, 512),
},

crosbymichael · 2015-04-30T20:56:35Z

@keloyang what about arguments to the specific syscalls?

keloyang · 2015-05-02T03:45:55Z

@crosbymichael, the code don't surport for arguments, and I'm doing this.

LK4D4 · 2015-05-16T05:38:37Z

@keloyang Looks like rebase is needed :/

crosbymichael · 2015-05-20T22:59:56Z

@keloyang i guess one of the goals with seccomp support is to be able to prevent the CLONE_NEWUSER flag being passed to clone(). Is this hard to do?

pcmoore · 2015-05-21T02:14:35Z

Blocking clone(CLONE_NEWUSER) is possible with libseccomp and the SCMP_CMP_MASKED_EQ comparison operator.

keloyang · 2015-05-22T15:39:00Z

@crosbymichael @LK4D4
I have rebased and add args surport; CLONE_SECCOMP is also added which is like to CLOEN_IPC.

LK4D4 · 2015-05-22T15:42:31Z

@keloyang

seccomp/seccomp.go:331: undefined: syscallMap

Thanks for your work! It's amazing.

keloyang · 2015-05-22T18:03:29Z

maybe "chmod 755 hack/seccomp.sh" work!

LK4D4 · 2015-05-22T18:06:02Z

hack/seccomp.pl

@@ -0,0 +1,56 @@
+#!/usr/bin/perl


If you want I can help you rewrite this on go and use go generate for generating. So, developers won't be dependent on golang.

Also, of course you should commit generated code and not rely on generation tool. It is pretty hard to track bugs this way.

Cool, I'll prepare PR to your PR.

LK4D4 · 2015-05-22T19:42:19Z

@keloyang There is no seccomp_main in your PR.

LK4D4 · 2015-05-22T19:53:01Z

container_linux.go

@@ -169,6 +169,13 @@ func (c *linuxContainer) newInitProcess(p *Process, cmd *exec.Cmd, parentPipe, c
 			cmd.SysProcAttr.Credential = &syscall.Credential{}
 		}
 	}
+	if cloneFlags&uintptr(configs.CLONE_SECCOMP) != 0 {


it should be done by client. Silently ignore security issue isn't good idea.

@crosbymichael @LK4D4

Can you disable certain flags passed to syscalls with this implementation. Like preventing the CLONE_NEW* flags to clone()?
kernel don' surport for CLONE_SECCOMP, it is a pseudo flag which give a way to enable the seccomp feature. I misunderstand ?

LK4D4 · 2015-05-22T19:54:15Z

Hmm, it's weird to have sec.Sys. We should accept []string as syscall then. Or just rely on client with string -> syscall conversion.

LK4D4 · 2015-05-22T19:58:07Z

configs/namespaces_syscall.go

@@ -4,17 +4,22 @@ package configs

 import "syscall"

+var (
+	CLONE_SECCOMP = 0x10000 //diffrent from other flag, hard code


It's actually CLONE_NEWUSER. Why we need this at all?

CLONE_NEWUSER = 0x10000000
CLONE_NEWSECCOMP is a pseudo flag, which is diffrent other CLONE_xx flag, and it can not pass to clone.

add seccomp feature which is not use third-party add multi arch surport add test case all code use golang this pr is relate to #511 because I close it and find it can not be reopen Signed-off-by: Yang Shukui <[email protected]>

1. add args surport for seccomp 2. add CLONE_SECCOMP flag for preventing seccomp feature Signed-off-by: Yang Shukui <[email protected]>

Signed-off-by: Yang Shukui <[email protected]>

add the generated go file Signed-off-by: Yang Shukui <[email protected]>

add maskequal surport Signed-off-by: Yang Shukui <[email protected]>

crosbymichael · 2015-06-09T17:15:09Z

#613 was merged with your changes

thanks again for all your help

GordonTheTurtle added the status/0-needs-triage label Apr 14, 2015

hqhq mentioned this pull request Apr 14, 2015

Introduce support for syscall filtering in containers #237 #384

Closed

GordonTheTurtle added status/0-needs-triage dco/yes dco/no and removed dco/yes labels Apr 14, 2015

keloyang closed this Apr 15, 2015

keloyang reopened this Apr 15, 2015

GordonTheTurtle added dco/no dco/yes and removed dco/yes dco/no labels Apr 16, 2015

LK4D4 reviewed May 22, 2015
View reviewed changes

GordonTheTurtle added dco/no and removed dco/no labels May 25, 2015

yangshukui and others added 5 commits June 4, 2015 04:01

Add seccomp feature

b13d146

add seccomp feature which is not use third-party add multi arch surport add test case all code use golang this pr is relate to #511 because I close it and find it can not be reopen Signed-off-by: Yang Shukui <[email protected]>

seccomp surport filter args

656e4a0

1. add args surport for seccomp 2. add CLONE_SECCOMP flag for preventing seccomp feature Signed-off-by: Yang Shukui <[email protected]>

add the generated go file

0ad5156

Signed-off-by: Yang Shukui <[email protected]>

delete seccomp.Sys

7995bcc

add the generated go file Signed-off-by: Yang Shukui <[email protected]>

Add maskequal surport

ed4ddbf

add maskequal surport Signed-off-by: Yang Shukui <[email protected]>

crosbymichael mentioned this pull request Jun 4, 2015

Golang seccomp package #613

Merged

crosbymichael closed this Jun 9, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add seccomp feature #529

Add seccomp feature #529

keloyang commented Apr 14, 2015

hqhq commented Apr 14, 2015

hqhq commented Apr 14, 2015

pcmoore commented Apr 14, 2015

keloyang commented Apr 15, 2015

pcmoore commented Apr 16, 2015

keloyang commented Apr 17, 2015

crosbymichael commented Apr 23, 2015

crosbymichael commented Apr 23, 2015

keloyang commented Apr 30, 2015

crosbymichael commented Apr 30, 2015

keloyang commented May 2, 2015

LK4D4 commented May 16, 2015

crosbymichael commented May 20, 2015

pcmoore commented May 21, 2015

keloyang commented May 22, 2015

LK4D4 commented May 22, 2015

keloyang commented May 22, 2015

LK4D4 May 22, 2015

LK4D4 May 22, 2015

keloyang May 22, 2015

LK4D4 May 22, 2015

LK4D4 commented May 22, 2015

LK4D4 May 22, 2015

keloyang May 25, 2015

LK4D4 commented May 22, 2015

LK4D4 May 22, 2015

keloyang May 25, 2015

crosbymichael commented Jun 9, 2015

Add seccomp feature #529

Add seccomp feature #529

Conversation

keloyang commented Apr 14, 2015

hqhq commented Apr 14, 2015

hqhq commented Apr 14, 2015

pcmoore commented Apr 14, 2015

keloyang commented Apr 15, 2015

pcmoore commented Apr 16, 2015

keloyang commented Apr 17, 2015

crosbymichael commented Apr 23, 2015

crosbymichael commented Apr 23, 2015

keloyang commented Apr 30, 2015

crosbymichael commented Apr 30, 2015

keloyang commented May 2, 2015

LK4D4 commented May 16, 2015

crosbymichael commented May 20, 2015

pcmoore commented May 21, 2015

keloyang commented May 22, 2015

LK4D4 commented May 22, 2015

keloyang commented May 22, 2015

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

LK4D4 commented May 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LK4D4 commented May 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crosbymichael commented Jun 9, 2015