=== modified file 'data/apport' --- old/data/apport 2012-07-12 15:11:48 +0000 +++ new/data/apport 2013-10-23 15:49:55 +0000 @@ -51,7 +51,7 @@ stat = None try: - stat = os.stat('/proc/' + pid) + stat = os.stat('/proc/%s/stat' % pid) except OSError as e: raise ValueError('Invalid process ID: ' + str(e)) @@ -145,7 +145,7 @@ with open('/proc/sys/kernel/core_uses_pid') as f: if f.read().strip() != '0': core_path += '.' + str(pid) - core_file = os.open(core_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o640) + core_file = os.open(core_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) except (OSError, IOError): return @@ -300,7 +300,7 @@ core_ulimit = -1 try: - pidstat = os.stat('/proc/' + pid) + pidstat = os.stat('/proc/%s/stat' % pid) except OSError: error_log('Invalid PID') sys.exit(1) === modified file 'test/test_signal_crashes.py' --- old/test/test_signal_crashes.py 2012-07-12 15:11:48 +0000 +++ new/test/test_signal_crashes.py 2013-10-23 15:51:50 +0000 @@ -289,12 +289,12 @@ self.check_report_coredump(self.test_report) apport.fileutils.delete_report(self.test_report) resource.setrlimit(resource.RLIMIT_CORE, (10000, -1)) - self.do_crash(expect_corefile=True) + self.do_crash(expect_corefile=True, expect_corefile_owner=os.geteuid()) self.assertEqual(apport.fileutils.get_all_reports(), [self.test_report]) self.check_report_coredump(self.test_report) apport.fileutils.delete_report(self.test_report) resource.setrlimit(resource.RLIMIT_CORE, (-1, -1)) - self.do_crash(expect_corefile=True) + self.do_crash(expect_corefile=True, expect_corefile_owner=os.geteuid()) self.assertEqual(apport.fileutils.get_all_reports(), [self.test_report]) self.check_report_coredump(self.test_report) apport.fileutils.delete_report(self.test_report) @@ -308,11 +308,11 @@ self.assertEqual(apport.fileutils.get_all_reports(), [self.test_report]) apport.fileutils.delete_report(self.test_report) resource.setrlimit(resource.RLIMIT_CORE, (10000, -1)) - self.do_crash(expect_corefile=True, sig=signal.SIGABRT) + self.do_crash(expect_corefile=True, sig=signal.SIGABRT, expect_corefile_owner=os.geteuid()) self.assertEqual(apport.fileutils.get_all_reports(), [self.test_report]) apport.fileutils.delete_report(self.test_report) resource.setrlimit(resource.RLIMIT_CORE, (-1, -1)) - self.do_crash(expect_corefile=True, sig=signal.SIGABRT) + self.do_crash(expect_corefile=True, sig=signal.SIGABRT, expect_corefile_owner=os.geteuid()) self.assertEqual(apport.fileutils.get_all_reports(), [self.test_report]) # creates core file with existing crash report, too @@ -534,12 +534,64 @@ self.assertEqual(pr['ExecutablePath'], test_executable) self.assertEqual(pr['CoreDump'], b'hel\x01lo') + @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''' + + # 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" + 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) + + # 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 not be owned by root as it is a setuid binary + self.assertEqual(st.st_uid, 0, 'report has correct owner') + + @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''' + + # 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) + + # 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 not be owned by root as it is a setuid binary + self.assertEqual(st.st_uid, 0, 'report has correct owner') + # # Helper methods # @classmethod - def create_test_process(klass, check_running=True, command=test_executable): + def create_test_process(klass, check_running=True, command=test_executable, + uid=None, args=[]): '''Spawn test_executable. Wait until it is fully running, and return its PID. @@ -549,10 +601,12 @@ assert subprocess.call(['pidof', command]) == 1, 'no running test executable processes' pid = os.fork() if pid == 0: + if uid is not None: + os.setuid(uid) os.dup2(os.open('/dev/null', os.O_WRONLY), sys.stdout.fileno()) sys.stdin.close() os.setsid() - os.execv(command, [command]) + os.execv(command, [command] + args) assert False, 'Could not execute ' + command # wait until child process has execv()ed properly @@ -569,7 +623,8 @@ def do_crash(self, expect_coredump=True, expect_corefile=False, sig=signal.SIGSEGV, check_running=True, sleep=0, - command=test_executable): + command=test_executable, uid=None, + expect_corefile_owner=None, args=[]): '''Generate a test crash. This runs command (by default test_executable) in /tmp, lets it crash, @@ -581,7 +636,7 @@ already running. ''' self.assertFalse(os.path.exists('core'), '/tmp/core already exists, please clean up first') - pid = self.create_test_process(check_running, command) + pid = self.create_test_process(check_running, command, uid=uid, args=args) if sleep > 0: time.sleep(sleep) os.kill(pid, sig) @@ -609,6 +664,12 @@ if expect_corefile: self.assertTrue(os.path.exists('/tmp/core'), 'leaves wanted core file') try: + # check core file permissions + st = os.stat('/tmp/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'],