Package: release.debian.org Severity: normal Tags: buster User: release.debian....@packages.debian.org Usertags: pu
I have prepared a update for cloud-init in buster targeting CVE-2021-3429. After consulation with the security team, we've decided that this doesn't warrant a DSA on its own but that it should be included with a stable point release. The issue in question is that cloud-init can be configured to generate and apply a random password for an account on the system where it's running. When it does so, the password is logged to a world-readable file. The immediate fix is to stop logging to that file, and Ubuntu is also going to stop making that file world-readable in the future. In the future we may consider changing the default permissions of the log file, but for buster we won't do that. The proposed debdiff, backported from upstream's fix, is attached. I have verified that functionality is preserved but the password is no longer logged to /var/log/cloud-init-output.log. It is still logged to the console as expected (access to which is typically access-controlled in cloud environments). Please note that, since the details of this issue were just made public, the fix has been uploaded to unstable only within the past few minutes. Thanks noah
diff -Nru cloud-init-20.2/debian/changelog cloud-init-20.2/debian/changelog --- cloud-init-20.2/debian/changelog 2020-06-30 17:20:38.000000000 -0700 +++ cloud-init-20.2/debian/changelog 2021-03-19 09:43:23.000000000 -0700 @@ -1,3 +1,10 @@ +cloud-init (20.2-2~deb10u2) buster; urgency=high + + * Avoid logging generated passwords to world-readable log files. + CVE-2021-3429. (Closes: #985540) + + -- Noah Meyerhans <no...@debian.org> Fri, 19 Mar 2021 09:43:23 -0700 + cloud-init (20.2-2~deb10u1) buster; urgency=medium * Release for buster. No further changes. diff -Nru cloud-init-20.2/debian/patches/dont_log_generated_passwords.patch cloud-init-20.2/debian/patches/dont_log_generated_passwords.patch --- cloud-init-20.2/debian/patches/dont_log_generated_passwords.patch 1969-12-31 16:00:00.000000000 -0800 +++ cloud-init-20.2/debian/patches/dont_log_generated_passwords.patch 2021-03-19 09:41:57.000000000 -0700 @@ -0,0 +1,278 @@ +Description: Don't log generated passwords +Origin: upstream +Bug-Debian: https://bugs.debian.org/985540 +Applied-Upstream: https://github.com/canonical/cloud-init/commit/b794d426b9ab43ea9d6371477466070d86e10668 +--- +This patch header follows DEP-3: http://dep.debian.net/deps/dep3/ +Index: cloud-init/cloudinit/config/cc_set_passwords.py +=================================================================== +--- cloud-init.orig/cloudinit/config/cc_set_passwords.py ++++ cloud-init/cloudinit/config/cc_set_passwords.py +@@ -78,7 +78,6 @@ password. + """ + + import re +-import sys + + from cloudinit.distros import ug_util + from cloudinit import log as logging +@@ -213,7 +212,9 @@ def handle(_name, cfg, cloud, log, args) + if len(randlist): + blurb = ("Set the following 'random' passwords\n", + '\n'.join(randlist)) +- sys.stderr.write("%s\n%s\n" % blurb) ++ util.multi_log( ++ "%s\n%s\n" % blurb, stderr=False, fallback_to_stdout=False ++ ) + + if expire: + expired_users = [] +Index: cloud-init/cloudinit/config/tests/test_set_passwords.py +=================================================================== +--- cloud-init.orig/cloudinit/config/tests/test_set_passwords.py ++++ cloud-init/cloudinit/config/tests/test_set_passwords.py +@@ -74,10 +74,6 @@ class TestSetPasswordsHandle(CiTestCase) + + with_logs = True + +- def setUp(self): +- super(TestSetPasswordsHandle, self).setUp() +- self.add_patch('cloudinit.config.cc_set_passwords.sys.stderr', 'm_err') +- + def test_handle_on_empty_config(self, *args): + """handle logs that no password has changed when config is empty.""" + cloud = self.tmp_cloud(distro='ubuntu') +@@ -129,10 +125,11 @@ class TestSetPasswordsHandle(CiTestCase) + mock.call(['pw', 'usermod', 'ubuntu', '-p', '01-Jan-1970'])], + m_subp.call_args_list) + ++ @mock.patch(MODPATH + "util.multi_log") + @mock.patch(MODPATH + "util.is_BSD") + @mock.patch(MODPATH + "util.subp") + def test_handle_on_chpasswd_list_creates_random_passwords(self, m_subp, +- m_is_bsd): ++ m_is_bsd, m_multi_log): + """handle parses command set random passwords.""" + m_is_bsd.return_value = False + cloud = self.tmp_cloud(distro='ubuntu') +@@ -146,10 +143,32 @@ class TestSetPasswordsHandle(CiTestCase) + self.assertIn( + 'DEBUG: Handling input for chpasswd as list.', + self.logs.getvalue()) +- self.assertNotEqual( +- [mock.call(['chpasswd'], +- '\n'.join(valid_random_pwds) + '\n')], +- m_subp.call_args_list) ++ ++ self.assertEqual(1, m_subp.call_count) ++ args, _kwargs = m_subp.call_args ++ self.assertEqual(["chpasswd"], args[0]) ++ ++ stdin = args[1] ++ user_pass = { ++ user: password ++ for user, password ++ in (line.split(":") for line in stdin.splitlines()) ++ } ++ ++ self.assertEqual(1, m_multi_log.call_count) ++ self.assertEqual( ++ mock.call(mock.ANY, stderr=False, fallback_to_stdout=False), ++ m_multi_log.call_args ++ ) ++ ++ self.assertEqual(set(["root", "ubuntu"]), set(user_pass.keys())) ++ written_lines = m_multi_log.call_args[0][0].splitlines() ++ for password in user_pass.values(): ++ for line in written_lines: ++ if password in line: ++ break ++ else: ++ self.fail("Password not emitted to console") + + + # vi: ts=4 expandtab +Index: cloud-init/cloudinit/tests/test_util.py +=================================================================== +--- cloud-init.orig/cloudinit/tests/test_util.py ++++ cloud-init/cloudinit/tests/test_util.py +@@ -703,4 +703,60 @@ class TestReadCcFromCmdline: + assert expected_cfg == util.read_conf_from_cmdline(cmdline=cmdline) + + ++@mock.patch("cloudinit.util.grp.getgrnam") ++@mock.patch("cloudinit.util.os.setgid") ++@mock.patch("cloudinit.util.os.umask") ++class TestRedirectOutputPreexecFn: ++ """This tests specifically the preexec_fn used in redirect_output.""" ++ ++ @pytest.fixture(params=["outfmt", "errfmt"]) ++ def preexec_fn(self, request): ++ """A fixture to gather the preexec_fn used by redirect_output. ++ ++ This enables simpler direct testing of it, and parameterises any tests ++ using it to cover both the stdout and stderr code paths. ++ """ ++ test_string = "| piped output to invoke subprocess" ++ if request.param == "outfmt": ++ args = (test_string, None) ++ elif request.param == "errfmt": ++ args = (None, test_string) ++ with mock.patch("cloudinit.util.subprocess.Popen") as m_popen: ++ util.redirect_output(*args) ++ ++ assert 1 == m_popen.call_count ++ _args, kwargs = m_popen.call_args ++ assert "preexec_fn" in kwargs, "preexec_fn not passed to Popen" ++ return kwargs["preexec_fn"] ++ ++ def test_preexec_fn_sets_umask( ++ self, m_os_umask, _m_setgid, _m_getgrnam, preexec_fn ++ ): ++ """preexec_fn should set a mask that avoids world-readable files.""" ++ preexec_fn() ++ ++ assert [mock.call(0o037)] == m_os_umask.call_args_list ++ ++ def test_preexec_fn_sets_group_id_if_adm_group_present( ++ self, _m_os_umask, m_setgid, m_getgrnam, preexec_fn ++ ): ++ """We should setgrp to adm if present, so files are owned by them.""" ++ fake_group = mock.Mock(gr_gid=mock.sentinel.gr_gid) ++ m_getgrnam.return_value = fake_group ++ ++ preexec_fn() ++ ++ assert [mock.call("adm")] == m_getgrnam.call_args_list ++ assert [mock.call(mock.sentinel.gr_gid)] == m_setgid.call_args_list ++ ++ def test_preexec_fn_handles_absent_adm_group_gracefully( ++ self, _m_os_umask, m_setgid, m_getgrnam, preexec_fn ++ ): ++ """We should handle an absent adm group gracefully.""" ++ m_getgrnam.side_effect = KeyError("getgrnam(): name not found: 'adm'") ++ ++ preexec_fn() ++ ++ assert 0 == m_setgid.call_count ++ + # vi: ts=4 expandtab +Index: cloud-init/cloudinit/util.py +=================================================================== +--- cloud-init.orig/cloudinit/util.py ++++ cloud-init/cloudinit/util.py +@@ -492,7 +492,7 @@ def find_modules(root_dir): + + + def multi_log(text, console=True, stderr=True, +- log=None, log_level=logging.DEBUG): ++ log=None, log_level=logging.DEBUG, fallback_to_stdout=True): + if stderr: + sys.stderr.write(text) + if console: +@@ -501,7 +501,7 @@ def multi_log(text, console=True, stderr + with open(conpath, 'w') as wfh: + wfh.write(text) + wfh.flush() +- else: ++ elif fallback_to_stdout: + # A container may lack /dev/console (arguably a container bug). If + # it does not exist, then write output to stdout. this will result + # in duplicate stderr and stdout messages if stderr was True. +@@ -749,6 +749,26 @@ def redirect_output(outfmt, errfmt, o_ou + if not o_err: + o_err = sys.stderr + ++ # pylint: disable=subprocess-popen-preexec-fn ++ def set_subprocess_umask_and_gid(): ++ """Reconfigure umask and group ID to create output files securely. ++ ++ This is passed to subprocess.Popen as preexec_fn, so it is executed in ++ the context of the newly-created process. It: ++ ++ * sets the umask of the process so created files aren't world-readable ++ * if an adm group exists in the system, sets that as the process' GID ++ (so that the created file(s) are owned by root:adm) ++ """ ++ os.umask(0o037) ++ try: ++ group_id = grp.getgrnam("adm").gr_gid ++ except KeyError: ++ # No adm group, don't set a group ++ pass ++ else: ++ os.setgid(group_id) ++ + if outfmt: + LOG.debug("Redirecting %s to %s", o_out, outfmt) + (mode, arg) = outfmt.split(" ", 1) +@@ -758,7 +778,12 @@ def redirect_output(outfmt, errfmt, o_ou + owith = "wb" + new_fp = open(arg, owith) + elif mode == "|": +- proc = subprocess.Popen(arg, shell=True, stdin=subprocess.PIPE) ++ proc = subprocess.Popen( ++ arg, ++ shell=True, ++ stdin=subprocess.PIPE, ++ preexec_fn=set_subprocess_umask_and_gid, ++ ) + new_fp = proc.stdin + else: + raise TypeError("Invalid type for output format: %s" % outfmt) +@@ -780,7 +805,12 @@ def redirect_output(outfmt, errfmt, o_ou + owith = "wb" + new_fp = open(arg, owith) + elif mode == "|": +- proc = subprocess.Popen(arg, shell=True, stdin=subprocess.PIPE) ++ proc = subprocess.Popen( ++ arg, ++ shell=True, ++ stdin=subprocess.PIPE, ++ preexec_fn=set_subprocess_umask_and_gid, ++ ) + new_fp = proc.stdin + else: + raise TypeError("Invalid type for error format: %s" % errfmt) +Index: cloud-init/tests/integration_tests/test_logging.py +=================================================================== +--- /dev/null ++++ cloud-init/tests/integration_tests/test_logging.py +@@ -0,0 +1,22 @@ ++"""Integration tests relating to cloud-init's logging.""" ++ ++ ++class TestVarLogCloudInitOutput: ++ """Integration tests relating to /var/log/cloud-init-output.log.""" ++ ++ def test_var_log_cloud_init_output_not_world_readable(self, client): ++ """ ++ The log can contain sensitive data, it shouldn't be world-readable. ++ ++ LP: #1918303 ++ """ ++ # Check the file exists ++ assert client.execute("test -f /var/log/cloud-init-output.log").ok ++ ++ # Check its permissions are as we expect ++ perms, user, group = client.execute( ++ "stat -c %a:%U:%G /var/log/cloud-init-output.log" ++ ).split(":") ++ assert "640" == perms ++ assert "root" == user ++ assert "adm" == group +Index: cloud-init/tests/unittests/test_util.py +=================================================================== +--- cloud-init.orig/tests/unittests/test_util.py ++++ cloud-init/tests/unittests/test_util.py +@@ -687,6 +687,10 @@ class TestMultiLog(helpers.FilesystemMoc + util.multi_log(logged_string) + self.assertEqual(logged_string, self.stdout.getvalue()) + ++ def test_logs_dont_go_to_stdout_if_fallback_to_stdout_is_false(self): ++ util.multi_log('something', fallback_to_stdout=False) ++ self.assertEqual('', self.stdout.getvalue()) ++ + def test_logs_go_to_log_if_given(self): + log = mock.MagicMock() + logged_string = 'something very important' diff -Nru cloud-init-20.2/debian/patches/series cloud-init-20.2/debian/patches/series --- cloud-init-20.2/debian/patches/series 2020-06-30 17:20:38.000000000 -0700 +++ cloud-init-20.2/debian/patches/series 2021-03-19 09:36:30.000000000 -0700 @@ -5,3 +5,4 @@ cloud-init-before-chronyd.patch 0009-Drop-all-unused-extended-version-handling.patch 0012-Fix-message-when-a-local-is-missing.patch +dont_log_generated_passwords.patch
