Merge lp:~stgraber/apport/bug1445064 into lp:~apport-hackers/apport/trunk

[Stéphane Graber]

This is the implementation of https://bugs.launchpad.net/ubuntu/+source/apport/+bug/1445064

The original implementation of this feature was a security nightmare and had to be reverted. This new design should be safe as the host will never actually execute any code, just contact a pre-existing apport setup and forward the crash to it.

This change introduces a couple of systemd units to setup a new /run/apport.socket UNIX socket. Upon receiving a container crash, the host apport looks for that socket in the crashed process' root. If found, it attempts to connect to it, then send the command line arguments it received along with its stdin.

Systemd in the container spawns apport whenever a crash happens, apport then connects to the host through the fd it receives from systemd and receives the stdin fd as well as the argument list from the host.

It then mangles its own sys.stdin and sys.argv to match what's sent by the host, then continues execution as normal.

The code change is pretty self contained, only new external dependency is on python3-systemd.

[Stéphane Graber (stgraber)]

Ok, so the code looks good to me, the only problem is that it doesn't work.

It's not entirely clear to me why it doesn't. The host does get the crash, finds the container's socket and sends the crash over to it. The container's apport kicks in, parses the argument line just fine and then fails during actual crash parsing due to the crashed process no longer existing.

I don't get why the crashed process disappears though, I thought the kernel was supposed to keep it around until the crash handler exists, however the host side of the crash handler sure is still running (in fact doesn't seem to want to die for some reason)...

So basically the two remaining issues are:
 - The crashed process appears to be destroyed by the kernel before the handler exits.
 - The handler seems to never exit, apparently stuck on recv() of the unix socket, despite the remote end having long exited. This may be a weirdness of systemd's socket handler, you'd think that with accept=True it'd close the socket on exit of the service, but it doesn't appear to do so.

[Stéphane Graber (stgraber)]

Correction, the timeout does kick in, so the handler eventually exits, it just doesn't do so when the container apport handler dies.

[lp:~stgraber/apport/bug1445064]

3049. By Stéphane Graber on 2016-01-27

Implement apport crash forwarding for containers

[Stéphane Graber (stgraber)]

Implemented a new, cleaner and working version of this change. I've confirmed it to work and it's now ready for review.

[lp:~stgraber/apport/bug1445064]

3050. By Stéphane Graber on 2016-01-27

Move systemd.daemon import under the LISTEN_FDS check

3051. By Stéphane Graber on 2016-01-27

Add test for apport_forward. This doesn't integrate in the test framework!

[Martin Pitt (pitti)]

Stephane, some nitpicks and questions after initial review. I'll work on the test suite integration now.

review: Needs Fixing

[Martin Pitt (pitti)]

Based on this MP I did some followup commits to lp:~pitti/apport/container-forward which turn your sketch test into a proper test case, fix the above remarks, and some other minor fallout that I noticed during testing. Stephane, can you please have a quick look at this to ensure I didn't do anything stupid?

I tested this in an LXD xenial container, and it's working nicely. Thanks again!

review: Needs Information

[Stéphane Graber (stgraber)]

Replied to the comments.

[Stéphane Graber (stgraber)]

Your changes look good to me, the ImportError try/except certainly isn't harmful even if I wouldn't expect it to be needed for a trusty backport.

I'm not sure whether that Also statement is actually useful for systemd but you know systemd way better than I do so I'll assume you're right :)

=== modified file 'data/apport'
--- data/apport	2015-11-04 20:04:52 +0000
+++ data/apport	2016-01-27 09:28:56 +0000
@@ -14,7 +14,7 @@
 # the full text of the license.
 
 import sys, os, os.path, subprocess, time, traceback, pwd, io
-import signal, inspect, grp, fcntl
+import signal, inspect, grp, fcntl, socket, atexit, array
 
 import apport, apport.fileutils
 
@@ -303,6 +303,36 @@
 #
 #################################################################
 
+# Systemd socket activation
+if "LISTEN_FDS" in os.environ:
+    from systemd.daemon import listen_fds
+
+    # Extract and validate the fd
+    fds = listen_fds()
+    if len(fds) < 1:
+        print("Invalid socket activation, no fd provided")
+        sys.exit(1)
+
+    # Open the socket
+    sock = socket.fromfd(int(fds[0]), socket.AF_UNIX, socket.SOCK_STREAM)
+    atexit.register(sock.shutdown, socket.SHUT_RDWR)
+
+    # Replace stdin by the socket activation fd
+    sys.stdin.close()
+
+    fds = array.array("i")
+    msg, ancdata, flags, addr = sock.recvmsg(4096, socket.CMSG_LEN(fds.itemsize))
+    for cmsg_level, cmsg_type, cmsg_data in ancdata:
+        if (cmsg_level == socket.SOL_SOCKET and cmsg_type == socket.SCM_RIGHTS):
+            fds.fromstring(cmsg_data[:len(cmsg_data) - (len(cmsg_data) % fds.itemsize)])
+
+    sys.stdin = os.fdopen(int(fds[0]), "r")
+
+    # Replace argv by the arguments received over the socket
+    sys.argv = [sys.argv[0]]
+    sys.argv += msg.decode().split()
+
+# Normal startup
 if len(sys.argv) not in (4, 5):
     try:
         print('Usage: %s <pid> <signal number> <core file ulimit> [global pid]' % sys.argv[0])
