Merge ~mdeslaur/apport:sec202205 into ~apport-hackers/apport/+git/apport-old:main

[Marc Deslauriers]

These are the security fixes that went into USN-5427-1.

cd7544b... by Marc Deslauriers on 2022-05-18

Merge remote-tracking branch 'origin/main' into sec202205

[Benjamin Drung (bdrung)]

Merged. Thanks.

review: Approve

diff --git a/apport/fileutils.py b/apport/fileutils.py
index a8385c8..e050858 100644
--- a/apport/fileutils.py
+++ b/apport/fileutils.py
@@ -57,6 +57,33 @@ def allowed_to_report():
         return False
 
 
+def get_dbus_socket(dbus_addr):
+    '''Extract the socket from a DBus address.'''
+
+    if not dbus_addr:
+        return None
+
+    # Only support unix domain sockets, and only the default Ubuntu path
+    if not dbus_addr.startswith("unix:path=/run/user/"):
+        return None
+
+    # Prevent path traversal
+    if "../" in dbus_addr:
+        return None
+
+    # Don't support escaped values, multiple addresses, or multiple keys
+    # and values
+    for search in ["%", ",", ";"]:
+        if search in dbus_addr:
+            return None
+
+    parts = dbus_addr.split("=")
+    if len(parts) != 2:
+        return None
+
+    return parts[1]
+
+
 def find_package_desktopfile(package):
     '''Return a package's .desktop file.
 
@@ -370,7 +397,7 @@ def get_config(section, setting, default=None, path=None, bool=False):
     fd = None
     f = None
     if not get_config.config:
-        get_config.config = ConfigParser()
+        get_config.config = ConfigParser(interpolation=None)
 
         try:
             fd = os.open(path, os.O_NOFOLLOW | os.O_RDONLY)
@@ -432,6 +459,22 @@ def get_uid_and_gid(contents):
     return (real_uid, real_gid)
 
 
+def search_map(mapfd, uid):
+    '''Search for an ID in a map fd'''
+    for line in mapfd:
+        fields = line.split()
+        if len(fields) != 3:
+            continue
+
+        host_start = int(fields[1])
+        host_end = host_start + int(fields[2])
+
+        if uid >= host_start and uid <= host_end:
+            return True
+
+    return False
+
+
 def get_boot_id():
     '''Gets the kernel boot id'''
 
@@ -440,7 +483,18 @@ def get_boot_id():
     return boot_id
 
 
-def get_core_path(pid=None, exe=None, uid=None, timestamp=None):
+def get_process_path(proc_pid_fd=None):
+    '''Gets the process path from a proc directory file descriptor'''
+
+    if proc_pid_fd is None:
+        return 'unknown'
+    try:
+        return os.readlink('exe', dir_fd=proc_pid_fd)
+    except OSError:
+        return 'unknown'
+
+
+def get_core_path(pid=None, exe=None, uid=None, timestamp=None, proc_pid_fd=None):
     '''Get the path to a core file'''
 
     if pid is None:
@@ -453,9 +507,8 @@ def get_core_path(pid=None, exe=None, uid=None, timestamp=None):
             timestamp = get_starttime(stat_contents)
 
     if exe is None:
-        exe = 'unknown'
-    else:
-        exe = exe.replace('/', '_').replace('.', '_')
+        exe = get_process_path(proc_pid_fd)
+    exe = exe.replace('/', '_').replace('.', '_')
 
     if uid is None:
         uid = os.getuid()
@@ -474,6 +527,7 @@ def find_core_files_by_uid(uid):
     '''Searches the core file directory for files that belong to a
        specified uid. Returns a list of lists containing the filename and
        the file modification time.'''
+    uid = str(uid)
     core_files = []
     uid_files = []
 
@@ -482,7 +536,7 @@ def find_core_files_by_uid(uid):
 
     for f in core_files:
         try:
-            if f.split('.')[2] == str(uid):
+            if f.split('.')[2] == uid:
                 time = os.path.getmtime(os.path.join(core_dir, f))
                 uid_files.append([f, time])
         except (IndexError, FileNotFoundError):
@@ -497,7 +551,7 @@ def clean_core_directory(uid):
     uid_files = find_core_files_by_uid(uid)
     sorted_files = sorted(uid_files, key=itemgetter(1))
 
-    # Substract a extra one to make room for the new core file
+    # Subtract a extra one to make room for the new core file
     if len(uid_files) > max_corefiles_per_uid - 1:
         for x in range(len(uid_files) - max_corefiles_per_uid + 1):
             os.remove(os.path.join(core_dir, sorted_files[0][0]))
diff --git a/data/apport b/data/apport
index 416173e..427f340 100755
--- a/data/apport
+++ b/data/apport
@@ -200,9 +200,10 @@ def write_user_coredump(pid, timestamp, limit, from_report=None):
         return
 
     (core_name, core_path) = apport.fileutils.get_core_path(pid,
-                                                            options.exe_path,
+                                                            options.executable_path,
                                                             crash_uid,
-                                                            timestamp)
+                                                            timestamp,
+                                                            proc_pid_fd)
 
     try:
         # Limit number of core files to prevent DoS
@@ -269,12 +270,53 @@ def usable_ram():
     return (memfree + cached - writeback) * 1024
 
 
+def _run_with_output_limit_and_timeout(args, output_limit, timeout, close_fds=True, env=None):
+    '''Run command like subprocess.run() but with output limit and timeout.
+
+    Return (stdout, stderr).'''
+
+    stdout = b""
+    stderr = b""
+
+    process = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+                               close_fds=close_fds, env=env)
+    try:
+        # Don't block so we don't deadlock
+        os.set_blocking(process.stdout.fileno(), False)
+        os.set_blocking(process.stderr.fileno(), False)
+
+        for _ in range(timeout):
+            alive = process.poll() is None
+
+            while len(stdout) < output_limit and len(stderr) < output_limit:
+                tempout = process.stdout.read(100)
+                if tempout:
+                    stdout += tempout
+                temperr = process.stderr.read(100)
+                if temperr:
+                    stderr += temperr
+                if not tempout and not temperr:
+                    break
+
+            if not alive or len(stdout) >= output_limit or len(stderr) >= output_limit:
+                break
+            time.sleep(1)
+    finally:
+        process.kill()
+
+    return stdout, stderr
+
+
 def is_closing_session():
     '''Check if pid is in a closing user session.
 
     During that, crashes are common as the session D-BUS and X.org are going
     away, etc. These crash reports are mostly noise, so should be ignored.
     '''
+    # Sanity check, don't do anything for root processes
+    if crash_uid == 0 or crash_gid == 0:
+        return False
+
     with open('environ', 'rb', opener=proc_pid_opener) as e:
         env = e.read().split(b'\0')
     for e in env:
@@ -285,6 +327,15 @@ def is_closing_session():
         error_log('is_closing_session(): no DBUS_SESSION_BUS_ADDRESS in environment')
         return False
 
+    dbus_socket = apport.fileutils.get_dbus_socket(dbus_addr)
+    if not dbus_socket:
+        error_log('is_closing_session(): Could not determine DBUS socket.')
+        return False
+
+    if not os.path.exists(dbus_socket):
+        error_log("is_closing_session(): DBUS socket doesn't exist.")
+        return False
+
     # We need to drop both the real and effective uid/gid before calling
     # gdbus because DBUS_SESSION_BUS_ADDRESS is untrusted and may allow
     # reading arbitrary files as a noncefile. We can't just drop effective
@@ -297,11 +348,11 @@ def is_closing_session():
     try:
         os.setresgid(crash_gid, crash_gid, real_gid)
         os.setresuid(crash_uid, crash_uid, real_uid)
-        gdbus = subprocess.Popen(['/usr/bin/gdbus', 'call', '-e', '-d',
-                                  'org.gnome.SessionManager', '-o', '/org/gnome/SessionManager', '-m',
-                                  'org.gnome.SessionManager.IsSessionRunning'], stdout=subprocess.PIPE,
-                                 stderr=subprocess.PIPE, env={'DBUS_SESSION_BUS_ADDRESS': dbus_addr})
-        (out, err) = gdbus.communicate()
+        out, err = _run_with_output_limit_and_timeout(['/usr/bin/gdbus', 'call', '-e', '-d',
+                                                       'org.gnome.SessionManager', '-o', '/org/gnome/SessionManager', '-m',
+                                                       'org.gnome.SessionManager.IsSessionRunning', '-t', '5'],
+                                                      1000, 5, env={'DBUS_SESSION_BUS_ADDRESS': dbus_addr})
+
         if err:
             error_log('gdbus call error: ' + err.decode('UTF-8'))
     except OSError as e:
@@ -384,38 +435,46 @@ def parse_arguments():
     parser.add_argument("-s", "--signal-number", help="signal number (%%s)")
     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)")
+    parser.add_argument("-P", "--global-pid", help="pid in root namespace (%%P)")
+    parser.add_argument("-u", "--uid", type=int, help="real UID (%%u)")
+    parser.add_argument("-g", "--gid", type=int, help="real GID (%%g)")
+    parser.add_argument("executable_path", nargs='*', help="path of executable (%%E)")
 
     options, rest = parser.parse_known_args()
 
-    if options.pid is not None:
-        for arg in rest:
-            error_log("Unknown argument: %s" % arg)
-
-    elif len(rest) in (4, 5, 6, 7):
+    # Legacy command line needs to remain for the scenario where a more
+    # recent apport is running inside a container with an older apport
+    # running on the host.
+    if options.pid is None and len(sys.argv) == 5:
         # Translate legacy command line
-        options.pid = rest[0]
-        options.signal_number = rest[1]
-        options.core_ulimit = rest[2]
-        options.dump_mode = rest[3]
-        try:
-            options.global_pid = rest[4]
-        except IndexError:
-            options.global_pid = None
-        try:
-            options.exe_path = rest[5].replace('!', '/')
-        except IndexError:
-            options.exe_path = None
-        try:
-            if rest[6] == '(deleted)':
-                options.exe_path += ' %s' % rest[6]
-        except IndexError:
-            pass
-    else:
+        return argparse.Namespace(
+            pid=sys.argv[1],
+            signal_number=sys.argv[2],
+            core_ulimit=sys.argv[3],
+            dump_mode=sys.argv[4],
+            global_pid=None,
+            uid=None,
+            gid=None,
+            executable_path=None,
+        )
+
+    if options.pid is None:
         parser.print_usage()
         sys.exit(1)
 
+    for arg in rest:
+        error_log("Unknown argument: %s" % arg)
+
+    # In kernels before 5.3.0, an executable path with spaces may be split
+    # into separate arguments. If options.executable_path is a list, join
+    # it back into a string. Also restore directory separators.
+    if isinstance(options.executable_path, list):
+        options.executable_path = " ".join(options.executable_path)
+    options.executable_path = options.executable_path.replace('!', '/')
+    # Sanity check to prevent trickery later on
+    if '../' in options.executable_path:
+        options.executable_path = None
+
     return options
 
 
@@ -485,32 +544,34 @@ if options.global_pid is not None:
 
         # Instead, attempt to find apport inside the container and
         # forward the process information there.
-        if not os.path.exists('/proc/%d/root/run/apport.socket' % host_pid):
-            error_log('host pid %s crashed in a container without apport support' %
-                      options.global_pid)
-            sys.exit(0)
 
         proc_host_pid_fd = os.open('/proc/%d' % host_pid, os.O_RDONLY | os.O_PATH | os.O_DIRECTORY)
 
         def proc_host_pid_opener(path, flags):
             return os.open(path, flags, dir_fd=proc_host_pid_fd)
 
+        # Validate that the target socket is owned by the user namespace of the process
+        try:
+            sock_fd = os.open("root/run/apport.socket", os.O_RDONLY | os.O_PATH, dir_fd=proc_host_pid_fd)
+            socket_uid = os.fstat(sock_fd).st_uid
+        except FileNotFoundError:
+            error_log('host pid %s crashed in a container without apport support' %
+                      options.global_pid)
+            sys.exit(0)
+
+        try:
+            with open("uid_map", "r", opener=proc_host_pid_opener) as fd:
+                if not apport.fileutils.search_map(fd, socket_uid):
+                    error_log("user is trying to trick apport into accessing a socket that doesn't belong to the container")
+                    sys.exit(0)
+        except FileNotFoundError:
+            pass
+
         # Validate that the crashed binary is owned by the user namespace of the process
         task_uid = os.stat("exe", dir_fd=proc_host_pid_fd).st_uid
         try:
             with open("uid_map", "r", opener=proc_host_pid_opener) as fd:
-                for line in fd:
-                    fields = line.split()
-                    if len(fields) != 3:
-                        continue
-
-                    host_start = int(fields[1])
-                    host_end = host_start + int(fields[2])
-
-                    if task_uid >= host_start and task_uid <= host_end:
-                        break
-
-                else:
+                if not apport.fileutils.search_map(fd, task_uid):
                     error_log("host pid %s crashed in a container with no access to the binary"
                               % options.global_pid)
                     sys.exit(0)
@@ -520,45 +581,28 @@ if options.global_pid is not None:
         task_gid = os.stat("exe", dir_fd=proc_host_pid_fd).st_gid
         try:
             with open("gid_map", "r", opener=proc_host_pid_opener) as fd:
-                for line in fd:
-                    fields = line.split()
-                    if len(fields) != 3:
-                        continue
-
-                    host_start = int(fields[1])
-                    host_end = host_start + int(fields[2])
-
-                    if task_gid >= host_start and task_gid <= host_end:
-                        break
-
-                else:
+                if not apport.fileutils.search_map(fd, task_gid):
                     error_log("host pid %s crashed in a container with no access to the binary"
                               % options.global_pid)
                     sys.exit(0)
         except FileNotFoundError:
             pass
 
-        # Chdir and chroot to the task
-        # WARNING: After this point, all "import" calls are security issues
-        __builtins__.__dict__['__import__'] = None
-
-        host_cwd = os.open('cwd', os.O_RDONLY | os.O_PATH | os.O_DIRECTORY, dir_fd=proc_host_pid_fd)
-
-        os.chdir(host_cwd)
-        # WARNING: we really should be using a file descriptor here,
-        #          but os.chroot won't take it
-        os.chroot(os.readlink('root', dir_fd=proc_host_pid_fd))
-
+        # Now open the socket
         sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
         try:
-            sock.connect('/run/apport.socket')
+            sock.connect('/proc/self/fd/%s' % sock_fd)
         except Exception:
             error_log('host pid %s crashed in a container with a broken apport' %
                       options.global_pid)
             sys.exit(0)
 
-        # Send all arguments except for the first (exec path) and last (global pid)
-        args = ' '.join(sys.argv[1:5])
+        # Send main arguments only
+        # Older apport in containers doesn't support positional arguments
+        args = "%s %s %s %s" % (options.pid,
+                                options.signal_number,
+                                options.core_ulimit,
+                                options.dump_mode)
         try:
             sock.sendmsg([args.encode()], [
                 # Send a ucred containing the global pid
@@ -567,7 +611,7 @@ if options.global_pid is not None:
                 # Send fd 0 (the coredump)
                 (socket.SOL_SOCKET, socket.SCM_RIGHTS, array.array('i', [0]))])
             sock.shutdown(socket.SHUT_RDWR)
-        except socket.timeout:
+        except Exception:
             error_log('Container apport failed to process crash within 30s')
 
         sys.exit(0)
@@ -598,16 +642,30 @@ try:
 
     get_pid_info(pid)
 
-    # Check if the process was replaced after the crash happened.
-    # Ideally we'd use the time of dump value provided by the kernel,
-    # but this would be a backwards-incompatible change that would
-    # require adding support to apport outside and inside of containers.
+    # Sanity check to make sure the process wasn't replaced after the crash
+    # happened. The start time isn't fine-grained enough to be an adequate
+    # security check.
     apport_start = get_apport_starttime()
     process_start = get_process_starttime()
     if process_start > apport_start:
         error_log('process was replaced after Apport started, ignoring')
         sys.exit(0)
 
+    # Make sure the process uid/gid match the ones provided by the kernel
+    # if available, if not, it may have been replaced
+    if (options.uid is not None) and (options.gid is not None):
+        if (options.uid != crash_uid) or (options.gid != crash_gid):
+            error_log("process uid/gid doesn't match expected, ignoring")
+            sys.exit(0)
+
+    # check if the executable was modified after the process started (e. g.
+    # package got upgraded in between).
+    exe_mtime = os.stat('exe', dir_fd=proc_pid_fd).st_mtime
+    process_mtime = os.lstat('cmdline', dir_fd=proc_pid_fd).st_mtime
+    if not os.path.exists(os.readlink('exe', dir_fd=proc_pid_fd)) or exe_mtime > process_mtime:
+        error_log('executable was modified after program start, ignoring')
+        sys.exit(0)
+
     error_log('called for pid %s, signal %s, core limit %s, dump mode %s' % (pid, signum, core_ulimit, dump_mode))
 
     try:
@@ -631,14 +689,6 @@ try:
         write_user_coredump(pid, process_start, core_ulimit)
         sys.exit(0)
 
-    # check if the executable was modified after the process started (e. g.
-    # package got upgraded in between)
-    exe_mtime = os.stat('exe', dir_fd=proc_pid_fd).st_mtime
-    process_mtime = os.lstat('cmdline', dir_fd=proc_pid_fd).st_mtime
-    if not os.path.exists(os.readlink('exe', dir_fd=proc_pid_fd)) or exe_mtime > process_mtime:
-        error_log('executable was modified after program start, ignoring')
-        sys.exit(0)
-
     info = apport.Report('Crash')
     info['Signal'] = signum
     core_size_limit = usable_ram() * 3 / 4
@@ -647,9 +697,8 @@ try:
 
     # 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
+    if options.executable_path is not None and os.path.exists(options.executable_path):
+        info['ExecutablePath'] = options.executable_path
 
     # Drop privileges temporarily to make sure that we don't
     # include information in the crash report that the user should
@@ -739,7 +788,7 @@ try:
                 # os.O_CREAT|os.O_EXCL
                 os.unlink(report)
             else:
-                error_log('apport: report %s already exists and unseen, doing nothing to avoid disk usage DoS' % report)
+                error_log('apport: report %s already exists and unseen, skipping to avoid disk usage DoS' % report)
                 write_user_coredump(pid, process_start, core_ulimit)
                 sys.exit(0)
         # we prefer having a file mode of 0 while writing;
diff --git a/etc/init.d/apport b/etc/init.d/apport
index 4997b83..1e10d02 100755
--- a/etc/init.d/apport
+++ b/etc/init.d/apport
@@ -50,13 +50,9 @@ do_start()
 		rm -f /var/lib/pm-utils/resume-hang.log
 	fi
 
-	# Old compatibility mode, switch later to second one
-	if true; then
-		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 -E%E" > /proc/sys/kernel/core_pattern
-	fi
+	echo "|$AGENT -p%p -s%s -c%c -d%d -P%P -u%u -g%g -- %E" > /proc/sys/kernel/core_pattern
 	echo 2 > /proc/sys/fs/suid_dumpable
+	echo 10 > /proc/sys/kernel/core_pipe_limit
 }
 
 #
@@ -70,6 +66,7 @@ do_stop()
 	#   2 if daemon could not be stopped
 	#   other if a failure occurred
 
+	echo 0 > /proc/sys/kernel/core_pipe_limit
 	echo 0 > /proc/sys/fs/suid_dumpable
 
 	# Check for a hung resume.  If we find one try and grab everything
diff --git a/tests/test_fileutils.py b/tests/test_fileutils.py
index 23f9984..0b973a4 100644
--- a/tests/test_fileutils.py
+++ b/tests/test_fileutils.py
@@ -372,8 +372,32 @@ f6423dfbc4faf022e58b4d3f5ff71a70  %s
         self.assertEqual(apport.fileutils.get_config('spethial', 'nope', 'moo'), 'moo')
         apport.fileutils.get_config.config = None  # trash cache
 
+        # interpolation
+        f.write(b'[inter]\none=1\ntwo = TWO\ntest = %(two)s\n')
+        f.flush()
+        self.assertEqual(apport.fileutils.get_config('inter', 'one'), '1')
+        self.assertEqual(apport.fileutils.get_config('inter', 'two'), 'TWO')
+        self.assertEqual(apport.fileutils.get_config('inter', 'test'), '%(two)s')
+        apport.fileutils.get_config.config = None  # trash cache
+
         f.close()
 
+    def test_get_dbus_socket(self):
+        '''get_dbus_socket()'''
+
+        tests = [("unix:path=/run/user/1000/bus", "/run/user/1000/bus"),
+                 ("unix:path=/run/user/1000/bus;unix:path=/run/user/0/bus", None),
+                 ("unix:path=%2Frun/user/1000/bus", None),
+                 ("unix:path=/run/user/1000/bus,path=/run/user/0/bus", None),
+                 ("unix:path=/etc/passwd", None),
+                 ("unix:path=/run/user/../../etc/passwd", None),
+                 ("unix:path=/run/user/1000/bus=", None),
+                 ("", None),
+                 ("tcp:host=localhost,port=8100", None)]
+
+        for (addr, result) in tests:
+            self.assertEqual(apport.fileutils.get_dbus_socket(addr), result)
+
     def test_shared_libraries(self):
         '''shared_libraries()'''