diff -u apport-2.0.1/data/apport apport-2.0.1/data/apport --- apport-2.0.1/data/apport +++ apport-2.0.1/data/apport @@ -44,28 +44,51 @@ error_log('another apport instance is already running, aborting') sys.exit(1) -def drop_privileges(pid, partial=False): - '''Change user and group to match the given target process.''' +(pidstat, real_uid, real_gid, cwd) = (None, None, None, None) - stat = None - try: - stat = os.stat('/proc/%s/stat' % pid) - except OSError as e: - raise ValueError('Invalid process ID: ' + str(e)) - - if partial: - effective_gid = os.getegid() - effective_uid = os.geteuid() - else: - effective_gid = stat.st_gid - effective_uid = stat.st_uid - os.setregid(stat.st_gid, effective_gid) - os.setreuid(stat.st_uid, effective_uid) - assert os.getegid() == effective_gid - assert os.getgid() == stat.st_gid - assert os.geteuid() == effective_uid - assert os.getuid() == stat.st_uid +def get_pid_info(pid): + '''Read /proc information about pid''' + + global pidstat, real_uid, real_gid, cwd + + # unhandled exceptions on missing or invalidly formatted files are okay + # here -- we want to know in the log file + pidstat = os.stat('/proc/%s/stat' % pid) + + # determine real UID of the target process; do *not* use the owner of + # /proc/pid/stat, as that will be root for setuid or unreadable programs! + # (this matters when suid_dumpable is enabled) + with open('/proc/%s/status' % pid) as f: + for line in f: + if line.startswith('Uid:'): + real_uid = int(line.split()[1]) + elif line.startswith('Gid:'): + real_gid = int(line.split()[1]) + break + assert real_uid is not None, 'failed to parse Uid' + assert real_gid is not None, 'failed to parse Gid' + + cwd = os.readlink('/proc/' + pid + '/cwd') + + +def drop_privileges(pid, real_only=False): + '''Change user and group to match the given target process + + Normally that irrevocably drops privileges to the real user/group of the + target process. With real_only=True only the real IDs are changed, but + the effective IDs remain. + ''' + if real_only: + os.setregid(real_gid, -1) + os.setreuid(real_uid, -1) + else: + os.setgid(real_gid) + os.setuid(real_uid) + assert os.getegid() == real_gid + assert os.geteuid() == real_uid + assert os.getgid() == real_gid + assert os.getuid() == real_uid def init_error_log(): '''Open a suitable error log if sys.stderr is not a tty.''' @@ -137,6 +160,16 @@ # ulimit specifies kB limit *= 1024 + # don't write a core dump for suid/sgid/unreadable or otherwise + # protected executables, in accordance with core(5) + # (suid_dumpable==2 and core_pattern restrictions); when this happens, + # /proc/pid/stat is owned by root (or the user suid'ed to), but we already + # changed to the crashed process' real uid + assert pidstat, 'pidstat not initialized' + if pidstat.st_uid != os.getuid() or pidstat.st_gid != os.getgid(): + error_log('disabling core dump for suid/sgid/unreadable executable') + return + core_path = os.path.join(cwd, 'core') try: if open('/proc/sys/kernel/core_uses_pid').read().strip() != '0': @@ -203,7 +236,7 @@ return False orig_uid = os.geteuid() - os.setresuid(uid, uid, -1) + os.setresuid(-1, os.getuid(), -1) try: gdbus = subprocess.Popen(['/usr/bin/gdbus', 'call', '-e', '-d', 'org.gnome.SessionManager', '-o', '/org/gnome/SessionManager', '-m', @@ -216,7 +249,7 @@ error_log('gdbus call failed, cannot determine running session: ' + str(e)) return False finally: - os.setresuid(orig_uid, orig_uid, -1) + os.setresuid(-1, orig_uid, -1) error_log('debug: session gdbus call: ' + out.decode('UTF-8')) if out.startswith(b'(false,'): return True @@ -254,25 +287,17 @@ except OSError: pass # *shrug*, we tried + get_pid_info(pid) + # Partially drop privs to gain proper os.access() checks drop_privileges(pid, True) - # try to find the core dump file; if path is relative, prepend cwd of - # crashed process - cwd = os.readlink('/proc/' + pid + '/cwd') - # ignore SIGQUIT (it's usually deliberately generated by users) if signum == str(signal.SIGQUIT): sys.exit(0) error_log('called for pid %s, signal %s' % (pid, signum)) - try: - pidstat = os.stat('/proc/%s/stat' % pid) - except OSError: - error_log('Invalid PID') - sys.exit(1) - # check if the executable was modified after the process started (e. g. # package got upgraded in between) exe_mtime = os.stat('/proc/%s/exe' % pid).st_mtime @@ -351,8 +376,14 @@ drop_privileges(pid) write_user_coredump(pid, cwd, core_ulimit) sys.exit(1) - reportfile = os.fdopen(os.open(report, - os.O_WRONLY|os.O_CREAT|os.O_EXCL, 0), 'w') + # we prefer having a file mode of 0 while writing; this doesn't work + # for suid binaries as we completely drop privs and thus can't chmod + # them later on + if pidstat.st_uid != os.getuid(): + mode = 0o640 + else: + mode = 0 + reportfile = os.fdopen(os.open(report, os.O_WRONLY | os.O_CREAT | os.O_EXCL, mode), 'wb') assert reportfile.fileno() > sys.stderr.fileno() # Make sure the crash reporting daemon can read this report @@ -387,7 +418,9 @@ if reportfile != sys.stderr: os.unlink(report) raise - if report: + if report and mode == 0: + # for non-suid programs, make the report writable now, when it's + # completely written os.chmod(report, 0o640) if reportfile != sys.stderr: error_log('wrote report %s' % report) diff -u apport-2.0.1/debian/changelog apport-2.0.1/debian/changelog --- apport-2.0.1/debian/changelog +++ apport-2.0.1/debian/changelog @@ -1,3 +1,23 @@ +apport (2.0.1-0ubuntu17.9) precise-security; urgency=medium + + * SECURITY UPDATE: When /proc/sys/fs/suid_dumpable is enabled, crashing a + program that is suid root or not readable for the user would create + root-owned core files in the current directory of that program. Creating + specially crafted core files in /etc/logrotate.d or similar could then + lead to arbitrary code execution with root privileges. Now core files do + not get written for these kinds of programs, in accordance with the + intention of core(5). + Thanks to Sander Bos for discovering this issue! + (CVE-2015-1324, LP: #1452239) + * Add test case to ensure that users cannot inject arbitrary core dump file + contents (CVE-2015-1325). This version is not affected, but having the + test will ensure that backported changes don't introduce this + vulnerability. (LP: #1453900) + * test_signal_crashes(): Drop hardcoded /tmp/ path in do_crash(), + test_nonwritable_cwd() uses a different dir. + + -- Martin Pitt Wed, 13 May 2015 13:58:17 +0200 + apport (2.0.1-0ubuntu17.8) precise-proposed; urgency=medium [ Martin Pitt ] diff -u apport-2.0.1/test/test_signal_crashes.py apport-2.0.1/test/test_signal_crashes.py --- apport-2.0.1/test/test_signal_crashes.py +++ apport-2.0.1/test/test_signal_crashes.py @@ -256,16 +256,54 @@ self.assertGreater(count, 1, 'gets at least 2 repeated crashes') self.assertLess(count, 7, 'stops flooding after less than 7 repeated crashes') + @unittest.skipIf(os.access('/run', os.W_OK), 'this test needs to be run as user') def test_nonwritable_cwd(self): '''core dump works for non-writable cwd''' - os.chdir('/') + os.chdir('/run') + resource.setrlimit(resource.RLIMIT_CORE, (-1, -1)) self.do_crash() pr = apport.Report() self.assertTrue(os.path.exists(self.test_report)) + self.assertFalse(os.path.exists('/run/core')) pr.load(open(self.test_report)) assert set(required_fields).issubset(set(pr.keys())) + @unittest.skipIf(os.access('/run', os.W_OK), 'this test needs to be run as user') + def test_nonwritable_cwd_nonreadable_exe(self): + '''no core file for non-readable exe in non-writable cwd''' + + # CVE-2015-1324: if a user cannot read an executable, it behaves much + # like a suid root binary in terms of writing a core dump + + # create a non-readable executable in a path we can modify which apport + # regards as likely packaged + (fd, myexe) = tempfile.mkstemp(dir='/var/tmp') + self.addCleanup(os.unlink, myexe) + with open(test_executable, 'rb') as f: + os.write(fd, f.read()) + os.close(fd) + os.chmod(myexe, 0o111) + + os.chdir('/run') + resource.setrlimit(resource.RLIMIT_CORE, (-1, -1)) + + self.do_crash(True, command=myexe, expect_corefile=False) + + # check crash report + reports = apport.fileutils.get_new_system_reports() + self.assertEqual(len(reports), 1) + report = reports[0] + st = os.stat(report) + # FIXME: we would like to clean up this, but don't have privileges for that + # os.unlink(report) + self.assertEqual(stat.S_IMODE(st.st_mode), 0o640, 'report has correct permissions') + # this must be owned by root as it is an unreadable binary + self.assertEqual(st.st_uid, 0, 'report has correct owner') + + # no user reports + self.assertEqual(apport.fileutils.get_all_reports(), []) + def test_core_dump_packaged(self): '''packaged executables create core dumps on proper ulimits''' @@ -339,6 +377,48 @@ self.do_crash(expect_corefile=True, command=local_exe, sig=signal.SIGABRT) self.assertEqual(apport.fileutils.get_all_reports(), []) + def test_core_file_injection(self): + '''cannot inject core file''' + + # CVE-2015-1325: ensure that apport does not re-open its .crash report, + # as that allows us to intercept and replace the report and tinker with + # the core dump + + with open(self.test_report + '.inject', 'w') as f: + # \x01pwned + f.write('''ProblemType: Crash +CoreDump: base64 + H4sICAAAAAAC/0NvcmVEdW1wAA== + Yywoz0tNAQBl1rhlBgAAAA== +''') + + # crash our test process and let it write a core file + resource.setrlimit(resource.RLIMIT_CORE, (-1, -1)) + pid = self.create_test_process() + os.kill(pid, signal.SIGSEGV) + + # replace report with the crafted one above as soon as it exists and + # becomes deletable for us; this is a busy loop, we need to be really + # fast to intercept + while True: + try: + os.unlink(self.test_report) + break + except OSError: + pass + os.rename(self.test_report + '.inject', self.test_report) + + os.waitpid(pid, 0) + time.sleep(0.5) + + # verify that we get the original core, not the injected one + with open('core', 'rb') as f: + core = f.read() + self.assertNotIn(b'pwned', core) + self.assertGreater(len(core), 10000) + + os.unlink('core') + def test_limit_size(self): '''core dumps are capped on available memory size''' @@ -447,7 +527,7 @@ @unittest.skipIf(os.geteuid() != 0, 'this test needs to be run as root') def test_crash_setuid_keep(self): - '''report generation and core dump for setuid program which stays root''' + '''report generation for setuid program which stays root''' # create suid root executable in a path we can modify which apport # regards as likely packaged @@ -460,9 +540,8 @@ # run test program as user "mail" resource.setrlimit(resource.RLIMIT_CORE, (-1, -1)) - # expect the core file to be owned by root - self.do_crash(command=myexe, expect_corefile=True, uid=8, - expect_corefile_owner=0) + # if a user can crash a suid root binary, it should not create core files + self.do_crash(command=myexe, uid=8) # check crash report reports = apport.fileutils.get_all_reports() @@ -477,14 +556,12 @@ @unittest.skipUnless(os.path.exists('/bin/ping'), 'this test needs /bin/ping') @unittest.skipIf(os.geteuid() != 0, 'this test needs to be run as root') def test_crash_setuid_drop(self): - '''report generation and core dump for setuid program which drops root''' + '''report generation for setuid program which drops root''' # run ping as user "mail" resource.setrlimit(resource.RLIMIT_CORE, (-1, -1)) - # expect the core file to be owned by root - self.do_crash(command='/bin/ping', args=['127.0.0.1'], - expect_corefile=True, uid=8, - expect_corefile_owner=0) + # if a user can crash a suid root binary, it should not create core files + self.do_crash(command='/bin/ping', args=['127.0.0.1'], uid=8) # check crash report reports = apport.fileutils.get_all_reports() @@ -496,6 +573,59 @@ # this must not be owned by root as it is a setuid binary self.assertEqual(st.st_uid, 0, 'report has correct owner') + @unittest.skipIf(os.geteuid() != 0, 'this test needs to be run as root') + def test_crash_setuid_unpackaged(self): + '''report generation for unpackaged setuid program''' + + # create suid root executable in a path we can modify which apport + # regards as not packaged + (fd, myexe) = tempfile.mkstemp(dir='/tmp') + self.addCleanup(os.unlink, myexe) + with open(test_executable, 'rb') as f: + os.write(fd, f.read()) + os.close(fd) + os.chmod(myexe, 0o4755) + + # run test program as user "mail" + resource.setrlimit(resource.RLIMIT_CORE, (-1, -1)) + + # if a user can crash a suid root binary, it should not create core files + self.do_crash(command=myexe, expect_corefile=False, uid=8) + + # there should not be a crash report + self.assertEqual(apport.fileutils.get_all_reports(), []) + + @unittest.skipIf(os.geteuid() != 0, 'this test needs to be run as root') + def test_crash_setuid_nonwritable_cwd(self): + '''report generation and core dump for setuid program, non-writable cwd''' + + # create suid root executable in a path we can modify which apport + # regards as likely packaged + (fd, myexe) = tempfile.mkstemp(dir='/var/tmp') + self.addCleanup(os.unlink, myexe) + with open(test_executable, 'rb') as f: + os.write(fd, f.read()) + os.close(fd) + os.chmod(myexe, 0o4755) + + # run test program as user "mail" in /run (which should only be + # writable to root) + os.chdir('/run') + resource.setrlimit(resource.RLIMIT_CORE, (-1, -1)) + + # we expect a report, but no core file + self.do_crash(command=myexe, expect_corefile=False, uid=8) + + # check crash report + reports = apport.fileutils.get_all_reports() + self.assertEqual(len(reports), 1) + report = reports[0] + st = os.stat(report) + os.unlink(report) + self.assertEqual(stat.S_IMODE(st.st_mode), 0o640, 'report has correct permissions') + # this must be owned by root as it is a setuid binary + self.assertEqual(st.st_uid, 0, 'report has correct owner') + # # Helper methods # @@ -532,7 +662,7 @@ expect_corefile_owner=None, args=[]): '''Generate a test crash. - This runs command (by default test_executable) in /tmp, lets it crash, + This runs command (by default test_executable) in cwd, lets it crash, and checks that it exits with the expected return code, leaving a core file behind if expect_corefile is set, and generating a crash report if expect_coredump is set. @@ -540,7 +670,7 @@ If check_running is set (default), this will abort if test_process is already running. ''' - self.assertFalse(os.path.exists('core'), '/tmp/core already exists, please clean up first') + self.assertFalse(os.path.exists('core'), '%s/core already exists, please clean up first' % os.getcwd()) pid = self.create_test_process(check_running, command, uid=uid, args=args) if sleep > 0: time.sleep(sleep) @@ -565,26 +695,26 @@ 'no running test executable processes') if expect_corefile: - self.assertTrue(os.path.exists('/tmp/core'), 'leaves wanted core file') + self.assertTrue(os.path.exists('core'), 'leaves wanted core file') try: # check core file permissions - st = os.stat('/tmp/core') + st = os.stat('core') self.assertEqual(stat.S_IMODE(st.st_mode), 0o600, 'core file has correct permissions') if expect_corefile_owner is not None: self.assertEqual(st.st_uid, expect_corefile_owner, 'core file has correct owner') # check that core file is valid gdb = subprocess.Popen(['gdb', '--batch', '--ex', 'bt', - command, '/tmp/core'], stdout=subprocess.PIPE, + command, 'core'], stdout=subprocess.PIPE, stderr=subprocess.PIPE) (out, err) = gdb.communicate() self.assertEqual(gdb.returncode, 0) err = err.strip() self.assertTrue(err == '' or err.startswith('warning'), err) finally: - os.unlink('/tmp/core') + os.unlink('core') else: - self.assertFalse(os.path.exists('/tmp/core'), 'leaves unexpected core file behind') + self.assertFalse(os.path.exists('core'), 'leaves unexpected core file behind') def get_temp_all_reports(self): '''Call apport.fileutils.get_all_reports() for our temp dir'''