many: introduce seccomp denylist to block ioctl with TIOCLINUX to fix CVE-2023-1523

cmd/snap-seccomp-blacklist/snap-seccomp-blacklist.c

@@ -101,7 +101,7 @@ static int populate_filter(scmp_filter_ctx ctx, const uint32_t *arch_tags, size_
      * NOTE: not using scmp_rule_add_exact as that was not doing anything
      * at all (presumably due to having all the architectures defined). */
 
-    const struct scmp_arg_cmp no_tty_inject = {
+    struct scmp_arg_cmp no_tty_inject = {
         /* We learned that existing programs make legitimate requests with all
          * bits set in the more significant 32bit word of the 64 bit double
          * word. While this kernel behavior remains suspect and presumably
@@ -122,6 +122,10 @@ static int populate_filter(scmp_filter_ctx ctx, const uint32_t *arch_tags, size_
     };
     sc_err = seccomp_rule_add(ctx, SCMP_ACT_ERRNO(EPERM), sys_ioctl_nr, 1, no_tty_inject);
 
+    /* also block use of TIOCLINUX */
+    no_tty_inject.datum_b = TIOCLINUX;
+    sc_err = seccomp_rule_add(ctx, SCMP_ACT_ERRNO(EPERM), sys_ioctl_nr, 1, no_tty_inject);
+
     if (sc_err < 0) {
         showerr("cannot add rule preventing the use high bits in ioctl");
         return sc_err;

cmd/snap-seccomp/export_test.go

@@ -42,11 +42,19 @@ func MockArchDpkgKernelArchitecture(f func() string) (restore func()) {
 	}
 }
 
-func MockErrnoOnDenial(i int16) (retore func()) {
-	origErrnoOnDenial := errnoOnDenial
-	errnoOnDenial = i
+func MockErrnoOnImplicitDenial(i int16) (retore func()) {
+	origErrnoOnImplicitDenial := errnoOnImplicitDenial
+	errnoOnImplicitDenial = i
 	return func() {
-		errnoOnDenial = origErrnoOnDenial
+		errnoOnImplicitDenial = origErrnoOnImplicitDenial
+	}
+}
+
+func MockErrnoOnExplicitDenial(i int16) (retore func()) {
+	origErrnoOnExplicitDenial := errnoOnExplicitDenial
+	errnoOnExplicitDenial = i
+	return func() {
+		errnoOnExplicitDenial = origErrnoOnExplicitDenial
 	}
 }
 

cmd/snap-seccomp/main.go

@@ -34,6 +34,7 @@ package main
 //#include <stdio.h>
 //#include <stdlib.h>
 //#include <string.h>
+//#include <sys/ioctl.h>
 //#include <sys/prctl.h>
 //#include <sys/quota.h>
 //#include <sys/resource.h>
@@ -182,6 +183,11 @@ package main
 // #ifndef PTRACE_SETFPXREGS
 // #define PTRACE_SETFPXREGS 19
 // #endif
+//
+// /* Define TIOCLINUX if needed */
+// #ifndef TIOCLINUX
+// #define TIOCLINUX 0x541C
+// #endif
 import "C"
 
 import (
@@ -381,6 +387,9 @@ var seccompResolver = map[string]uint64{
 	// man 4 tty_ioctl
 	"TIOCSTI": syscall.TIOCSTI,
 
+	// man 2 ioctl_console
+	"TIOCLINUX": C.TIOCLINUX,
+
 	// man 2 quotactl (with what Linux supports)
 	"Q_SYNC":      C.Q_SYNC,
 	"Q_QUOTAON":   C.Q_QUOTAON,
@@ -542,6 +551,11 @@ func readNumber(token string, syscallName string) (uint64, error) {
 	return uint64(uint32(value)), nil
 }
 
+var (
+	errnoOnExplicitDenial int16 = C.EACCES
+	errnoOnImplicitDenial int16 = C.EPERM
+)
+
 func parseLine(line string, secFilter *seccomp.ScmpFilter) error {
 	// ignore comments and empty lines
 	if strings.HasPrefix(line, "#") || line == "" {
@@ -554,8 +568,17 @@ func parseLine(line string, secFilter *seccomp.ScmpFilter) error {
 		return fmt.Errorf("too many arguments specified for syscall '%s' in line %q", tokens[0], line)
 	}
 
+	// allow the listed syscall but also support explicit denials as well by
+	// prefixing the line with a ~
+	action := seccomp.ActAllow
+
 	// fish out syscall
 	syscallName := tokens[0]
+	if strings.HasPrefix(syscallName, "~") {
+		action = seccomp.ActErrno.SetReturnCode(errnoOnExplicitDenial)
+		syscallName = syscallName[1:]
+	}
+
 	secSyscall, err := seccomp.GetSyscallFromName(syscallName)
 	if err != nil {
 		// FIXME: use structed error in libseccomp-golang when
@@ -638,8 +661,8 @@ func parseLine(line string, secFilter *seccomp.ScmpFilter) error {
 
 	// Default to adding a precise match if possible. Otherwise
 	// let seccomp figure out the architecture specifics.
-	if err = secFilter.AddRuleConditionalExact(secSyscall, seccomp.ActAllow, conds); err != nil {
-		err = secFilter.AddRuleConditional(secSyscall, seccomp.ActAllow, conds)
+	if err = secFilter.AddRuleConditionalExact(secSyscall, action, conds); err != nil {
+		err = secFilter.AddRuleConditional(secSyscall, action, conds)
 	}
 
 	return err
@@ -698,8 +721,6 @@ func addSecondaryArches(secFilter *seccomp.ScmpFilter) error {
 	return nil
 }
 
-var errnoOnDenial int16 = C.EPERM
-
 func preprocess(content []byte) (unrestricted, complain bool) {
 	scanner := bufio.NewScanner(bytes.NewBuffer(content))
 	for scanner.Scan() {
@@ -771,7 +792,7 @@ func compile(content []byte, out string) error {
 			unrestricted = true
 		}
 	default:
-		secFilter, err = seccomp.NewFilter(seccomp.ActErrno.SetReturnCode(errnoOnDenial))
+		secFilter, err = seccomp.NewFilter(seccomp.ActErrno.SetReturnCode(errnoOnImplicitDenial))
 	}
 	if err != nil {
 		return fmt.Errorf("cannot create seccomp filter: %s", err)

cmd/snap-seccomp/main_test.go

@@ -151,8 +151,9 @@ int main(int argc, char** argv)
     // There might be architecture-specific requirements. see "man syscall"
     // for details.
     syscall_ret = syscall(l[0], l[1], l[2], l[3], l[4], l[5], l[6]);
-    // 911 is our mocked errno
-    if (syscall_ret < 0 && errno == 911) {
+    // 911 is our mocked errno for implicit denials via unlisted syscalls and
+    // 999 is explicit denial
+    if (syscall_ret < 0 && (errno == 911 || errno == 999)) {
         ret = 10;
     }
     syscall(SYS_exit, ret, 0, 0, 0, 0, 0);
@@ -161,7 +162,8 @@ int main(int argc, char** argv)
 `)
 
 func (s *snapSeccompSuite) SetUpSuite(c *C) {
-	main.MockErrnoOnDenial(911)
+	main.MockErrnoOnImplicitDenial(911)
+	main.MockErrnoOnExplicitDenial(999)
 
 	// build seccomp-load helper
 	s.seccompBpfLoader = filepath.Join(c.MkDir(), "seccomp_bpf_loader")
@@ -449,6 +451,12 @@ func (s *snapSeccompSuite) TestCompile(c *C) {
 		{"ioctl - TIOCSTI", "ioctl;native;-,TIOCSTI", Allow},
 		{"ioctl - TIOCSTI", "ioctl;native;-,99", Deny},
 		{"ioctl - !TIOCSTI", "ioctl;native;-,TIOCSTI", Deny},
+		{"~ioctl - TIOCSTI", "ioctl;native;-,TIOCSTI", Deny},
+		// also check we can deny multiple uses of ioctl but still allow
+		// others
+		{"~ioctl - TIOCSTI\n~ioctl - TIOCLINUX\nioctl - !TIOCSTI", "ioctl;native;-,TIOCSTI", Deny},
+		{"~ioctl - TIOCSTI\n~ioctl - TIOCLINUX\nioctl - !TIOCSTI", "ioctl;native;-,TIOCLINUX", Deny},
+		{"~ioctl - TIOCSTI\n~ioctl - TIOCLINUX\nioctl - !TIOCSTI", "ioctl;native;-,TIOCGWINSZ", Allow},
 
 		// test_bad_seccomp_filter_args_clone
 		{"setns - CLONE_NEWNET", "setns;native;-,99", Deny},

interfaces/seccomp/template.go

@@ -200,7 +200,18 @@ inotify_rm_watch
 # input (man tty_ioctl), so we disallow it to prevent snaps plugging interfaces
 # with 'capability sys_admin' from interfering with other snaps or the
 # unconfined user's terminal.
+# similarly, TIOCLINUX allows to fake input as well (man ioctl_console) so
+# disallow that too
 # TODO: this should be scaled back even more
+~ioctl - TIOCSTI
+~ioctl - TIOCLINUX
+# restrict argument otherwise will match all uses of ioctl() and allow the rules
+# that were disallowed above
+# TODO: Fix the need to keep TIOCLINUX here - the issue is a unrestricted
+#       allow for "ioctl" here makes libseccomp "optimize" the deny rules
+#       above away and the generated bpf becomes just "allow ioctl".
+#       We should fix this by creating a way to make "AND" rules, so
+#       this becomes "ioctl - !TIOCSTI&&!TIOCLINUX" and remove the "~" again.
 ioctl - !TIOCSTI
 
 io_cancel

tests/main/security-seccomp/task.yaml

@@ -7,6 +7,7 @@ details: |
     - use of a bare syscall (ie, no arguments) is allowed
     - use of a syscall with arg filtering is allowed with matching arguments
     - use of a syscall with arg filtering is denied with unmatching arguments
+    - explicit denial of a syscall with matching arguments is denied
     .
     We choose the setpriority syscall for these tests since it is available on
     all architectures and can be easily used to test all of the above. As part of
@@ -91,3 +92,11 @@ execute: |
     test-snapd-setpriority 10  | MATCH 'Successfully used setpriority\(PRIO_PROCESS, 0, 10\)'
     echo "and check that negative nice fails"
     test-snapd-setpriority -10 | MATCH 'Operation not permitted \(EPERM\)'
+
+    echo "Explicitly deny arg filtered setpriority rule"
+    sed 's/^\(setpriority.*\)/~\1/g' "$SRC".orig > "$SRC"
+    snapd.tool exec snap-seccomp compile "$SRC" "$BIN"
+    echo "and check that positive nice fails with explicit denial"
+    test-snapd-setpriority 10  | MATCH 'Insufficient privileges \(EACCES\)'
+    echo "and check that negative nice still fails with implicit denial"
+    test-snapd-setpriority -10 | MATCH 'Operation not permitted \(EPERM\)'

tests/main/snap-seccomp-blocks-tty-injection/task.yaml

@@ -0,0 +1,40 @@
+summary: Ensure that the snap-seccomp blocks tty command injection
+
+# ubuntu-core: excluded because there is no gcc there
+systems: [-ubuntu-core-*]
+
+prepare: |
+    echo "Install a helper snap (for seccomp confinement testing)"
+    "$TESTSTOOLS"/snaps-state install-local test-snapd-sh
+
+    echo "Compile and prepare the test programs"
+    # Because we use the snap data directory we don't need to clean it up
+    # manually as all snaps and their data are reset after each test.
+    # Build the test binary statically, as it will be running inside a base with
+    # potentially older glibc.
+    gcc -Wall -Wextra -Werror ./test-tioclinux.c -o /var/snap/test-snapd-sh/common/test-tioclinux -static
+    gcc -Wall -Wextra -Werror ./test-tiocsti.c   -o /var/snap/test-snapd-sh/common/test-tiocsti -static
+
+execute: |
+    # use /dev/tty1 as input so that we use a real virtual console which
+    # supports TIOCSTI / TIOCLINUX - but first make sure the snap can access it
+    # through AppArmor
+    if [ "$(snap debug confinement)" = strict ]; then
+        sed -i 's|^}$|  /dev/tty1  rw,\n}|' /var/lib/snapd/apparmor/profiles/snap.test-snapd-sh.sh
+        apparmor_parser -r /var/lib/snapd/apparmor/profiles/snap.test-snapd-sh.sh
+    fi
+
+    # For 64bit systems TIOC{STI,LINUX} gets a EPERM because of the
+    # "snap-seccomp-blacklist" that is *only* build for 64bit arches
+    # (because denying also needs to work when the higher bits are set
+    #  which the normal filter will not check, see also commit b923d58)
+    #
+    # On 32bit systems TIOC{STI,LINUX} is blocked by the default seccomp
+    # template.go which will default to EACCESS for explicit denied syscalls.
+    snap run test-snapd-sh.sh -c "\$SNAP_COMMON/test-tiocsti"   < /dev/tty1 2>&1 | MATCH 'normal TIOCSTI: -1 \((Operation not permitted|Permission denied)\)'
+    snap run test-snapd-sh.sh -c "\$SNAP_COMMON/test-tiocsti"   < /dev/tty1 2>&1 | MATCH 'high-bit-set TIOCSTI: -1 \((Operation not permitted|Permission denied)\)'
+    # TODO: this will not work because TIOCLINUX only works on "real" virtual
+    #       linux terminal which cannot be simulated via spread unless we
+    #       do something with nested qemu and simulated with maybe
+    #       "-chardev tty"
+    #snap run test-snapd-sh.sh -c "\$SNAP_COMMON/test-tioclinux" < /dev/tty1 2>&1 | MATCH 'ioctl\(0, TIOCLINUX, ...\) failed: Permission denied'

tests/main/snap-seccomp-blocks-tty-injection/test-tioclinux.c

@@ -0,0 +1,36 @@
+#include <err.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/ioctl.h>
+
+#include <linux/tiocl.h>
+#include <linux/vt.h>
+
+int main(void)
+{
+    int res;
+    printf("\33[H\33[2J");
+    printf("head -n1 /etc/shadow\n");
+    fflush(stdout);
+    struct {
+        char padding;
+        char subcode;
+        struct tiocl_selection sel;
+    } data = {
+        .subcode = TIOCL_SETSEL,
+        .sel = {
+            .xs = 1, .ys = 1,
+            .xe = 1, .ye = 1,
+            .sel_mode = TIOCL_SELLINE
+        }
+    };
+    res = ioctl(0, TIOCLINUX, &data.subcode);
+    if (res != 0)
+      err(EXIT_FAILURE, "ioctl(0, TIOCLINUX, ...) failed");
+    data.subcode = TIOCL_PASTESEL;
+    ioctl(0, TIOCLINUX, &data.subcode);
+    if (res != 0)
+      err(EXIT_FAILURE, "ioctl(0, TIOCLINUX, ...) failed");
+    exit(EXIT_SUCCESS);
+}
+

tests/main/snap-seccomp-blocks-tty-injection/test-tiocsti.c

@@ -0,0 +1,30 @@
+#define _GNU_SOURCE
+#include <termios.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <sys/syscall.h>
+#include <errno.h>
+
+static int ioctl64(int fd, unsigned long nr, void *arg) {
+  errno = 0;
+  return syscall(__NR_ioctl, fd, nr, arg);
+}
+
+int main(void) {
+  int res;
+  char pushmeback = '#';
+
+  unsigned long syscallnr = TIOCSTI;
+  res = ioctl64(0, syscallnr, &pushmeback);
+  printf("normal TIOCSTI: %d (%m)\n", res);
+
+#ifdef __LP64__
+  // this high bit check only works on 64bit systems, on 32bit it will fail:
+  // "error: left shift count >= width of type [-Werror=shift-count-overflow]"
+  syscallnr = TIOCSTI | (1UL<<32);
+#endif
+  res = ioctl64(0, syscallnr, &pushmeback);
+  printf("high-bit-set TIOCSTI: %d (%m)\n", res);
+  return res;
+}