diff -u apport-2.14.1/data/iwlwifi_error_dump apport-2.14.1/data/iwlwifi_error_dump --- apport-2.14.1/data/iwlwifi_error_dump +++ apport-2.14.1/data/iwlwifi_error_dump @@ -56,7 +56,7 @@ pass try: - with open(apport.fileutils.make_report_path(pr), 'wb') as f: + with apport.fileutils.make_report_file(pr) as f: pr.write(f) except IOError as e: apport.fatal('Cannot create report: ' + str(e)) diff -u apport-2.14.1/data/kernel_crashdump apport-2.14.1/data/kernel_crashdump --- apport-2.14.1/data/kernel_crashdump +++ apport-2.14.1/data/kernel_crashdump @@ -11,12 +11,9 @@ # option) any later version. See http://www.gnu.org/copyleft/gpl.html for # the full text of the license. -import os, re +import os, re, glob import apport, apport.fileutils -vmcore_root = os.path.join(apport.fileutils.report_dir) -vmcore_path = os.path.join(vmcore_root, 'vmcore') - pr = apport.Report('KernelCrash') package = apport.packaging.get_kernel_package() try: @@ -30,38 +27,45 @@ + +vmcore_path = os.path.join(apport.fileutils.report_dir, 'vmcore') +# only accept plain files here, not symlinks; otherwise we might recursively +# include the report, or similar DoS attacks if os.path.exists(vmcore_path + '.log'): - pr['VmCoreLog'] = (vmcore_path + '.log',) + try: + log_fd = os.open(vmcore_path + '.log', os.O_RDONLY | os.O_NOFOLLOW) + pr['VmCoreLog'] = (os.fdopen(log_fd, 'rb'),) + os.unlink(vmcore_path + '.log') + except OSError as e: + apport.fatal('Cannot open vmcore log: ' + str(e)) if os.path.exists(vmcore_path): - pr['VmCore'] = (vmcore_path,) try: - with open(apport.fileutils.make_report_path(pr), 'wb') as f: + core_fd = os.open(vmcore_path, os.O_RDONLY | os.O_NOFOLLOW) + pr['VmCore'] = (os.fdopen(core_fd, 'rb'),) + with apport.fileutils.make_report_file(pr) as f: pr.write(f) - except IOError as e: + except (IOError, OSError) as e: apport.fatal('Cannot create report: ' + str(e)) -else: -# kdump-tools has moved vmcore to timestamped dir - for root, dirs, files in os.walk(vmcore_root): - for timedir in dirs: - if re.search('^[0-9]{12}$', timedir): - vmcore_dir = os.path.join(vmcore_root, timedir) - dmesgfile = os.path.join(vmcore_dir, 'dmesg.' + timedir) - report_name = pr['Package'] + '-' + timedir + '.crash' - crash_report = os.path.join(vmcore_root, report_name) - pr['VmCoreDmesg'] = (dmesgfile,) - if not os.path.exists(crash_report): - try: - with open(crash_report, 'wb') as f: - pr.write(f) - except IOError as e: - apport.fatal('Cannot create report: ' + str(e)) - -# clean up the core file -# if not generated by kdump-tools -if os.path.exists(vmcore_path): try: os.unlink(vmcore_path) except OSError: pass # huh, already gone? - try: - os.unlink(vmcore_path + '.log') - except OSError: - pass +else: + # check for kdump-tools generated dmesg in timestamped dir + for dmesg_file in glob.glob(os.path.join(apport.fileutils.report_dir, '*', 'dmesg.*')): + timedir = os.path.dirname(dmesg_file) + timestamp = os.path.basename(timedir) + if re.match('^[0-9]{12}$', timestamp): + # we require the containing dir to be owned by root, to avoid users + # creating a symlink to someplace else and disclosing data; we just + # compare against euid here so that we can test this as non-root + if os.lstat(timedir).st_uid != os.geteuid(): + apport.fatal('%s has unsafe permissions, ignoring' % timedir) + report_name = package + '-' + timestamp + '.crash' + try: + crash_report = os.path.join(apport.fileutils.report_dir, report_name) + dmesg_fd = os.open(dmesg_file, os.O_RDONLY | os.O_NOFOLLOW) + pr['VmCoreDmesg'] = (os.fdopen(dmesg_fd, 'rb'),) + # TODO: Replace with open(..., 'xb') once we drop Python 2 support + with os.fdopen(os.open(crash_report, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o640), 'wb') as f: + pr.write(f) + except (IOError, OSError) as e: + apport.fatal('Cannot create report: ' + str(e)) diff -u apport-2.14.1/data/kernel_oops apport-2.14.1/data/kernel_oops --- apport-2.14.1/data/kernel_oops +++ apport-2.14.1/data/kernel_oops @@ -46,4 +46,2 @@ -report_path = apport.fileutils.make_report_path(pr, uid=checksum) -if not os.path.exists(report_path): - with open(report_path, 'wb') as f: - pr.write(f) +with apport.fileutils.make_report_file(pr, uid=checksum) as f: + pr.write(f) diff -u apport-2.14.1/data/package_hook apport-2.14.1/data/package_hook --- apport-2.14.1/data/package_hook +++ apport-2.14.1/data/package_hook @@ -73,3 +73,3 @@ # write report -with open(apport.fileutils.make_report_path(pr), 'wb') as f: +with apport.fileutils.make_report_file(pr) as f: pr.write(f) diff -u apport-2.14.1/debian/changelog apport-2.14.1/debian/changelog --- apport-2.14.1/debian/changelog +++ apport-2.14.1/debian/changelog @@ -1,3 +1,31 @@ +apport (2.14.1-0ubuntu3.15) trusty-security; urgency=medium + + [ Martin Pitt ] + * SECURITY FIX: kernel_crashdump: Enforce that the log/dmesg files are not a + symlink. + This prevents normal users from pre-creating a symlink to the predictable + .crash file, and thus triggering a "fill up disk" DoS attack when the + .crash report tries to include itself. Also clean up the code to make this + easier to read: Drop the "vmcore_root" alias, move the vmcore and + vmcore.log cleanup into the "no kdump" section, and replace the buggy + os.walk() loop with a glob to only catch direct timestamp subdirectories + of /var/crash/. + Thanks to halfdog for discovering this! + (CVE-2015-1338, part of LP #1492570) + * SECURITY FIX: Fix all writers of report files to open the report file + exclusively. + Fix package_hook, kernel_crashdump, and similar hooks to fail if the + report already exists. This prevents privilege escalation through symlink + attacks. Note that this will also prevent overwriting previous reports + with the same same. Thanks to halfdog for discovering this! + (CVE-2015-1338, LP: #1492570) + + [ Marc Deslauriers ] + * This package does _not_ contain the changes from 2.14.1-0ubuntu3.14 in + trusty-proposed. + + -- Marc Deslauriers Wed, 23 Sep 2015 11:28:26 -0400 + apport (2.14.1-0ubuntu3.13) trusty-proposed; urgency=medium * data/package_hook: when creating the problem report include the version of diff -u apport-2.14.1/test/test_hooks.py apport-2.14.1/test/test_hooks.py --- apport-2.14.1/test/test_hooks.py +++ apport-2.14.1/test/test_hooks.py @@ -205,6 +205,60 @@ r.add_package_info(r['Package']) self.assertTrue(' ' in r['Package']) # appended version number + def test_kernel_crashdump_log_symlink(self): + '''attempted DoS with vmcore.log symlink + + We must only accept plain files, otherwise vmcore.log might be a + symlink to the .crash file, which would recursively fill itself. + ''' + f = open(os.path.join(apport.fileutils.report_dir, 'vmcore'), 'wb') + f.write(b'\x01' * 100) + f.close() + os.symlink('vmcore', os.path.join(apport.fileutils.report_dir, 'vmcore.log')) + + self.assertNotEqual(subprocess.call('%s/kernel_crashdump' % datadir, + stderr=subprocess.PIPE), + 0, 'kernel_crashdump unexpectedly succeeded') + + self.assertEqual(apport.fileutils.get_new_reports(), []) + + def test_kernel_crashdump_kdump_log_symlink(self): + '''attempted DoS with dmesg symlink with kdump-tools''' + + timedir = datetime.strftime(datetime.now(), '%Y%m%d%H%M') + vmcore_dir = os.path.join(apport.fileutils.report_dir, timedir) + os.mkdir(vmcore_dir) + + dmesgfile = os.path.join(vmcore_dir, 'dmesg.' + timedir) + os.symlink('../kernel.crash', dmesgfile) + + self.assertNotEqual(subprocess.call('%s/kernel_crashdump' % datadir, + stderr=subprocess.PIPE), + 0, 'kernel_crashdump unexpectedly succeeded') + self.assertEqual(apport.fileutils.get_new_reports(), []) + + @unittest.skipIf(os.geteuid() != 0, 'this test needs to be run as root') + def test_kernel_crashdump_kdump_log_dir_symlink(self): + '''attempted DoS with dmesg dir symlink with kdump-tools''' + + timedir = datetime.strftime(datetime.now(), '%Y%m%d%H%M') + vmcore_dir = os.path.join(apport.fileutils.report_dir, timedir) + os.mkdir(vmcore_dir + '.real') + # pretend that a user tries information disclosure by pre-creating a + # symlink to another dir + os.symlink(vmcore_dir + '.real', vmcore_dir) + os.lchown(vmcore_dir, 65534, 65534) + + dmesgfile = os.path.join(vmcore_dir, 'dmesg.' + timedir) + f = open(dmesgfile, 'wt') + f.write('1' * 100) + f.close() + + self.assertNotEqual(subprocess.call('%s/kernel_crashdump' % datadir, + stderr=subprocess.PIPE), + 0, 'kernel_crashdump unexpectedly succeeded') + self.assertEqual(apport.fileutils.get_new_reports(), []) + @classmethod def _gcc_version_path(klass): '''Determine a valid version and executable path of gcc and return it only in patch2: unchanged: --- apport-2.14.1.orig/apport/fileutils.py +++ apport-2.14.1/apport/fileutils.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 os, glob, subprocess, os.path, time, pwd +import os, glob, subprocess, os.path, time, pwd, sys try: from configparser import ConfigParser, NoOptionError, NoSectionError @@ -266,10 +266,14 @@ return 0 -def make_report_path(report, uid=None): - '''Construct a canonical pathname for the given report. +def make_report_file(report, uid=None): + '''Construct a canonical pathname for a report and open it for writing If uid is not given, it defaults to the uid of the current process. + The report file must not exist already, to prevent losing previous reports + or symlink attacks. + + Return an open file object for binary writing. ''' if 'ExecutablePath' in report: subject = report['ExecutablePath'].replace('/', '_') @@ -281,7 +285,11 @@ if not uid: uid = os.getuid() - return os.path.join(report_dir, '%s.%s.crash' % (subject, str(uid))) + path = os.path.join(report_dir, '%s.%s.crash' % (subject, str(uid))) + if sys.version >= '3': + return open(path, 'xb') + else: + return os.fdopen(os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o640), 'wb') def check_files_md5(sumfile): only in patch2: unchanged: --- apport-2.14.1.orig/data/gcc_ice_hook +++ apport-2.14.1/data/gcc_ice_hook @@ -31,5 +31,5 @@ pr['PreprocessedSource'] = (sourcefile, False) # write report -with open(apport.fileutils.make_report_path(pr), 'wb') as f: +with apport.fileutils.make_report_file(pr) as f: pr.write(f) only in patch2: unchanged: --- apport-2.14.1.orig/data/java_uncaught_exception +++ apport-2.14.1/data/java_uncaught_exception @@ -85,7 +85,7 @@ report['Title'] = make_title(report) - with open(apport.fileutils.make_report_path(report), 'wb') as f: + with apport.fileutils.make_report_file(report) as f: report.write(f) if __name__ == '__main__': only in patch2: unchanged: --- apport-2.14.1.orig/data/recoverable_problem +++ apport-2.14.1/data/recoverable_problem @@ -57,7 +57,7 @@ report.add_user_info() # Write the final report - with open(apport.fileutils.make_report_path(report), 'wb') as fp: + with apport.fileutils.make_report_file(report) as fp: report.write(fp) if __name__ == '__main__': only in patch2: unchanged: --- apport-2.14.1.orig/data/unkillable_shutdown +++ apport-2.14.1/data/unkillable_shutdown @@ -95,7 +95,7 @@ if blacklist: r['OmitPids'] = ' '.join(blacklist) - with open(apport.fileutils.make_report_path(r), 'wb') as f: + with apport.fileutils.make_report_file(r) as f: r.write(f) # only in patch2: unchanged: --- apport-2.14.1.orig/test/test_fileutils.py +++ apport-2.14.1/test/test_fileutils.py @@ -269,16 +269,36 @@ CrashCounter: 3''') self.assertEqual(apport.fileutils.get_recent_crashes(r), 3) - def test_make_report_path(self): - '''make_report_path()''' + def test_make_report_file(self): + '''make_report_file()''' pr = problem_report.ProblemReport() - self.assertRaises(ValueError, apport.fileutils.make_report_path, pr) + self.assertRaises(ValueError, apport.fileutils.make_report_file, pr) pr['Package'] = 'bash 1' - self.assertTrue(apport.fileutils.make_report_path(pr).startswith('%s/bash' % apport.fileutils.report_dir)) + with apport.fileutils.make_report_file(pr) as f: + if sys.version >= '3': + path = f.name + else: + path = os.path.join(apport.fileutils.report_dir, os.listdir(apport.fileutils.report_dir)[0]) + self.assertTrue(path.startswith('%s/bash' % apport.fileutils.report_dir), path) + os.unlink(path) + pr['ExecutablePath'] = '/bin/bash' - self.assertTrue(apport.fileutils.make_report_path(pr).startswith('%s/_bin_bash' % apport.fileutils.report_dir)) + with apport.fileutils.make_report_file(pr) as f: + if sys.version >= '3': + path = f.name + else: + path = os.path.join(apport.fileutils.report_dir, os.listdir(apport.fileutils.report_dir)[0]) + self.assertTrue(path.startswith('%s/_bin_bash' % apport.fileutils.report_dir), path) + + # file exists already, should fail now + self.assertRaises(OSError, apport.fileutils.make_report_file, pr) + + # should still fail if it's a dangling symlink + os.unlink(path) + os.symlink(os.path.join(apport.fileutils.report_dir, 'pwned'), path) + self.assertRaises(OSError, apport.fileutils.make_report_file, pr) def test_check_files_md5(self): '''check_files_md5()'''