Package: release.debian.org Severity: normal X-Debbugs-Cc: autopkgt...@packages.debian.org, Debian CI team <team...@tracker.debian.org> Control: affects -1 + src:autopkgtest User: release.debian....@packages.debian.org Usertags: unblock
Hi. (I'm not sure "unblock" is the right request type.) unblock autopkgtest/5.49 Summary: Please would you arrange that src:autopkgtest 5.49 will migrate (when the 10 days are up) despite the apparent regression in the autopkgtest for src:surf? I have looked at the logs for the test, and I am confident that the apparent regression is not due to any change in the autopkgtest package. It looks like the tests are flaky (and the surf package is also possibly just broken). I hope it's right for me to file this request now rather than waiting for the 10 days to be up. Analysing the "regression" in more detail: Firstly, note that two versions of autopkgtest are involved in this scenario, in totally different roles. One is the autopkgtest that's used as a test runner in ci.debian.net. That autopkgtest is *not being updated* here. Presumably it will be updated to 5.49 in due course if 5.49 updates to testing. The other is the autopkgtest from sid, the migration candidate, which is pulled in as a dependency because it's listed in the dependencies of one of surf's test cases. It is this latter autopkgtest which is impugned by the "regression". The surf package has three test cases, which I will call "command1", "command2" and "command3", in the order from d/t/control, as ci.d.n's autopkgtest calls thme. "command1" passes, is marked superficial, and is a simple `cmp`. "command3" Depends on autopkgtest. This is because it uses autopkgtest's README.package-tests as test data. The test fails with an error message which seems to indicate that surf, or webkit, or webdriver, or something, didn't start up at all. The test is marked flaky, so this is not classed as a "regression". "command2" is *not* marked flaky, and fails in the britney-requested migration runs. But it does *not* Depend on autopkgtest. Indeed, looking at the test logs, autopkgtest isn't even installed! The error message is similar to the failure from "command3". Or to put it another way, if it weren't for the flaky "command3" test case, nothing would even have worried that a new autopkgtest might break surf - the test wouldn't have been triggered. The "regression" appears only on riscv64. All of the changes in autopkgtest are arch-neutral. I have grepped the surf source code and it doesn't invoke autopkgtest. This is discussed some more in the bug report where Paul Gevers asked for the "command3" test to be marked flaky: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1055639 I did ask ci.d.n to retry the baseline, and to retry the failed test, but this didn't change the results. Even so, it seems impossible that this failure can be blamed on the new autopkgtest. I filed a bug about the "command2" test being flaky too https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1103356 There hasn't been a response from the maintainer yet. Outside the freeze, this would eventually result in surf being removed from testing, but that will come too slowly now. Additionally, there is another bug against surf saying that it doesn't work at all: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1054150 I think there is probably something inherently flaky inside surf :-/. So, in summary: it seems clear that this "regression" is not real. What is in autopkgtest 5.49 The autopkgtest that is currently in testing, 5.47, has (like most previous versions) some serious security problems. It is only suitable for use with highly-isolating virtualisation environments. This is mostly because it makes a number of important directories world-writeable. It also plays fast and loose with ownership and user-vs-root, and the autopkgtest-virt-null backend (which does not offer any isolation whatsoever) falsely advertises isolation-container. These things are all fixed in 5.48, with a NEWS entry added in 5.49. I think it is important to get this security update into trixie. There is some risk that the changes will break something. The design intent is to avoid operational breakage in existing containerised or virtualised deployments - the code looks for the isolation capabilities, and uses the world-writeable permissions and a relaxed attitude, when those capabilities are present. This is tested in the extensive test suite - the salsa CI tests are quite comprehensive. There are autopkgtests too. So I'm hoping that there won't be any significant fallout. Nevertheless, if there is, I think we want to detect it early. The alternatives to migrating this to trixie now are to delay these critical security fixes for another ~two years, or to try to do them as a stable release update or security update. Both of these are quite unpalatable. I say "critical security fixes", because right now anyone who runs autopkgtest from their own normal development environment, and is using anything other than full container-based virtualisation, is exposing their main account to all other uids on their system (for example, uids used for privilege separation, or maybe, container uid namespaces). There is additionally one small bugfix, to install a missing file. The changes in question are in the changelog in detail, and summarised in NEWS.Debian. Thanks for your attention, Ian. diff --git a/Makefile b/Makefile index 13ef15b6..ba9ab010 100644 --- a/Makefile +++ b/Makefile @@ -97,6 +97,7 @@ install: $(INSTALL_DATA) $(rstfiles) $(htmlfiles) $(docdir) $(INSTALL_PROG) lib/*.sh $(datadir)/lib/ $(INSTALL_PROG) lib/in-testbed/*.sh $(datadir)/lib/in-testbed/ + $(INSTALL_PROG) lib/arch-is-concerned.pl $(datadir)/lib/ $(INSTALL_PROG) lib/parse-deps.pl $(datadir)/lib/ $(INSTALL_PROG) lib/unshare-helper $(datadir)/lib/ $(INSTALL_PROG) setup-commands/*[!~] $(datadir)/setup-commands diff --git a/debian/NEWS b/debian/NEWS index 2fe8d5ed..920b4a37 100644 --- a/debian/NEWS +++ b/debian/NEWS @@ -1,3 +1,25 @@ +autopkgtest (5.48) unstable; urgency=medium + + Security fixes involving many permissiosn changes: + * Several directories are nowworld-writeable only when used with + suitably isolating virtualisation servers. Previously they were + world-writeable in circumstances where this wasn't safe. + * autopkgtest-virt-null no longer advertises isolation-machine. + This was a dangerous lie. For environments where the caller provides + isolation, there's a new --fake-capability option to restore the + previous behaviour. + * autopkgtest-virt-unshare no longer offers the `downtmp-host` + capability, since it is not safe for host processes to directly copy + to and from the testbed which has a different security context. + * There's a new --insecure option to autopkgtest which enables some old, + risky, behaviours, in some circumstances. + * autopkgtest-virt-ssh should not be provided with a way to get root on + the testbed, unless the whole testbed is fully isolated from any + untrusted code. This security restriction was alwsays implied by the + behaviour, but has now been documented. + + -- Ian Jackson <ijack...@chiark.greenend.org.uk> Wed, 16 Apr 2025 19:38:57 +0100 + autopkgtest (5.0) unstable; urgency=medium * autopkgtest 4.0 with its "autopkgtest" program has been around for a diff --git a/debian/changelog b/debian/changelog index 05f00b86..5a500fbc 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,33 @@ +autopkgtest (5.49) unstable; urgency=medium + + * NEWS.Debian: Document the security fixes in 5.48. + + -- Ian Jackson <ijack...@chiark.greenend.org.uk> Wed, 16 Apr 2025 19:41:19 +0100 + +autopkgtest (5.48) unstable; urgency=medium + + [ Ural Tunaboyu ] + * Makefile: install arch-is-concerned.pl. Fixes testing on very + old distros with very old python3-debian. + + [ Ian Jackson ] + * SECURITY: autopkgtest-virt-*: Don't make downtmp dangerously world-writeable + * SECURITY: Fix mode (permissions) of AUTOPKGTEST_ARTIFACTS + * SECURITY: autopkgtest-virt-null: Do not advertise isolation-machine + * SECURITY: Fix autopkgtest-virt-unshare to not advertise downtmp-host + * SECURITY: Only chmod copied-down things world-writeable with --insecure + * autopkgtest-virt-null: Add --fake-capabilities option + * Clarify that mangling config in $HOME is breaks-testbed + * No suggested-normal-user without root-on-testbed + * If necessary, chown the scratchspace to the normal user + * Introduce autopkgtest --insecure option + * d/t/control: Declare isolation-container for tests/autopkgtest + * Centralise downtmp and capabilities + * autopkgtest-virt-qemu: Move the base caps list to a global + * Make downtmp-host= cap filtering more careful + + -- Ian Jackson <ijack...@chiark.greenend.org.uk> Mon, 14 Apr 2025 00:31:53 +0100 + autopkgtest (5.47) unstable; urgency=medium [ Ian Jackson ] diff --git a/debian/tests/control b/debian/tests/control index 0c02766f..08703f75 100644 --- a/debian/tests/control +++ b/debian/tests/control @@ -9,6 +9,7 @@ Depends: fakeroot, python3-cap-ng, Restrictions: + isolation-container, needs-sudo, Tests: diff --git a/doc/README.package-tests.rst b/doc/README.package-tests.rst index d71cd27e..7575f93f 100644 --- a/doc/README.package-tests.rst +++ b/doc/README.package-tests.rst @@ -42,6 +42,9 @@ there is no need for the test to clean up files left there). Tests can expect that the ``$HOME`` environment variable to be set to a directory that exists and is writeable by the user running the test. +Tests should not make configuration changes in ``$HOME``, or other +destructive changes, unless they declare ``breaks-testbed``, +``isolation-machine``, or ``isolation-container``. If tests want to create artifacts which are useful to attach to test results, such as additional log files or screenshots, they can put them @@ -49,6 +52,10 @@ into the directory specified by the ``$AUTOPKGTEST_ARTIFACTS`` environment variable. When using the ``--output-dir`` option, they will be copied into ``outputdir/artifacts/``. +If the test case declares the ``isolation-machine`` or +``isolation-container`` restriction, ``$AUTOPKGTEST_ARTIFACTS`` will +be world-writeable. + Since autopkgtest version 5.43, the architecture of the packages that are being tested is made available in the test environment via the ``AUTOPKGTEST_TEST_ARCH`` environment variable, while the testbed architecture @@ -272,7 +279,7 @@ isolation-container The test wants to start services or open network TCP ports. This commonly fails in a simple chroot/schroot, so tests need to be run in their own container (e. g. autopkgtest-virt-lxc) or their own - machine/VM (e. g. autopkgtest-virt-qemu or autopkgtest-virt-null). + machine/VM (e. g. autopkgtest-virt-qemu). When running the test in a virtualization server which does not provide this (like autopkgtest-schroot) it will be skipped. @@ -301,7 +308,7 @@ isolation-machine The test wants to interact with the kernel, reboot the machine, or other things which fail in a simple schroot and even a container. Those tests need to be run in their own machine/VM (e. g. - autopkgtest-virt-qemu or autopkgtest-virt-null). When running the + autopkgtest-virt-qemu). When running the test in a virtualization server which does not provide this it will be skipped. diff --git a/doc/README.virtualisation-server.rst b/doc/README.virtualisation-server.rst index 9e427bb8..ab571700 100644 --- a/doc/README.virtualisation-server.rst +++ b/doc/README.virtualisation-server.rst @@ -100,6 +100,14 @@ suggested-normal-user=\ *username* names) may be advertised in which case more than one such user is available. + In testbeds without ``isolation-machine`` or + ``isolation-container``, the the testbed promises that this user + does not run any untrusted code. So it is OK for the root user + inside the testbed to complete trust this user (by, for example, + running code in files owned by this user). + + This is only meaningful if ``root-on-testbed`` is also advertised. + Command: open ------------- @@ -117,6 +125,22 @@ uses of the testbed to finish first, if necessary). freely by the test scripts and which is valid until the next ``close``, ``revert``, or ``quit``. +In testbeds with ``isolation-machine`` or ``isolation-container`` +capability, the scratchspace directory is world-writeable. +(This must be taken into account if the caller intends to impose +security boundaries within the testbed; but, such a situation would be +highly unusual for tests.) + +In testbeds without such isolation, the directory is mode 755, +and owned by the testbed user; therefore, with the ``root-on-testbed`` +capability, it is not writeable by the ``suggested-normal-user``. +If desired, the caller should use ``chown`` to change the ownership, +or create a suitable subdirectory. + +(In autopkgtest 5.47 and earlier this directory was always mode 1777, +like ``/tmp``. This was a security vulnerability in non-isolating +virt environments, and overly restrictive in isolating ones.) + Command: revert --------------- diff --git a/lib/VirtSubproc.py b/lib/VirtSubproc.py index 1cc7ebd6..5fe5145a 100644 --- a/lib/VirtSubproc.py +++ b/lib/VirtSubproc.py @@ -359,27 +359,63 @@ def cmd_open(c, ce): return [downtmp] -def downtmp_mktemp(path): +def downtmp_mktemp(capabilities, path, downtmp_prefix): '''Generate a downtmp When a path is given, this is the downtmp that we created when opening the testbed the first time. We always want to keep the same path between resets, as built package trees sometimes refer to absolute paths and thus fail if they get moved around. + + capabilities will be used to determine the appropriate permissions. + + If downtmp_prefix is not None, updates capabilities to add an downtmp-host, + consisting of downtmp_prefix + the downtmp path. ''' + + # The caller is entitled to use the scratch directory normally, without taking + # the kind of extreme special care needed for a world-writeable directory. + if any(cap in ['isolation-container', 'isolation-machine'] for cap in capabilities): + # The testbed is fully isolated by the host, so the only things running + # in the testbed are part of the testbed or specified by the caller. + # So there is no untrusted code. + # + # Make the scratch directory world-writeable is convenient for use cases + # that have to deal with multiple users. + # + # If the downtmp is visible to the host, then the isolation is not enough. + # This assertion is a double-check, therefore, that we aren't exposing + # the caller's code (which is using the downtmp) to possible hostile actions. + assert downtmp_prefix is None + mode = '777' + else: + # Untrusted code might be running in the testbed - so we must protect it. + mode = '755' + if path: - check_exec(['/bin/mkdir', '--mode=1777', '--parents', downtmp_open], + check_exec(['/bin/mkdir', f"--mode={mode}", '--parents', downtmp_open], downp=True) - return path + d = path else: d = check_exec(['/bin/mktemp', '--directory', '--tmpdir', 'autopkgtest.XXXXXX'], downp=True, outp=True) - check_exec(['/bin/chmod', '1777', d], downp=True) - return d + check_exec(['/bin/chmod', mode, d], downp=True) + if downtmp_prefix is not None: + capabilities.append('downtmp-host=' + downtmp_prefix + d) + return d + +def downtmp_remove(capabilities): + '''Removes the downtmp -def downtmp_remove(): + Also removes any downtmp-host entry from capabilities. + ''' global downtmp, auxverb + + # Remove from capabilities first. Some call sites catch exceptions and continue, + # and they also want to see downtmp removed. + capabilities[:] = [c for c in capabilities if not c.startswith('downtmp-host=')] + if downtmp: execute_timeout(None, copy_timeout, auxverb + ['rm', '-rf', '--', downtmp]) diff --git a/lib/adt_testbed.py b/lib/adt_testbed.py index 33a51cf6..37407f80 100644 --- a/lib/adt_testbed.py +++ b/lib/adt_testbed.py @@ -106,6 +106,7 @@ class Testbed: enable_apt_fallback: bool = True, shell_fail: bool = False, needs_internet: str = 'run', + insecure: bool = False, ) -> None: self.sp = None self.lastsend = None @@ -130,6 +131,7 @@ class Testbed: self.apt_upgrade = apt_upgrade self.copy_files = copy_files self.initial_kernel_version = None + self.insecure = insecure # tests might install a different kernel; [(testname, reboot_marker, kver)] self.test_kernel_versions: List[Tuple[str, str, str]] = [] # used for tracking kernel version changes @@ -354,6 +356,23 @@ class Testbed: ) as reader: self.check_exec(['sh', '-euc', reader.read(), 'sh', self.user]) + if self.su_user is not None and self.su_user != 'root' and not any(c in ['isolation-machine', 'isolation-container'] for c in self.caps): + # scratch directory is owned by root and not world-writeable. + # (See the rules in README.virtualisation-server.) + # + # But, we like to run things (particularly, tests) as a normal user. + # That normal user needs to be able to write to this directory. + # + # Once upon a time, this was possible because the scratch directory was 1777. + # Nowadays, we make it possible by making it owned by the normal user. + # + # This means that we make root on the testbed completely trust self.user. + # This is OK because either: + # self.user came from the testbed capability suggested-normal-user= + # self.user came from the --user command line option + # and in both cases this trust relationship is documented there. + self.check_exec(['chown', self.su_user, '--', self.scratch]) + # determine testbed architecture self.dpkg_arch = self.check_exec(['dpkg', '--print-architecture'], True).strip() adtlog.info('testbed dpkg architecture: ' + self.dpkg_arch) @@ -1330,6 +1349,10 @@ class Testbed: # This should come first so that it affects all CLI options argv.append('--debug') + if any(r in ['isolation-machine', 'isolation-container'] for r in test.restrictions): + # Must come early, because it influences behaviour of other options eg --artifacts + argv.append('--isolation') + argv.extend([ '--artifacts={}'.format(test_artifacts), '--chdir={}'.format(tree.tb), @@ -2155,6 +2178,11 @@ class TestbedPath: if rc != 0: # chowning doesn't work on all shared downtmps, try to chmod # instead + + # I believe this is only needed for autopkgtest-virt-unshare, which + # no longer advertises a downtmp at all. + # But we allow it still, with --insecure, in case someone needs it. + assert self.insecure self.testbed.check_exec(['chmod', '-R', 'go+rwX', '--', self.tb]) def copyup(self, check_existing=False): diff --git a/lib/autopkgtest_args.py b/lib/autopkgtest_args.py index 91e383a2..80e8b633 100644 --- a/lib/autopkgtest_args.py +++ b/lib/autopkgtest_args.py @@ -268,6 +268,11 @@ for details.''' action=AppendCommaSeparatedArg, help='Run tests even if these restrictions would ' 'normally prevent it') + g_setup.add_argument('--insecure', action='store_true', default=False, + help='Run without caring about security. ' + 'Tests might corrupt the testbed, or escape from it. ' + 'Other processes on the host or the testbed ' + 'might be able to suborn the account running autopkgtest.') # privileges g_priv = parser.add_argument_group('user/privilege handling options') diff --git a/lib/in-testbed/wrapper.sh b/lib/in-testbed/wrapper.sh index d352177b..e2f57802 100755 --- a/lib/in-testbed/wrapper.sh +++ b/lib/in-testbed/wrapper.sh @@ -28,6 +28,7 @@ cleanup () { trap cleanup EXIT INT QUIT PIPE getopt_temp="debug" +getopt_temp="$getopt_temp,isolation" getopt_temp="$getopt_temp,artifacts:" getopt_temp="$getopt_temp,chdir:" getopt_temp="$getopt_temp,env:" @@ -42,12 +43,14 @@ getopt_temp="$(getopt -o '' --long "$getopt_temp" -n "$0" -- "$@")" eval "set -- $getopt_temp" unset getopt_temp +artifacts_dir_mode=755 + while [ "$#" -gt 0 ]; do case "$1" in (--artifacts) debug "creating AUTOPKGTEST_ARTIFACTS: $2" # shellcheck disable=SC2174 - mkdir -p -m 1777 -- "$2" + mkdir -p -m "$artifacts_dir_mode" -- "$2" export AUTOPKGTEST_ARTIFACTS="$2" export ADT_ARTIFACTS="$2" shift 2 @@ -80,6 +83,12 @@ while [ "$#" -gt 0 ]; do shift 2 ;; + (--isolation) + debug "testbed provides isolation, using world-writeable artifacts directory" + artifacts_dir_mode=777 + shift + ;; + (--make-executable) debug "marking as executable: $2" chmod +x -- "$2" diff --git a/runner/autopkgtest b/runner/autopkgtest index df915a33..4d2c5fb9 100755 --- a/runner/autopkgtest +++ b/runner/autopkgtest @@ -909,6 +909,7 @@ def main(): apt_default_release=opts.apt_default_release, apt_upgrade=opts.apt_upgrade, test_arch=opts.test_architecture, + insecure=opts.insecure, ) testbed.start() testbed.open() diff --git a/runner/autopkgtest.1 b/runner/autopkgtest.1 index eaa8f078..b0d92630 100644 --- a/runner/autopkgtest.1 +++ b/runner/autopkgtest.1 @@ -206,6 +206,10 @@ If the setup commands affect anything in boot directories (like /boot or rebooted after the setup commands. This can be suppressed by creating a file .BR /run/autopkgtest_no_reboot.stamp . +.B Warning: +The way that autopkgtest uses this user means it will be completely +trusted by root inside the testbed. + .TP .BI \-\-setup\-commands\-boot= commands Run @@ -360,6 +364,20 @@ The default is not to use anything, except that if \fB\-\-user\fR is supplied or root on the testbed is not available the default is \fBfakeroot\fR. +.TP +.B \-\-insecure +Relaxes file permissions etc., to try to make things not fail with +permission errors. + +Passing this option might allow tests to corrupt the testbed (if it +doesn't support resetting), or to escape from it, or for untrusted +processes on the host or the testbed to take over the account running +autopkgtest. + +It is safe to pass \-\-insecure on a dedicated throwaway host. + +This partially restores the behaviour of autopkgtest 5.47 and earlier, +which had several world-writable directories. .SH DEBUGGING OPTIONS .TP diff --git a/tests/autopkgtest b/tests/autopkgtest index f2092970..21cfc2e7 100755 --- a/tests/autopkgtest +++ b/tests/autopkgtest @@ -1,5 +1,11 @@ #!/usr/bin/python3 +# WARNING +# +# This script may not be safe to run, except inside a container. +# Some of the test cases assume that there is no hostile code around. +# So don't run it on a shared machine, or in your normal desktop session. + # this testsuite is part of autopkgtest # autopkgtest is a tool for testing Debian binary packages # @@ -323,7 +329,7 @@ class AdtTestCase(unittest.TestCase): @property def has_isolation_machine(self): - return self.virt_args[0] in ['qemu', 'null'] + return self.virt_args[0] in ['qemu'] @property def has_isolation_container(self): @@ -333,6 +339,7 @@ class AdtTestCase(unittest.TestCase): return self.virt_args[0] not in [ 'chroot', 'docker', + 'null', 'podman', 'schroot', 'unshare', diff --git a/virt/autopkgtest-virt-chroot b/virt/autopkgtest-virt-chroot index c7590ff7..ae700e93 100755 --- a/virt/autopkgtest-virt-chroot +++ b/virt/autopkgtest-virt-chroot @@ -68,16 +68,13 @@ def hook_open(): def hook_downtmp(path): global capabilities - d = VirtSubproc.downtmp_mktemp(path) - if chroot_dir: - capabilities.append('downtmp-host=%s/%s' % (chroot_dir, d)) + d = VirtSubproc.downtmp_mktemp(capabilities, path, chroot_dir) return d def hook_cleanup(): global capabilities - VirtSubproc.downtmp_remove() - capabilities = [c for c in capabilities if not c.startswith('downtmp-host')] + VirtSubproc.downtmp_remove(capabilities) def hook_capabilities(): diff --git a/virt/autopkgtest-virt-docker b/virt/autopkgtest-virt-docker index ae7371fc..ae6d9484 100755 --- a/virt/autopkgtest-virt-docker +++ b/virt/autopkgtest-virt-docker @@ -295,7 +295,7 @@ def hook_downtmp(path): VirtSubproc.check_exec(['mkdir', '-m', '777', d], downp=True) capabilities.append('downtmp-host=' + d) else: - d = VirtSubproc.downtmp_mktemp(path) + d = VirtSubproc.downtmp_mktemp(capabilities, path, None) return d @@ -307,8 +307,7 @@ def hook_revert(): def hook_cleanup(): global capabilities, container_id, shared_dir - VirtSubproc.downtmp_remove() - capabilities = [c for c in capabilities if not c.startswith('downtmp-host')] + VirtSubproc.downtmp_remove(capabilities) if shared_dir: shutil.rmtree(shared_dir) diff --git a/virt/autopkgtest-virt-lxc b/virt/autopkgtest-virt-lxc index 7f114f27..a319a12a 100755 --- a/virt/autopkgtest-virt-lxc +++ b/virt/autopkgtest-virt-lxc @@ -279,7 +279,7 @@ def hook_downtmp(path): VirtSubproc.check_exec(['mkdir', '-m', '777', d], downp=True, timeout=30) capabilities.append('downtmp-host=' + d) else: - d = VirtSubproc.downtmp_mktemp(path) + d = VirtSubproc.downtmp_mktemp(path, capabilities, None) return d @@ -299,8 +299,7 @@ def hook_wait_reboot(*func_args, **kwargs): def hook_cleanup(): global capabilities, shared_dir, lxc_container_name - VirtSubproc.downtmp_remove() - capabilities = [c for c in capabilities if not c.startswith('downtmp-host')] + VirtSubproc.downtmp_remove(capabilities) if lxc_container_name: VirtSubproc.check_exec( diff --git a/virt/autopkgtest-virt-lxd b/virt/autopkgtest-virt-lxd index 95297aa1..856731c4 100755 --- a/virt/autopkgtest-virt-lxd +++ b/virt/autopkgtest-virt-lxd @@ -218,7 +218,7 @@ def hook_open(): def hook_downtmp(path): - return VirtSubproc.downtmp_mktemp(path) + return VirtSubproc.downtmp_mktemp(capabilities, path, None) def hook_revert(): @@ -279,7 +279,7 @@ def hook_wait_reboot(*func_args, **kwargs): def hook_cleanup(): - VirtSubproc.downtmp_remove() + VirtSubproc.downtmp_remove(capabilities) VirtSubproc.check_exec([args.command, 'delete', '--force', container_name], timeout=600) diff --git a/virt/autopkgtest-virt-null b/virt/autopkgtest-virt-null index 0ab2f30b..514a34ca 100755 --- a/virt/autopkgtest-virt-null +++ b/virt/autopkgtest-virt-null @@ -34,25 +34,27 @@ import VirtSubproc import adtlog -capabilities = [ - 'isolation-machine', -] +capabilities = [] if os.getuid() == 0: capabilities.append('root-on-testbed') def parse_args(): - global args + global args, capabilities parser = argparse.ArgumentParser() parser.add_argument('-d', '--debug', action='store_true', help='Enable debugging output') parser.add_argument('--retain-tmp', action='store_true', help='Retain temporary subdir (you should set TMPDIR)') + parser.add_argument('--fake-capability', action='append', + metavar='CAPABILITY', default=[], + help='Declare capability CAPABILITY') args = parser.parse_args() if args.debug: adtlog.verbosity = 2 + capabilities += args.fake_capability def hook_open(): @@ -62,7 +64,7 @@ def hook_open(): def hook_downtmp(path): global capabilities - d = VirtSubproc.downtmp_mktemp(path) + d = VirtSubproc.downtmp_mktemp(capabilities, path, '') capabilities.append('downtmp-host=' + d) return d @@ -71,8 +73,7 @@ def hook_cleanup(): global capabilities if not args.retain_tmp: - VirtSubproc.downtmp_remove() - capabilities = [c for c in capabilities if not c.startswith('downtmp-host')] + VirtSubproc.downtmp_remove(capabilities) def hook_capabilities(): diff --git a/virt/autopkgtest-virt-null.1 b/virt/autopkgtest-virt-null.1 index 142a6719..ef399be1 100644 --- a/virt/autopkgtest-virt-null.1 +++ b/virt/autopkgtest-virt-null.1 @@ -24,6 +24,21 @@ so as to reuse the same system as a testbed. .BR \-d " | " \-\-debug Enables debugging output. Probably not hugely interesting. +.TP +.BI \-\-fake-capability " CAPABILITY" +Advertise +.I CAPABILITY +in the list of capabilities, +regardless of whether the capability is implemented. +Note that this is hazardous - +tests may malfunction, or even mess up the system. + +In autopkgtest 5.47 and earlier, +autopkgtest\-virt\-null falsely advertises +.BR isolation\-machine . +This option can be used to restore that behaviour: +.BR \-\-fake\-capability=isolation\-machine . + .TP .BR \-\-retain-tmp Do not delete the temporary directory on exit. diff --git a/virt/autopkgtest-virt-qemu b/virt/autopkgtest-virt-qemu index 72799332..0a86f209 100755 --- a/virt/autopkgtest-virt-qemu +++ b/virt/autopkgtest-virt-qemu @@ -65,6 +65,13 @@ args = None factory: Optional[QemuFactory] = None qemu: Optional[QemuSession] = None normal_user = None +capabilities = [ + 'isolation-machine', + 'reboot', + 'revert', + 'revert-full-system', + 'root-on-testbed', +] def parse_args() -> None: @@ -645,11 +652,11 @@ def hook_downtmp(path: str) -> None: # trees # downtmp = '/run/autopkgtest/shared/tmp' # VirtSubproc.check_exec(['mkdir', '-m', '1777', downtmp], downp=True) - return VirtSubproc.downtmp_mktemp(path) + return VirtSubproc.downtmp_mktemp(capabilities, path, None) def hook_revert() -> None: - VirtSubproc.downtmp_remove() + VirtSubproc.downtmp_remove(capabilities) hook_cleanup() hook_open() @@ -699,16 +706,10 @@ def hook_wait_reboot(*func_args: Any, **kwargs: Any) -> None: def hook_capabilities() -> List[str]: global normal_user - caps = [ - 'isolation-machine', - 'reboot', - 'revert', - 'revert-full-system', - 'root-on-testbed', - ] - # disabled, see hook_downtmp() - # caps.append('downtmp-host=%s' % os.path.join(qemu.workdir, 'shared', 'tmp')) + caps = list(capabilities) if normal_user: + # Adding this here, each time, rather than putting it in `capabilities`, + # avoids worrying about whether it will change, or trying to filter it out and re-add it. caps.append('suggested-normal-user=' + normal_user) return caps diff --git a/virt/autopkgtest-virt-schroot b/virt/autopkgtest-virt-schroot index 59faa841..bf334d2a 100755 --- a/virt/autopkgtest-virt-schroot +++ b/virt/autopkgtest-virt-schroot @@ -110,9 +110,13 @@ def parse_args(): adtlog.debug('have "root-on-testbed" capability') capabilities.append('root-on-testbed') - if os.geteuid() != 0: - username = pwd.getpwuid(os.geteuid()).pw_name - capabilities.append('suggested-normal-user=' + username) + if os.geteuid() != 0: + username = pwd.getpwuid(os.geteuid()).pw_name + # Passing this capability promises that it is OK for root on the testbed + # to completely trust this user. That is correct, because this user + # is in schroot's `root-users` configuration - so that trust relationship + # is already defined by the schroot administrator. + capabilities.append('suggested-normal-user=' + username) def hook_open(): @@ -131,7 +135,7 @@ def hook_open(): def hook_downtmp(path): global capabilities - d = VirtSubproc.downtmp_mktemp(path) + d = VirtSubproc.downtmp_mktemp(capabilities, path, None) # determine mount location location = VirtSubproc.check_exec(['schroot', '--location', '--chroot', @@ -162,7 +166,7 @@ def hook_revert(): def hook_cleanup(): global schroot, sessid, capabilities - VirtSubproc.downtmp_remove() + VirtSubproc.downtmp_remove(capabilities) # sometimes fails on EBUSY retries = 10 try: @@ -180,7 +184,7 @@ def hook_cleanup(): adtlog.warning('schroot --end-session timed out after %d seconds;' 'please clean up manually', e.timeout_secs) - capabilities = [c for c in capabilities if not c.startswith('downtmp-host')] + capabilities = [c for c in capabilities if not c.startswith('downtmp-host=')] def hook_capabilities(): diff --git a/virt/autopkgtest-virt-ssh b/virt/autopkgtest-virt-ssh index 21c7d492..a1fa36bf 100755 --- a/virt/autopkgtest-virt-ssh +++ b/virt/autopkgtest-virt-ssh @@ -426,7 +426,7 @@ def hook_open(): def hook_downtmp(path): - return VirtSubproc.downtmp_mktemp(path) + return VirtSubproc.downtmp_mktemp(capabilities, path, None) def hook_revert(): @@ -492,7 +492,7 @@ def hook_cleanup(): global capabilities, workdir, cleanup_paths, sshcmd try: - VirtSubproc.downtmp_remove() + VirtSubproc.downtmp_remove(capabilities) if cleanup_paths: VirtSubproc.check_exec(['rm', '-rf'] + cleanup_paths, downp=True, timeout=VirtSubproc.copy_timeout) @@ -502,8 +502,6 @@ def hook_cleanup(): execute_setup_script('cleanup') - capabilities = [c for c in capabilities if not c.startswith('downtmp-host')] - # terminate ssh connection muxer; it inherits our stderr (which causes an # eternal hang of tee processes), and we are going to remove the socket dir # anyway diff --git a/virt/autopkgtest-virt-ssh.1 b/virt/autopkgtest-virt-ssh.1 index b815e0b5..723fb4f4 100644 --- a/virt/autopkgtest-virt-ssh.1 +++ b/virt/autopkgtest-virt-ssh.1 @@ -159,6 +159,21 @@ The behaviour of is as described by the AutomatedTesting virtualisation regime specification. +.SH SECURITY WARNING - USE OF ROOT + +If autopkgtest\-virt\-ssh is used in a way that means it has root +access on the testbed, the testbed environment +must be an isolated one which +.IR "does not ever run any untrusted code" , +or is itself not trusted by anything. + +This promise is implied by user-switching capabilities that +autopkgtest\-virt\-ssh may advertise, depending in a complex way on +the provided configuration and command line options. + +If autopkgtest\-virt\-ssh only has access to a single user on the +remote host, this problem does not arise. + .SH NOTES \fBautopkgtest\fR does not run \fBapt\-get update\fR at the start of a package diff --git a/virt/autopkgtest-virt-unshare b/virt/autopkgtest-virt-unshare index 243e0bb1..a5432ffd 100755 --- a/virt/autopkgtest-virt-unshare +++ b/virt/autopkgtest-virt-unshare @@ -191,12 +191,7 @@ def hook_open(): def hook_downtmp(path): - global capabilities - - tmp = VirtSubproc.downtmp_mktemp(path) - - capabilities.append(f'downtmp-host={rootdir}/{tmp}') - + tmp = VirtSubproc.downtmp_mktemp(capabilities, path, None) return tmp @@ -211,7 +206,7 @@ def hook_cleanup(revert=False): VirtSubproc.check_exec(['unshare', '--map-auto', '--map-root-user', 'rm', '-rf', rootdir]) - capabilities = [c for c in capabilities if not c.startswith('downtmp-host')] + capabilities = [c for c in capabilities if not c.startswith('downtmp-host=')] if not revert and temp_tarball: path = pathlib.Path(tarball)
