SECURITY UPDATE: Fix core dump file injection

When writing a core dump file for a crashed packaged program, don't close and
reopen the .crash report file but just rewind and re-read it. This prevents the
user from modifying the .crash report file while "apport" is running to inject
data and creating crafted core dump files.

By itself this is not a vulnerability, but in conjunction with the previous
vulnerability of writing core dump files to arbitrary directories
(CVE-2015-1324) this could be exploited to gain root privileges, by writing a
crafted "core" file to /etc/sudoers.d/, /etc/cron.d, or similar.

Thanks to Philip Pettersson for discovering this issue!

CVE-2015-1325
LP: #1453900

=== modified file 'NEWS'
--- NEWS	2015-05-13 07:33:04 +0000
+++ NEWS	2015-05-13 09:02:04 +0000
@@ -12,6 +12,15 @@
    with the intention of core(5).
    Thanks to Sander Bos for discovering this issue!
    (CVE-2015-1324, LP: #1452239)
+ * SECURITY UPDATE: When writing a core dump file for a crashed packaged
+   program, don't close and reopen the .crash report file but just rewind and
+   re-read it. This prevents the user from modifying the .crash report file
+   while "apport" is running to inject data and creating crafted core
+   dump files. In conjunction with the above vulnerability of writing core
+   dump files to arbitrary directories this could be exploited to gain root
+   privileges.
+   Thanks to Philip Pettersson for discovering this issue!
+   (CVE-2015-1325, LP: #1453900)
  * apportcheckresume: Fix "occured" typo, thanks Matthew Paul Thomas.
    (LP: #1448636)
  * signal_crashes test: Fix test_crash_setuid_* to look at whether

=== modified file 'data/apport'
--- data/apport	2015-05-13 07:33:04 +0000
+++ data/apport	2015-05-13 09:02:04 +0000
@@ -185,8 +185,7 @@
     # Priming read
     if from_report:
         r = apport.Report()
-        with open(from_report, 'rb') as f:
-            r.load(f)
+        r.load(from_report)
         core_size = len(r['CoreDump'])
         if limit > 0 and core_size > limit:
             error_log('aborting core dump writing, size %i exceeds current limit' % core_size)
@@ -438,7 +437,7 @@
             mode = 0o640
         else:
             mode = 0
-        reportfile = os.fdopen(os.open(report, os.O_WRONLY | os.O_CREAT | os.O_EXCL, mode), 'wb')
+        reportfile = os.fdopen(os.open(report, os.O_RDWR | os.O_CREAT | os.O_EXCL, mode), 'w+b')
         assert reportfile.fileno() > sys.stderr.fileno()
 
         # Make sure the crash reporting daemon can read this report
@@ -474,7 +473,6 @@
                     os.fsync(fd)
                 finally:
                     os.close(fd)
-            reportfile.close()
     except IOError:
         if reportfile != sys.stderr:
             os.unlink(report)
@@ -493,7 +491,8 @@
     # written report, as we can only read stdin once and write_user_coredump()
     # might abort reading from stdin and remove the written core file when
     # core_ulimit is > 0 and smaller than the core size.
-    write_user_coredump(pid, cwd, core_ulimit, from_report=report)
+    reportfile.seek(0)
+    write_user_coredump(pid, cwd, core_ulimit, from_report=reportfile)
 
 except (SystemExit, KeyboardInterrupt):
     raise

=== modified file 'test/test_signal_crashes.py'
--- test/test_signal_crashes.py	2015-05-13 07:33:04 +0000
+++ test/test_signal_crashes.py	2015-05-13 09:02:04 +0000
@@ -389,6 +389,49 @@
                               sig=sig)
                 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)
+        os.sync()
+
+        # 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'''