apport-hackers team mailing list archive
-
apport-hackers team
-
Mailing list archive
-
Message #00325
[Merge] ~mdeslaur/apport:sec202205 into apport:main
Marc Deslauriers has proposed merging ~mdeslaur/apport:sec202205 into apport:main.
Commit message:
These are the security fixes that went into USN-5427-1.
Requested reviews:
Benjamin Drung (bdrung)
For more details, see:
https://code.launchpad.net/~mdeslaur/apport/+git/apport/+merge/422837
--
Your team Apport upstream developers is subscribed to branch apport:main.
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/test/test_fileutils.py b/test/test_fileutils.py
index 23f9984..0b973a4 100644
--- a/test/test_fileutils.py
+++ b/test/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()'''
diff --git a/test/test_hookutils.py b/test/test_hookutils.py
index b37a357..7b0e54a 100644
--- a/test/test_hookutils.py
+++ b/test/test_hookutils.py
@@ -48,6 +48,7 @@ class T(unittest.TestCase):
self.assertFalse(good_ko.name in nonfree)
self.assertTrue(bad_ko.name in nonfree)
+<<<<<<< test/test_hookutils.py
def test_real_module_license_evaluation(self):
'''module licenses can be validated correctly for real module'''
isofs_license = apport.hookutils._get_module_license('isofs')
@@ -62,6 +63,8 @@ class T(unittest.TestCase):
nonfree = apport.hookutils.nonfree_kernel_modules(f.name)
self.assertNotIn('isofs', nonfree)
+=======
+>>>>>>> test/test_hookutils.py
@unittest.mock.patch("apport.hookutils.root_command_output")
def test_attach_dmesg(self, root_command_output_mock):
'''attach_dmesg()'''
diff --git a/test/test_python_crashes.py b/test/test_python_crashes.py
index 06d9769..66dcc08 100644
--- a/test/test_python_crashes.py
+++ b/test/test_python_crashes.py
@@ -14,15 +14,22 @@ import unittest, tempfile, subprocess, os, stat, shutil, atexit
import dbus
import unittest.mock
-temp_report_dir = tempfile.mkdtemp()
-os.environ['APPORT_REPORT_DIR'] = temp_report_dir
-atexit.register(shutil.rmtree, temp_report_dir)
-
import apport.fileutils
import apport.report
class T(unittest.TestCase):
+ @classmethod
+ def setUpClass(cls):
+ cls.orig_report_dir = apport.fileutils.report_dir
+ apport.fileutils.report_dir = tempfile.mkdtemp()
+ os.environ['APPORT_REPORT_DIR'] = apport.fileutils.report_dir
+ atexit.register(shutil.rmtree, apport.fileutils.report_dir)
+
+ @classmethod
+ def tearDownClass(cls):
+ apport.fileutils.report_dir = cls.orig_report_dir
+
def tearDown(self):
for f in apport.fileutils.get_all_reports():
os.unlink(f)
Follow ups