@@ -318,10 +348,30 @@
 # Check if we received a valid global PID (kernel >= 3.12). If we do,
 # then compare it with the local PID. If they don't match, it's an
 # indication that the crash originated from another PID namespace.
-# Simply log an entry in the host error log and exit 0.
+# Attempt to forward it to the container
 if len(sys.argv) == 5 and sys.argv[4].isdigit() and sys.argv[4] != sys.argv[1]:
-    error_log('host pid %s crashed in a container without apport support' %
-              sys.argv[4])
+    host_pid = int(sys.argv[4])
+
+    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' %
+                  sys.argv[4])
+        sys.exit(0)
+
+    sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+    try:
+        sock.connect("/proc/%d/root/run/apport.socket" % host_pid)
+    except:
+        error_log('host pid %s crashed in a container with a broken apport' %
+                  sys.argv[4])
+        sys.exit(0)
+
+    args = "%s %s %s" % (sys.argv[1], sys.argv[2], sys.argv[3])
+
+    try:
+        sock.sendmsg([args.encode()], [(socket.SOL_SOCKET, socket.SCM_RIGHTS, array.array("i", [0]))])
+        sock.shutdown(socket.SHUT_RDWR)
+    except socket.timeout:
+        error_log('Container apport failed to process crash within 30s')
     sys.exit(0)
 
 check_lock()

=== added directory 'lib'
=== added directory 'lib/systemd'
=== added directory 'lib/systemd/system'
=== added file 'lib/systemd/system/apport-forward.socket'
--- lib/systemd/system/apport-forward.socket	1970-01-01 00:00:00 +0000
+++ lib/systemd/system/apport-forward.socket	2016-01-27 09:28:56 +0000
@@ -0,0 +1,12 @@
+[Unit]
+Description=Unix socket for apport crash forwarding
+
+[Socket]
+ListenStream=/run/apport.socket
+SocketMode=0600
+Accept=yes
+MaxConnections=10
+Backlog=5
+
+[Install]
+WantedBy=sockets.target

=== added file 'lib/systemd/system/apport-forward@.service'
--- lib/systemd/system/apport-forward@.service	1970-01-01 00:00:00 +0000
+++ lib/systemd/system/apport-forward@.service	2016-01-27 09:28:56 +0000
@@ -0,0 +1,11 @@
+[Unit]
+Description=Apport crash forwarding receiver
+Requires=apport-forward.socket
+
+[Service]
+Type=forking
+ExecStart=/usr/share/apport/apport
+
+[Install]
+WantedBy=multi-user.target
+Also=status.socket

=== added file 'test/test_apport_forward.py'
--- test/test_apport_forward.py	1970-01-01 00:00:00 +0000
+++ test/test_apport_forward.py	2016-01-27 09:28:56 +0000
@@ -0,0 +1,44 @@
+#!/usr/bin/python3
+
+import array
+import os
+import socket
+import sys
+import tempfile
+
+path = tempfile.mktemp()
+
+server = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+server.bind(path)
+server.listen(1)
+
+parent_pid = os.getpid()
+
+if os.fork() == 0:
+    client = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+    client.connect(path)
+
+    blob = tempfile.mktemp()
+    with open(blob, "w+") as fd:
+        fd.write("blob")
+
+    with open(blob, "r") as fd:
+        args = "%s 11 100" % parent_pid
+        client.sendmsg([args.encode()], [(socket.SOL_SOCKET, socket.SCM_RIGHTS,
+                                          array.array("i", [fd.fileno()]))])
+    os.remove(blob)
+    sys.exit(0)
+
+conn, addr = server.accept()
+
+os.dup2(conn.fileno(), 3)
+child = os.fork()
+if child == 0:
+    os.environ["LISTEN_PID"] = "%s" % os.getpid()
+    os.environ["LISTEN_FDNAMES"] = "connection"
+    os.environ["LISTEN_FDS"] = "1"
+
+    os.execl("data/apport", "apport")
+
+os.waitpid(child, 0)
+os.remove(path)