Merge lp:~mwhudson/ubuntu/focal/apport/lp-1854237 into lp:~ubuntu-core-dev/ubuntu/focal/apport/ubuntu

[Michael Hudson-Doyle]

* 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.

[lp:~mwhudson/ubuntu/focal/apport/lp-1854237]

2728. By Michael Hudson-Doyle on 2019-12-05

Restore add_proc_info raising ValueError for a dead process.

2729. By Michael Hudson-Doyle on 2019-12-05

Delete test_lock_symlink, no longer applicable now that the lock is
created in a directory only root can write to.

2730. By Michael Hudson-Doyle on 2019-12-05

fix test_get_logind_session

apport.Report.get_logind_session now takes a proc_pid_fd, not a pid

[Michael Hudson-Doyle (mwhudson)]

This branch now passes the autopkgtests in my testing \o/

[Brian Murray (brian-murray)]

This looks great! Thanks for taking the time to dig into it and sort it out properly.

review: Approve

=== modified file 'apport/report.py'
--- apport/report.py	2019-12-03 03:18:16 +0000
+++ apport/report.py	2019-12-05 02:42:15 +0000
@@ -541,7 +541,15 @@
             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)
+            try:
+                proc_pid_fd = os.open('/proc/%s' % pid, os.O_RDONLY | os.O_PATH | os.O_DIRECTORY)
+            except (PermissionError, OSError, FileNotFoundError) 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)
@@ -551,15 +559,16 @@
         self['ProcStatus'] = _read_file('status', dir_fd=proc_pid_fd)
         self['ProcCmdline'] = _read_file('cmdline', dir_fd=proc_pid_fd).rstrip('\0')
         self['ProcMaps'] = _read_maps(proc_pid_fd)
-        try:
-            self['ExecutablePath'] = os.readlink('exe', dir_fd=proc_pid_fd)
-        except (PermissionError, OSError, FileNotFoundError) 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
+        if 'ExecutablePath' not in self:
+            try:
+                self['ExecutablePath'] = os.readlink('exe', dir_fd=proc_pid_fd)
+            except (PermissionError, OSError, FileNotFoundError) 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
         for p in ('rofs', 'rwfs', 'squashmnt', 'persistmnt'):
             if self['ExecutablePath'].startswith('/%s/' % p):
                 self['ExecutablePath'] = self['ExecutablePath'][len('/%s' % p):]
 
=== modified file 'data/apport'
--- data/apport	2019-12-03 03:18:16 +0000
+++ data/apport	2019-12-05 02:42:15 +0000
@@ -211,7 +211,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'])
@@ -227,17 +227,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():
@@ -340,7 +339,7 @@
 def parse_arguments():
     parser = argparse.ArgumentParser(epilog="""
     Alternatively, the following command line is understood for legacy hosts:
-        <pid> <signal number> <core file ulimit> <dump mode> [global pid]
+        <pid> <signal number> <core file ulimit> <dump mode> [global pid] [exe path]
     """)
 
     # TODO: Use type=int
@@ -349,6 +348,7 @@
     parser.add_argument("-c", "--core-ulimit", help="core ulimit (%%c)")
     parser.add_argument("-d", "--dump-mode", help="dump mode (%%d)")
     parser.add_argument("-P", "--global-pid", nargs='?', help="pid in root namespace (%%P)")
+    parser.add_argument("-E", "--executable-path", nargs='?', help="path of executable (%%E)")
 
     options, rest = parser.parse_known_args()
 
@@ -356,7 +356,7 @@
         for arg in rest:
             error_log("Unknown argument: %s", arg)
 
-    elif len(rest) in (4, 5):
+    elif len(rest) in (4, 5, 6):
         # Translate legacy command line
         options.pid = rest[0]
         options.signal_number = rest[1]
@@ -366,6 +366,10 @@
             options.global_pid = rest[4]
         except IndexError:
             options.global_pid = None
+        try:
+            options.exe_path = rest[5].replace('!', '/')
+        except IndexError:
+            options.exe_path = None
     else:
         parser.print_usage()
         sys.exit(1)
@@ -599,6 +603,9 @@
     # We already need this here to figure out the ExecutableName (for scripts,
     # etc).
 
+    if options.exe_path is not None and os.path.exists(options.exe_path):
+        info['ExecutablePath'] = options.exe_path
+
     euid = os.geteuid()
     egid = os.getegid()
     try:
 
=== modified file 'debian/changelog'
--- debian/changelog	2019-12-04 21:24:04 +0000
+++ debian/changelog	2019-12-05 02:42:15 +0000
@@ -1,8 +1,20 @@
 apport (2.20.11-0ubuntu13) UNRELEASED; urgency=medium
 
+  [ Brian Murray ]
   * Create additional symlinks to the source_linux.py apport package hook for
     many OEM kernels. Thanks to You-Sheng Yang for the patch. (LP: #1847967)
 
+  [ Michael Hudson-Doyle ]
+  * 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.
+
  -- Brian Murray <[e-mail]>  Wed, 04 Dec 2019 13:21:43 -0800
 
 apport (2.20.11-0ubuntu12) focal; urgency=medium
 
=== modified file 'etc/init.d/apport'
--- etc/init.d/apport	2019-05-16 19:37:48 +0000
+++ etc/init.d/apport	2019-12-05 02:42:15 +0000
@@ -52,9 +52,9 @@
 
 	# Old compatibility mode, switch later to second one
 	if true; then
-		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
 	else
-		echo "|$AGENT -p%p -s%s -c%c -d%d -P%P" > /proc/sys/kernel/core_pattern
+		echo "|$AGENT -p%p -s%s -c%c -d%d -P%P -E%E" > /proc/sys/kernel/core_pattern
 	fi
 	echo 2 > /proc/sys/fs/suid_dumpable
 }
 
=== modified file 'test/test_report.py'
--- test/test_report.py	2019-05-17 17:51:10 +0000
+++ test/test_report.py	2019-12-05 02:42:15 +0000
@@ -2274,7 +2274,9 @@
         self.assertEqual(expected, pr.crash_signature())
 
     def test_get_logind_session(self):
-        ret = apport.Report.get_logind_session(os.getpid())
+        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)
         if ret is None:
             # ensure that we don't run under logind, and thus the None is
             # justified
 
=== modified file 'test/test_signal_crashes.py'
--- test/test_signal_crashes.py	2018-05-09 23:30:27 +0000
+++ test/test_signal_crashes.py	2019-12-05 02:42:15 +0000
@@ -192,34 +192,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'''