/snapd

interfaces: simplify snap-confine by just loading pre-generated bpf code #3431

Merged
merged 95 commits into from Jun 22, 2017
Commits
Jump to file or symbol
Failed to load files and symbols.
+49 −16
Split
Viewing a subset of changes. View all
Prev Next

start porting sh based snap-confine tests to the new seccomp-bpf world

  • Loading branch information...
commit 5dafe3f80c0afc1378be975f5a14d7c27c03d3b2 @mvo5 mvo5 committed Jun 13, 2017
View
13 cmd/snap-confine/tests/test_bad_seccomp_filter_args
@@ -25,16 +25,3 @@ for i in 'bar' '-1' '0 - -1 0' '--10' '0:10' '1-10' '0,1' '0x0' 'a1' '1a' '1-' '
PASS
done
-# > SC_ARGS_MAXLENGTH in seccomp.c
-for i in '- - - - - - 7' '1 2 3 4 5 6 7' ; do
- printf "Test bad seccomp arg filtering (too many args (> 6): %s)" "$i"
- cat "$TMP"/tmpl >"$TMP"/snap.name.app
- echo "mbind $i" >>"$TMP"/snap.name.app
-
- if $L snap.name.app /bin/true 2>/dev/null; then
- # true returned successfully, bad arg test failed
- FAIL
- else
- PASS
- fi
-done
View
6 cmd/snap-seccomp/main.go
@@ -409,8 +409,7 @@ func parseLine(line string, secFilter *seccomp.ScmpFilter) error {
value, err = readNumber(arg)
}
if err != nil {
- fmt.Fprintf(os.Stderr, "cannot parse token %q\n", arg)
- continue
+ return fmt.Errorf("cannot parse token %q (line %q)", arg, line)
}
var scmpCond seccomp.ScmpCondition
@@ -500,11 +499,12 @@ func showVersion() error {
func main() {
var err error
+ var content []byte
cmd := os.Args[1]
switch cmd {
case "compile":
- content, err := ioutil.ReadFile(os.Args[2])
+ content, err = ioutil.ReadFile(os.Args[2])
if err != nil {
break
}
View
31 cmd/snap-seccomp/main_test.go
@@ -191,7 +191,15 @@ func (s *snapSeccompSuite) TestCompile(c *C) {
// with arg1 and name resolving
{"ioctl - TIOCSTI", "ioctl;native;0,TIOCSTI", main.SeccompRetAllow},
@jdstrand

jdstrand Jun 14, 2017 •
Edited 1 time

Contributor

I also find it confusing that for "ioctl - TIOCSTI" we expect "ioctl;native;0,TIOCSTI" because the first argument should be unspecified, not 0. Ie, for the bpfs we are simulating, what is the difference between these:

"ioctl - TIOCSTI"
"ioctl 0 TIOCSTI"
@mvo5

mvo5 Jun 19, 2017

Collaborator

With ioctl - TIOCSTI the first argument is ignored in the generated bpf. The ioctl;native;0,TIOCSTI input is just one of the possible inputs that are legal. I could replace the 0 in there with a 1 or a 42 and it would still pass. I'm happy to add support for - in the input as well. The - would mean a random in then and we would have better tests because of this.

@mvo5

mvo5 Jun 19, 2017

Collaborator

To complete the question - ioctl 0 TIOCSTI as the program will only match inputs of the form ioctl;native;0,TIOCSTI not ioctl;native;1,TIOCST or any other value for the first arg.

@jdstrand

jdstrand Jun 19, 2017

Contributor

For clarity, ioctl - TIOCSTI means that we aren't doing any filtering on the first arg, only the second. It is syntax to simply signify that TIOCSTI is the second argument, nothing else. Contrast that to ioctl 0 TIOCSTI that states that the first argument must be 0 and the second TIOCSTI to allow the syscall. I think we need to somehow differentiate between these in the tests.
+ {"ioctl - TIOCSTI", "ioctl;native;0,99", main.SeccompRetKill},
{"ioctl - !TIOCSTI", "ioctl;native;0,TIOCSTI", main.SeccompRetKill},
+
+ // test_bad_seccomp_filter_args_clone
+ {"setns - CLONE_NEWNET", "setns;native;0,99", main.SeccompRetKill},
+ {"setns - CLONE_NEWNET", "setns;native;0,CLONE_NEWNET", main.SeccompRetAllow},
+ // test_bad_seccomp_filter_args_mknod
+ {"mknod - |S_IFIFO", "mknod;native;0,S_IFIFO", main.SeccompRetAllow},
+ {"mknod - |S_IFIFO", "mknod;native;0,99", main.SeccompRetKill},
} {
outPath := filepath.Join(c.MkDir(), "bpf")
err := main.Compile([]byte(t.seccompWhitelist), outPath)
@@ -214,3 +222,26 @@ func (s *snapSeccompSuite) TestCompile(c *C) {
}
}
+
+func (s *snapSeccompSuite) TestCompileBadInput(c *C) {
+ for _, t := range []struct {
+ inp string
+ errMsg string
+ }{
+ // test_bad_seccomp_filter_args_clone (various typos in input)
+ {"setns - CLONE_NEWNE", `cannot parse line: cannot parse token "CLONE_NEWNE" \(line "setns - CLONE_NEWNE"\)`},
+ {"setns - CLONE_NEWNETT", `cannot parse line: cannot parse token "CLONE_NEWNETT" \(line "setns - CLONE_NEWNETT"\)`},
+ {"setns - CL0NE_NEWNET", `cannot parse line: cannot parse token "CL0NE_NEWNET" \(line "setns - CL0NE_NEWNET"\)`},
+ // test_bad_seccomp_filter_args_mknod (various typos in input)
+ {"mknod - |S_IFIF", `cannot parse line: cannot parse token "S_IFIF" \(line "mknod - |S_IFIF"\)`},
+ {"mknod - |S_IFIFOO", `cannot parse line: cannot parse token "S_IFIFOO" \(line "mknod - |S_IFIFOO"\)`},
+ {"mknod - |S_!FIFO", `cannot parse line: cannot parse token "S_IFIFO" \(line "mknod - |S_!FIFO"\)`},
+ // test_bad_seccomp_filter_args
+ {"mbind - - - - - - 7", `cannot parse line: too many tokens \(6\) in line.*`},
+ {"mbind 1 2 3 4 5 6 7", `cannot parse line: too many tokens \(6\) in line.*`},
+ } {
+ outPath := filepath.Join(c.MkDir(), "bpf")
+ err := main.Compile([]byte(t.inp), outPath)
+ c.Check(err, ErrorMatches, t.errMsg, Commentf("%q errors in unexpected ways, got: %q expected %q", t.inp, err, t.errMsg))
+ }
+}
View
15 tests/main/snap-confine/task.yaml
@@ -0,0 +1,15 @@
+summary: Port of the snap-confine integration tests
@jdstrand

jdstrand Jun 14, 2017 •
Edited 1 time

Contributor

This task.yaml is not complete and the summary is not representative of what should be here. It should probably instead be:

summary: snap-confine integration tests
...
execute: |
    # test invalid syntax fails to compile
    ...
    # test valid syntax compiles
    ...
    # and that snap-confine can use the compiled valid syntax
    ...
@mvo5

mvo5 Jun 19, 2017

Collaborator

This test (as is) is not needed, there is a better set of tests in tests/main/snap-seccomp which already tests a bunch of the above cases, i.e. invalid syntax is not compiled, valid syntax works and snap-confine can work with the generated bpf.
+
+restore: |
+ rm -f tmpl
+
+execute: |
+ # test_bad_seccomp_filter_args_clone
+ for i in 'CLONE_NEWNE' 'CLONE_NETNETT' 'CL0NE_NEWNET' ; do
+ printf "Test bad seccomp arg filtering (setns - %s)" "$i"
+ echo "setns - $i" >> tmpl
+ if /usr/lib/snapd/snap-seccomp compile tmpl tmpl.bpf; then
+ echo "snap-seccomp should have failed to compile this"
+ exit 1
+ fi
+ done
@jdstrand

jdstrand Jun 13, 2017

Contributor

This is a pure syntax check and therefore should be covered by the unit tests. All pure syntax checks should be ported to cmd/snap-seccomp/main_test.go while ones testing if confinement is in place should be in spread tests. To make this easier, here are the syntax tests to be ported to main_export.go:

  • test_bad_seccomp_filter_*
  • test_restrictions_working_args_clone
  • test_restrictions_working_args_mknod
  • test_restrictions_working_args_prctl
  • test_restrictions_working_args_prio
  • test_restrictions_working_args_quotactl
  • test_restrictions_working_args_socket
  • test_restrictions_working_args_termios

and here are the ones to be ported to spread:

  • test_complain
  • test_complain_missed
  • test_unrestricted
  • test_unrestricted_missed
  • test_noprofile
  • test_whitelist (this may already be sufficiently covered by other spread tests)
  • test_restrictions (this may already be sufficiently covered by other spread tests)
  • test_restrictions_working (this may already be sufficiently covered by other spread tests)
  • test_restrictions_working_args

All that said, having one spread test with busted syntax is valuable to make sure things are functionally sane.