diff -u apport-2.20.1/apport/report.py apport-2.20.1/apport/report.py --- apport-2.20.1/apport/report.py +++ apport-2.20.1/apport/report.py @@ -9,7 +9,7 @@ # option) any later version. See http://www.gnu.org/copyleft/gpl.html for # the full text of the license. -import subprocess, tempfile, os.path, re, pwd, grp, os, time +import subprocess, tempfile, os.path, re, pwd, grp, os, time, io import fnmatch, glob, traceback, errno, sys, atexit, locale, imp import xml.dom, xml.dom.minidom @@ -64,13 +64,29 @@ _transitive_dependencies(d, depends_set) -def _read_file(path, dir_fd=None): +def _read_proc_link(path, pid=None, dir_fd=None): + '''Use readlink() to resolve link. + + Return a string representing the path to which the symbolic link points. + ''' + if not _python2 and dir_fd is not None: + return os.readlink(path, dir_fd=dir_fd) + + return os.readlink("/proc/%s/%s" % (pid, path)) + + +def _read_proc_file(path, pid=None, dir_fd=None): '''Read file content. Return its content, or return a textual error if it failed. ''' try: - with open(path, 'rb', opener=lambda path, mode: os.open(path, mode, dir_fd=dir_fd)) as fd: + if not _python2 and dir_fd is not None: + proc_file = os.open(path, os.O_RDONLY | os.O_CLOEXEC, dir_fd=dir_fd) + else: + proc_file = "/proc/%s/%s" % (pid, path) + + with io.open(proc_file, 'rb') as fd: return fd.read().strip().decode('UTF-8', errors='replace') except (OSError, IOError) as e: return 'Error: ' + str(e) @@ -83,6 +99,10 @@ process, detect this, and attempt to attach/detach. ''' maps = 'Error: unable to read /proc maps file' + + if _python2: + return 'Error: python2 does not provide a secure way to read /proc maps file' + try: with open('maps', opener=lambda path, mode: os.open(path, mode, dir_fd=proc_pid_fd)) as fd: maps = fd.read().strip() @@ -516,23 +536,33 @@ if not self.pid: self.pid = int(pid) pid = str(pid) - proc_pid_fd = os.open('/proc/%s' % pid, os.O_RDONLY | os.O_PATH | os.O_DIRECTORY) + if not _python2: + try: + proc_pid_fd = os.open('/proc/%s' % pid, os.O_RDONLY | os.O_PATH | os.O_DIRECTORY) + except OSError as e: + if e.errno in (errno.EPERM, errno.EACCES): + raise ValueError('not accessible') + if e.errno == errno.ENOENT: + raise ValueError('invalid process') + else: + raise try: - self['ProcCwd'] = os.readlink('cwd', dir_fd=proc_pid_fd) + self['ProcCwd'] = _read_proc_link('cwd', pid, proc_pid_fd) except OSError: pass - self.add_proc_environ(proc_pid_fd=proc_pid_fd, extraenv=extraenv) - self['ProcStatus'] = _read_file('status', dir_fd=proc_pid_fd) - self['ProcCmdline'] = _read_file('cmdline', dir_fd=proc_pid_fd).rstrip('\0') + self.add_proc_environ(pid=pid, proc_pid_fd=proc_pid_fd, extraenv=extraenv) + self['ProcStatus'] = _read_proc_file('status', pid, proc_pid_fd) + self['ProcCmdline'] = _read_proc_file('cmdline', pid, proc_pid_fd).rstrip('\0') self['ProcMaps'] = _read_maps(proc_pid_fd) - try: - self['ExecutablePath'] = os.readlink('exe', dir_fd=proc_pid_fd) - except OSError as e: - if e.errno == errno.ENOENT: - raise ValueError('invalid process') - else: - raise + if 'ExecutablePath' not in self: + try: + self['ExecutablePath'] = _read_proc_link('exe', pid, proc_pid_fd) + except OSError as e: + if e.errno == errno.ENOENT: + raise ValueError('invalid process') + else: + raise for p in ('rofs', 'rwfs', 'squashmnt', 'persistmnt'): if self['ExecutablePath'].startswith('/%s/' % p): self['ExecutablePath'] = self['ExecutablePath'][len('/%s' % p):] @@ -554,14 +584,13 @@ # On Linux 2.6.28+, 'current' is world readable, but read() gives # EPERM; Python 2.5.3+ crashes on that (LP: #314065) if os.getuid() == 0: - with open('attr/current', opener=lambda path, mode: os.open(path, mode, dir_fd=proc_pid_fd)) as fd: - val = fd.read().strip() + val = _read_proc_file('attr/current', pid, proc_pid_fd) if val != 'unconfined': self['ProcAttrCurrent'] = val except (IOError, OSError): pass - ret = self.get_logind_session(proc_pid_fd) + ret = self.get_logind_session(pid, proc_pid_fd) if ret: self['_LogindSession'] = ret[0] @@ -586,10 +615,11 @@ if not pid: pid = os.getpid() pid = str(pid) - proc_pid_fd = os.open('/proc/%s' % pid, os.O_RDONLY | os.O_PATH | os.O_DIRECTORY) + if not _python2: + proc_pid_fd = os.open('/proc/%s' % pid, os.O_RDONLY | os.O_PATH | os.O_DIRECTORY) self['ProcEnviron'] = '' - env = _read_file('environ', dir_fd=proc_pid_fd).replace('\n', '\\n') + env = _read_proc_file('environ', pid, proc_pid_fd).replace('\n', '\\n') if env.startswith('Error:'): self['ProcEnviron'] = env else: @@ -1628,15 +1658,20 @@ int(m.group(2), 16), m.group(3))) @classmethod - def get_logind_session(klass, proc_pid_fd): + def get_logind_session(klass, pid=None, proc_pid_fd=None): '''Get logind session path and start time. Return (session_id, session_start_timestamp) if process is in a logind session, or None otherwise. ''' + if not _python2 and proc_pid_fd is not None: + cgroup_file = os.open('cgroup', os.O_RDONLY | os.O_CLOEXEC, dir_fd=proc_pid_fd) + else: + cgroup_file = "/proc/%s/cgroup" % pid + # determine cgroup try: - with open('cgroup', opener=lambda path, mode: os.open(path, mode, dir_fd=proc_pid_fd)) as f: + with io.open(cgroup_file) as f: for l in f: l = l.strip() if 'name=systemd:' in l and l.endswith('.scope') and '/session-' in l: diff -u apport-2.20.1/apport/ui.py apport-2.20.1/apport/ui.py --- apport-2.20.1/apport/ui.py +++ apport-2.20.1/apport/ui.py @@ -13,7 +13,7 @@ # option) any later version. See http://www.gnu.org/copyleft/gpl.html for # the full text of the license. -import glob, sys, os.path, optparse, traceback, locale, gettext, re +import glob, sys, os.path, optparse, traceback, locale, gettext, re, io import errno, zlib import subprocess, threading, webbrowser import signal @@ -223,8 +223,11 @@ logind_session = None else: reports = apport.fileutils.get_new_reports() - proc_pid_fd = os.open('/proc/%s' % os.getpid(), os.O_RDONLY | os.O_PATH | os.O_DIRECTORY) - logind_session = apport.Report.get_logind_session(proc_pid_fd) + if PY3: + proc_pid_fd = os.open('/proc/%s' % os.getpid(), os.O_RDONLY | os.O_PATH | os.O_DIRECTORY) + logind_session = apport.Report.get_logind_session(proc_pid_fd=proc_pid_fd) + else: + logind_session = apport.Report.get_logind_session(os.getpid()) for f in reports: if not self.load_report(f): @@ -424,15 +427,20 @@ # if PID is given, add info if self.options.pid: try: - proc_pid_fd = os.open('/proc/%s' % self.options.pid, os.O_RDONLY | os.O_PATH | os.O_DIRECTORY) - with open('stat', opener=lambda path, mode: os.open(path, mode, dir_fd=proc_pid_fd)) as f: + proc_pid_fd = None + if PY3: + proc_pid_fd = os.open('/proc/%s' % self.options.pid, os.O_RDONLY | os.O_PATH | os.O_DIRECTORY) + stat_file = os.open('stat', os.O_RDONLY | os.O_CLOEXEC, dir_fd=proc_pid_fd) + else: + stat_file = '/proc/%s/stat' % self.options.pid + with io.open(stat_file) as f: stat = f.read().split() flags = int(stat[8]) if flags & PF_KTHREAD: # this PID is a kernel thread self.options.package = 'linux' else: - self.report.add_proc_info(proc_pid_fd=proc_pid_fd) + self.report.add_proc_info(pid=self.options.pid, proc_pid_fd=proc_pid_fd) except (ValueError, IOError, OSError) as e: if hasattr(e, 'errno'): # silently ignore nonexisting PIDs; the user must not close the diff -u apport-2.20.1/data/apport apport-2.20.1/data/apport --- apport-2.20.1/data/apport +++ apport-2.20.1/data/apport @@ -32,18 +32,10 @@ This avoids bringing down the system to its knees if there is a series of crashes.''' - # create lock file directory - try: - os.mkdir("/var/lock/apport", mode=0o744) - except OSError as e: - if e.errno == errno.EEXIST: - pass - else: - raise - # create a lock file try: - fd = os.open("/var/lock/apport/lock", os.O_WRONLY | os.O_CREAT | os.O_NOFOLLOW) + fd = os.open("/var/run/apport.lock", + os.O_WRONLY | os.O_CREAT | os.O_NOFOLLOW, mode=0o600) except OSError as e: error_log('cannot create lock file (uid %i): %s' % (os.getuid(), str(e))) sys.exit(1) @@ -206,7 +198,7 @@ if limit > 0 and core_size > limit: error_log('aborting core dump writing, size %i exceeds current limit' % core_size) os.close(core_file) - os.unlink(core_path) + os.unlink(core_path, dir_fd=cwd) return error_log('writing core dump %s of size %i' % (core_path, core_size)) os.write(core_file, r['CoreDump']) @@ -222,17 +214,16 @@ if limit > 0 and written > limit: error_log('aborting core dump writing, size exceeds current limit %i' % limit) os.close(core_file) - os.unlink(core_path) + os.unlink(core_path, dir_fd=cwd) return if os.write(core_file, block) != size: error_log('aborting core dump writing, could not write') os.close(core_file) - os.unlink(core_path) + os.unlink(core_path, dir_fd=cwd) return block = os.read(0, 1048576) os.close(core_file) - return core_path def usable_ram(): @@ -381,9 +372,9 @@ sys.exit(1) # Normal startup -if len(sys.argv) not in (5, 6): +if len(sys.argv) not in (5, 6, 7): try: - print('Usage: %s [global pid]' % sys.argv[0]) + print('Usage: %s [global pid] [exe path]' % sys.argv[0]) print('The core dump is read from stdin.') except IOError: # sys.stderr might not actually exist, expecially not when being called @@ -397,7 +388,7 @@ # then compare it with the local PID. If they don't match, it's an # indication that the crash originated from another PID namespace. # Simply log an entry in the host error log and exit 0. -if len(sys.argv) == 6: +if len(sys.argv) >= 6: host_pid = int(sys.argv[5]) if not is_same_ns(host_pid, "pid") and not is_same_ns(host_pid, "mnt"): @@ -485,7 +476,7 @@ sys.exit(0) # Send all arguments except for the first (exec path) and last (global pid) - args = ' '.join(sys.argv[1:-1]) + args = ' '.join(sys.argv[1:5]) try: sock.sendmsg([args.encode()], [ # Send a ucred containing the global pid @@ -570,6 +561,11 @@ # We already need this here to figure out the ExecutableName (for scripts, # etc). + if len(sys.argv) >= 7: + exec_path = sys.argv[6].replace('!', '/') + if os.path.exists(exec_path): + info['ExecutablePath'] = exec_path + euid = os.geteuid() egid = os.getegid() try: @@ -668,15 +664,16 @@ mode = 0o640 else: mode = 0 - reportfile = os.fdopen(os.open(report, os.O_RDWR | os.O_CREAT | os.O_EXCL, mode), 'w+b') + fd = os.open(report, os.O_RDWR | os.O_CREAT | os.O_EXCL, mode) + reportfile = os.fdopen(fd, 'w+b') assert reportfile.fileno() > sys.stderr.fileno() # Make sure the crash reporting daemon can read this report try: gid = pwd.getpwnam('whoopsie').pw_gid - os.chown(report, pidstat.st_uid, gid) + os.fchown(fd, pidstat.st_uid, gid) except (OSError, KeyError): - os.chown(report, pidstat.st_uid, pidstat.st_gid) + os.fchown(fd, pidstat.st_uid, pidstat.st_gid) except (OSError, IOError) as e: error_log('Could not create report file: %s' % str(e)) sys.exit(1) diff -u apport-2.20.1/debian/changelog apport-2.20.1/debian/changelog --- apport-2.20.1/debian/changelog +++ apport-2.20.1/debian/changelog @@ -1,3 +1,52 @@ +apport (2.20.1-0ubuntu2.23) xenial-security; urgency=medium + + * SECURITY UPDATE: World writable root owned lock file created in user + controllable location (LP: #1862348) + - data/apport: Change location of lock file to be directly under + /var/run so that regular users can not directly access it or perform + symlink attacks. + - CVE-2020-8831 + * SECURITY UPDATE: Race condition between report creation and ownership + (LP: #1862933) + - data/apport: When setting owner of report file use a file-descriptor + to the report file instead of its path name to ensure that users can + not cause Apport to change the ownership of other files via a + symlink attack. + - CVE-2020-8833 + + -- Alex Murray Wed, 25 Mar 2020 11:50:41 +1030 + +apport (2.20.1-0ubuntu2.22) xenial-security; urgency=medium + + [ Michael Hudson-Doyle ] + * SECURITY REGRESSION: fix autopkgtest failures since recent security + update (LP: #1854237) + - Fix regression in creating report for crashing setuid process by getting + kernel to tell us the executable path rather than reading + /proc/[pid]/exe. + - Fix deletion of partially written core files. + - Fix test_get_logind_session to use new API. + - Restore add_proc_info raising ValueError for a dead process. + - Delete test_lock_symlink, no longer applicable now that the lock is + created in a directory only root can write to. + + [ Tiago Stürmer Daitx ] + * SECURITY REGRESSION: 'module' object has no attribute 'O_PATH' + (LP: #1851806) + - apport/report.py, apport/ui.py: use file descriptors for /proc/pid + directory access only when running under python 3; prevent reading /proc + maps under python 2 as it does not provide a secure way to do so; use + io.open for better compatibility between python 2 and 3. + * data/apport: fix number of arguments passed through socks into a container. + * test/test_report.py: test login session with both pid and proc_pid_fd. + * test/test_apport_valgrind.py: skip test_sandbox_cache_options if system + has little memory. + * test/test_ui.py: modify run_crash_kernel test to account for the fact that + linux-image-$kvers-$flavor is now built from the linux-signed source + package on amd64 and ppc64el. (LP: #1766740) + + -- Tiago Stürmer Daitx Thu, 27 Feb 2020 03:18:45 +0000 + apport (2.20.1-0ubuntu2.21) xenial-security; urgency=medium * SECURITY REGRESSION: missing argument in Report.add_proc_environ diff -u apport-2.20.1/debian/tests/control apport-2.20.1/debian/tests/control --- apport-2.20.1/debian/tests/control +++ apport-2.20.1/debian/tests/control @@ -4,6 +4,7 @@ apport-valgrind, apport-gtk, apport-kde, + python-apport, build-essential, python-twisted-core, python3-mock, diff -u apport-2.20.1/etc/init.d/apport apport-2.20.1/etc/init.d/apport --- apport-2.20.1/etc/init.d/apport +++ apport-2.20.1/etc/init.d/apport @@ -50,7 +50,7 @@ rm -f /var/lib/pm-utils/resume-hang.log fi - echo "|$AGENT %p %s %c %d %P" > /proc/sys/kernel/core_pattern + echo "|$AGENT %p %s %c %d %P %E" > /proc/sys/kernel/core_pattern echo 2 > /proc/sys/fs/suid_dumpable } diff -u apport-2.20.1/test/test_report.py apport-2.20.1/test/test_report.py --- apport-2.20.1/test/test_report.py +++ apport-2.20.1/test/test_report.py @@ -2228,7 +2228,28 @@ (session, timestamp) = ret self.assertNotEqual(session, '') - # session start must be >= 2014-01-01 and "now" + # session start must be >= 2014-01-01 and <= "now" + self.assertLess(timestamp, time.time()) + self.assertGreater(timestamp, + time.mktime(time.strptime('2014-01-01', '%Y-%m-%d'))) + + def test_get_logind_session_fd(self): + proc_pid_fd = os.open('/proc/%s' % os.getpid(), os.O_RDONLY | os.O_PATH | os.O_DIRECTORY) + self.addCleanup(os.close, proc_pid_fd) + ret = apport.Report.get_logind_session(proc_pid_fd=proc_pid_fd) + if ret is None: + # ensure that we don't run under logind, and thus the None is + # justified + with open('/proc/self/cgroup') as f: + contents = f.read() + sys.stdout.write('[not running under logind] ') + sys.stdout.flush() + self.assertNotIn('name=systemd:/user', contents) + return + + (session, timestamp) = ret + self.assertNotEqual(session, '') + # session start must be >= 2014-01-01 and <= "now" self.assertLess(timestamp, time.time()) self.assertGreater(timestamp, time.mktime(time.strptime('2014-01-01', '%Y-%m-%d'))) diff -u apport-2.20.1/test/test_signal_crashes.py apport-2.20.1/test/test_signal_crashes.py --- apport-2.20.1/test/test_signal_crashes.py +++ apport-2.20.1/test/test_signal_crashes.py @@ -190,34 +190,6 @@ os.kill(test_proc2, 9) os.waitpid(test_proc2, 0) - def test_lock_symlink(self): - '''existing .lock file as dangling symlink does not create the file - - This would be a vulnerability, as users could overwrite system files. - ''' - # prepare a symlink trap - lockpath = os.path.join(self.report_dir, '.lock') - trappath = os.path.join(self.report_dir, '0wned') - os.symlink(trappath, lockpath) - - # now call apport - test_proc = self.create_test_process() - - try: - app = subprocess.Popen([apport_path, str(test_proc), '42', '0', '1'], - stdin=subprocess.PIPE, stderr=subprocess.PIPE) - app.stdin.write(b'boo') - app.stdin.close() - - self.assertNotEqual(app.wait(), 0, app.stderr.read()) - app.stderr.close() - finally: - os.kill(test_proc, 9) - os.waitpid(test_proc, 0) - - self.assertEqual(self.get_temp_all_reports(), []) - self.assertFalse(os.path.exists(trappath)) - def test_unpackaged_binary(self): '''unpackaged binaries do not create a report''' diff -u apport-2.20.1/test/test_ui.py apport-2.20.1/test/test_ui.py --- apport-2.20.1/test/test_ui.py +++ apport-2.20.1/test/test_ui.py @@ -1,5 +1,5 @@ # coding: UTF-8 -import unittest, shutil, signal, tempfile, resource, pwd, time, os, sys +import unittest, shutil, signal, tempfile, resource, pwd, time, os, sys, imp import subprocess, errno, glob try: @@ -16,6 +16,11 @@ import apport.crashdb_impl.memory import stat +if os.environ.get('APPORT_TEST_LOCAL'): + impl = imp.load_source('', 'backends/packaging-apt-dpkg.py').impl +else: + from apport.packaging_impl import impl + logind_session = apport.Report.get_logind_session(os.getpid()) @@ -1259,8 +1264,14 @@ def test_run_crash_kernel(self): '''run_crash() for a kernel error''' + sys_arch = impl.get_system_architecture() + if sys_arch in ['amd64', 'ppc64el']: + src_pkg = 'linux-signed' + else: + src_pkg = 'linux' + # set up hook - f = open(os.path.join(self.hookdir, 'source_linux.py'), 'w') + f = open(os.path.join(self.hookdir, 'source_%s.py' % src_pkg), 'w') f.write('''def add_info(report, ui): report['KernelDebug'] = 'LotsMoreInfo' ''') @@ -1269,7 +1280,7 @@ # generate crash report r = apport.Report('KernelCrash') r['Package'] = apport.packaging.get_kernel_package() - r['SourcePackage'] = 'linux' + r['SourcePackage'] = src_pkg # write crash report report_file = os.path.join(apport.fileutils.report_dir, 'test.crash') @@ -1302,7 +1313,8 @@ self.assertEqual(self.ui.msg_severity, None, str(self.ui.msg_title) + ' ' + str(self.ui.msg_text)) self.assertEqual(self.ui.msg_title, None) - self.assertEqual(self.ui.opened_url, 'http://linux.bugs.example.com/%i' % self.ui.crashdb.latest_id()) + self.assertEqual(self.ui.opened_url, 'http://%s.bugs.example.com/%i' % + (src_pkg, self.ui.crashdb.latest_id())) self.assertTrue(self.ui.present_details_shown) self.assertTrue('SourcePackage' in self.ui.report.keys()) only in patch2: unchanged: --- apport-2.20.1.orig/test/test_apport_valgrind.py +++ apport-2.20.1/test/test_apport_valgrind.py @@ -17,6 +17,15 @@ p = subprocess.Popen(['which', 'valgrind'], stdout=subprocess.PIPE) p.communicate() have_valgrind = (p.returncode == 0) +with open('/proc/meminfo') as f: + for line in f.readlines(): + if line.startswith('MemTotal'): + memtotal = int(line.split()[1]) + break + if memtotal < 2000000: + low_memory = True + else: + low_memory = False @unittest.skipUnless(have_valgrind, 'valgrind not installed') @@ -133,13 +142,14 @@ log = f.read() self.assertTrue(exepath in log, log) + @unittest.skipIf(low_memory, 'not enough memory') def test_sandbox_cache_options(self): '''apport-valgrind creates a user specified sandbox and cache''' sandbox = os.path.join(self.workdir, 'test-sandbox') cache = os.path.join(self.workdir, 'test-cache') - cmd = ['apport-valgrind', '--sandbox-dir', sandbox, '--cache', cache, 'true'] + cmd = ['apport-valgrind', '-v', '--sandbox-dir', sandbox, '--cache', cache, 'true'] subprocess.check_call(cmd) self.assertTrue(os.path.exists(sandbox),