management: shell out external programs in a more secure way

management/backup.py

@@ -12,7 +12,7 @@
 
 import os, os.path, subprocess
 
-from utils import exclusive_process, load_environment
+from utils import exclusive_process, load_environment, shell
 
 env = load_environment()
 
@@ -24,31 +24,43 @@ rdiff_backup_dir = os.path.join(backup_dir, 'rdiff-history')
 os.makedirs(backup_dir, exist_ok=True)
 
 # Stop services.
-subprocess.check_call(["service", "dovecot", "stop"])
-subprocess.check_call(["service", "postfix", "stop"])
+shell('check_call', ["/usr/sbin/service", "dovecot", "stop"])
+shell('check_call', ["/usr/sbin/service", "postfix", "stop"])
 
 # Update the backup directory which stores increments.
 try:
-	subprocess.check_call([
-		"rdiff-backup",
+	shell('check_call', [
+		"/usr/bin/rdiff-backup",
 		"--exclude", backup_dir,
 	 	env["STORAGE_ROOT"],
 	 	rdiff_backup_dir])
 except subprocess.CalledProcessError:
+	# Trap the error so we restart services again.
 	pass
 
 # Start services.
-subprocess.check_call(["service", "dovecot", "start"])
-subprocess.check_call(["service", "postfix", "start"])
-
-# Tar the rdiff-backup directory into a single file encrypted using the backup private key.
-os.system(
-	"tar -zcC %s . | openssl enc -aes-256-cbc -a -salt -in /dev/stdin -out %s -pass file:%s"
-	%
-	(	rdiff_backup_dir,
-		os.path.join(backup_dir, "latest.tgz.enc"),
-		os.path.join(backup_dir, "secret_key.txt"),
-	))
+shell('check_call', ["/usr/sbin/service", "dovecot", "start"])
+shell('check_call', ["/usr/sbin/service", "postfix", "start"])
+
+# Tar the rdiff-backup directory into a single file.
+shell('check_call', [
+	"/bin/tar",
+	"-zc",
+	"-f", os.path.join(backup_dir, "latest.tgz"),
+	"-C", rdiff_backup_dir,
+	"."])
+
+# Encrypt the backup using the backup private key.
+shell('check_call', [
+	"/usr/bin/openssl",
+	"enc",
+	"-aes-256-cbc",
+	"-a",
+	"-salt",
+	"-in", os.path.join(backup_dir, "latest.tgz"),
+	"-out", os.path.join(backup_dir, "latest.tgz.enc"),
+	"-pass", "file:%s" % os.path.join(backup_dir, "secret_key.txt"),
+	])
 
 # The backup can be decrypted with:
 # openssl enc -d -aes-256-cbc -a -in latest.tgz.enc -out /dev/stdout -pass file:secret_key.txt | tar -z

management/daemon.py

 #!/usr/bin/python3
 
-import os, os.path, subprocess
+import os, os.path, re
 
 from flask import Flask, request, render_template
 app = Flask(__name__)
@@ -59,15 +59,21 @@ def dns_update():
 
 @app.route('/system/updates')
 def show_updates():
-	subprocess.check_call("apt-get -qq update", shell=True)
-	return subprocess.check_output(
-		r"""apt-get -qq -s upgrade | grep -v ^Conf | sed "s/^Inst /Updated Package Available: /" | sed "s/\[\(.*\)\] (\(\S*\).*/\(\1 => \2\)/" """,
-		shell=True)
+	utils.shell("check_call", ["/usr/bin/apt-get", "-qq", "update"])
+	simulated_install = utils.shell("check_output", ["/usr/bin/apt-get", "-qq", "-s", "upgrade"])
+	pkgs = []
+	for line in simulated_install.split('\n'):
+		if re.match(r'^Conf .*', line): continue # remove these lines, not informative
+		line = re.sub(r'^Inst (.*) \[(.*)\] \((\S*).*', r'Updated Package Available: \1 (\3)', line) # make these lines prettier
+		pkgs.append(line)
+	return "\n".join(pkgs)
 
 @app.route('/system/update-packages', methods=["POST"])
 def do_updates():
-	subprocess.check_call("apt-get -qq update", shell=True)
-	return subprocess.check_output("DEBIAN_FRONTEND=noninteractive  apt-get -y upgrade", shell=True)
+	utils.shell("check_call", ["/usr/bin/apt-get", "-qq", "update"])
+	return utils.shell("check_output", ["/usr/bin/apt-get", "-y", "upgrade"], env={
+		"DEBIAN_FRONTEND": "noninteractive"
+	})
 
 # APP
 

management/mailconfig.py

 import subprocess, shutil, os, sqlite3, re
+import utils
 
 def validate_email(email, strict):
 	# There are a lot of characters permitted in email addresses, but
@@ -52,7 +53,7 @@ def add_mail_user(email, pw, env):
 	conn, c = open_database(env, with_connection=True)
 
 	# hash the password
-	pw = subprocess.check_output(["/usr/bin/doveadm", "pw", "-s", "SHA512-CRYPT", "-p", pw]).strip()
+	pw = utils.shell('check_output', ["/usr/bin/doveadm", "pw", "-s", "SHA512-CRYPT", "-p", pw]).strip()
 
 	# add the user to the database
 	try:
@@ -68,14 +69,14 @@ def add_mail_user(email, pw, env):
 	# Check if the mailboxes exist before creating them. When creating a user that had previously
 	# been deleted, the mailboxes will still exist because they are still on disk.
 	try:
-		existing_mboxes = subprocess.check_output(["doveadm", "mailbox", "list", "-u", email, "-8"], stderr=subprocess.STDOUT).decode("utf8").split("\n")
+		existing_mboxes = utils.shell('check_output', ["doveadm", "mailbox", "list", "-u", email, "-8"], capture_stderr=True).split("\n")
 	except subprocess.CalledProcessError as e:
 		c.execute("DELETE FROM users WHERE email=?", (email,))
 		conn.commit()
 		return ("Failed to initialize the user: " + e.output.decode("utf8"), 400)
 
-	if "INBOX" not in existing_mboxes: subprocess.check_call(["doveadm", "mailbox", "create", "-u", email, "-s", "INBOX"])
-	if "Spam" not in existing_mboxes: subprocess.check_call(["doveadm", "mailbox", "create", "-u", email, "-s", "Spam"])
+	if "INBOX" not in existing_mboxes: utils.shell('check_call', ["doveadm", "mailbox", "create", "-u", email, "-s", "INBOX"])
+	if "Spam" not in existing_mboxes: utils.shell('check_call', ["doveadm", "mailbox", "create", "-u", email, "-s", "Spam"])
 
 	# Create the user's sieve script to move spam into the Spam folder, and make it owned by mail.
 	maildirstat = os.stat(env["STORAGE_ROOT"] + "/mail/mailboxes")
@@ -93,7 +94,7 @@ def add_mail_user(email, pw, env):
 
 def set_mail_password(email, pw, env):
 	# hash the password
-	pw = subprocess.check_output(["/usr/bin/doveadm", "pw", "-s", "SHA512-CRYPT", "-p", pw]).strip()
+	pw = utils.shell('check_output', ["/usr/bin/doveadm", "pw", "-s", "SHA512-CRYPT", "-p", pw]).strip()
 
 	# update the database
 	conn, c = open_database(env, with_connection=True)

management/utils.py

@@ -74,4 +74,14 @@ def is_pid_valid(pid):
         else: # EINVAL
             raise
     else:
-        return True
\ No newline at end of file
+        return True
+
+def shell(method, cmd_args, env={}, capture_stderr=False):
+    # A safe way to execute processes.
+    # Some processes like apt-get require being given a sane PATH.
+    import subprocess
+    env.update({ "PATH": "/sbin:/bin:/usr/sbin:/usr/bin" })
+    stderr = None if not capture_stderr else subprocess.STDOUT
+    ret = getattr(subprocess, method)(cmd_args, env=env, stderr=stderr)
+    if isinstance(ret, bytes): ret = ret.decode("utf8")
+    return ret