|
|
baab13 |
From a4e47c753e9d7988f4d938ed2e0fd690a909ce68 Mon Sep 17 00:00:00 2001
|
|
|
baab13 |
From: Jakub Filak <jfilak@redhat.com>
|
|
|
baab13 |
Date: Mon, 20 Apr 2015 15:15:40 +0200
|
|
|
baab13 |
Subject: [ABRT PATCH] upload: validate and sanitize uploaded dump directories
|
|
|
baab13 |
|
|
|
baab13 |
It was discovered that, when moving problem reports from
|
|
|
baab13 |
/var/spool/abrt-upload to /var/spool/abrt or /var/tmp/abrt,
|
|
|
baab13 |
abrt-handle-upload does not verify that the new problem directory
|
|
|
baab13 |
has appropriate permissions and does not contain symbolic links. A
|
|
|
baab13 |
crafted problem report exposes other parts of abrt to attack, and
|
|
|
baab13 |
the abrt-handle-upload script allows to overwrite arbitrary files.
|
|
|
baab13 |
|
|
|
baab13 |
Acknowledgement:
|
|
|
baab13 |
|
|
|
baab13 |
This issue was discovered by Florian Weimer of Red Hat Product Security.
|
|
|
baab13 |
|
|
|
baab13 |
Related: #1212953
|
|
|
baab13 |
|
|
|
baab13 |
Signed-off-by: Jakub Filak <jfilak@redhat.com>
|
|
|
baab13 |
---
|
|
|
baab13 |
src/daemon/abrt-handle-upload.in | 78 +++++++++++++++++++++++++++++++++++-----
|
|
|
baab13 |
1 file changed, 70 insertions(+), 8 deletions(-)
|
|
|
baab13 |
|
|
|
baab13 |
diff --git a/src/daemon/abrt-handle-upload.in b/src/daemon/abrt-handle-upload.in
|
|
|
baab13 |
index dbc4534..7720da4 100755
|
|
|
baab13 |
--- a/src/daemon/abrt-handle-upload.in
|
|
|
baab13 |
+++ b/src/daemon/abrt-handle-upload.in
|
|
|
baab13 |
@@ -10,6 +10,7 @@ import getopt
|
|
|
baab13 |
import tempfile
|
|
|
baab13 |
import shutil
|
|
|
baab13 |
import datetime
|
|
|
baab13 |
+import grp
|
|
|
baab13 |
|
|
|
baab13 |
from reportclient import set_verbosity, error_msg_and_die, error_msg, log
|
|
|
baab13 |
|
|
|
baab13 |
@@ -36,12 +37,77 @@ def init_gettext():
|
|
|
baab13 |
|
|
|
baab13 |
import problem
|
|
|
baab13 |
|
|
|
baab13 |
-def write_str_to(filename, s):
|
|
|
baab13 |
- fd = os.open(filename, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, @DEFAULT_DUMP_DIR_MODE@ | stat.S_IROTH)
|
|
|
baab13 |
+def write_str_to(filename, s, uid, gid, mode):
|
|
|
baab13 |
+ fd = os.open(filename, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, mode)
|
|
|
baab13 |
if fd >= 0:
|
|
|
baab13 |
+ os.fchown(fd, uid, gid)
|
|
|
baab13 |
os.write(fd, s)
|
|
|
baab13 |
os.close(fd)
|
|
|
baab13 |
|
|
|
baab13 |
+
|
|
|
baab13 |
+def validate_transform_move_and_notify(uploaded_dir_path, problem_dir_path, dest=None):
|
|
|
baab13 |
+ fsuid = 0
|
|
|
baab13 |
+ fsgid = 0
|
|
|
baab13 |
+
|
|
|
baab13 |
+ try:
|
|
|
baab13 |
+ gabrt = grp.getgrnam("abrt")
|
|
|
baab13 |
+ fsgid = gabrt.gr_gid
|
|
|
baab13 |
+ except KeyError as ex:
|
|
|
baab13 |
+ error_msg("Failed to get GID of 'abrt' (using 0 instead): {0}'".format(str(ex)))
|
|
|
baab13 |
+
|
|
|
baab13 |
+ try:
|
|
|
baab13 |
+ # give the uploaded directory to 'root:abrt' or 'root:root'
|
|
|
baab13 |
+ os.chown(uploaded_dir_path, fsuid, fsgid)
|
|
|
baab13 |
+ # set the right permissions for this machine
|
|
|
baab13 |
+ # (allow the owner and the group to access problem elements,
|
|
|
baab13 |
+ # the default dump dir mode lacks x bit for both)
|
|
|
baab13 |
+ os.chmod(uploaded_dir_path, @DEFAULT_DUMP_DIR_MODE@ | stat.S_IXUSR | stat.S_IXGRP)
|
|
|
baab13 |
+
|
|
|
baab13 |
+ # sanitize problem elements
|
|
|
baab13 |
+ for item in os.listdir(uploaded_dir_path):
|
|
|
baab13 |
+ apath = os.path.join(uploaded_dir_path, item)
|
|
|
baab13 |
+ if os.path.islink(apath):
|
|
|
baab13 |
+ # remove symbolic links
|
|
|
baab13 |
+ os.remove(apath)
|
|
|
baab13 |
+ elif os.path.isdir(apath):
|
|
|
baab13 |
+ # remove directories
|
|
|
baab13 |
+ shutil.rmtree(apath)
|
|
|
baab13 |
+ elif os.path.isfile(apath):
|
|
|
baab13 |
+ # set file ownership to 'root:abrt' or 'root:root'
|
|
|
baab13 |
+ os.chown(apath, fsuid, fsgid)
|
|
|
baab13 |
+ # set the right file permissions for this machine
|
|
|
baab13 |
+ os.chmod(apath, @DEFAULT_DUMP_DIR_MODE@)
|
|
|
baab13 |
+ else:
|
|
|
baab13 |
+ # remove things that are neither files, symlinks nor directories
|
|
|
baab13 |
+ os.remove(apath)
|
|
|
baab13 |
+ except OSError as ex:
|
|
|
baab13 |
+ error_msg("Removing uploaded dir '{0}': '{1}'".format(uploaded_dir_path, str(ex)))
|
|
|
baab13 |
+ try:
|
|
|
baab13 |
+ shutil.rmtree(uploaded_dir_path)
|
|
|
baab13 |
+ except OSError as ex2:
|
|
|
baab13 |
+ error_msg_and_die("Failed to clean up dir '{0}': '{1}'".format(uploaded_dir_path, str(ex2)))
|
|
|
baab13 |
+ return
|
|
|
baab13 |
+
|
|
|
baab13 |
+ # overwrite remote if it exists
|
|
|
baab13 |
+ remote_path = os.path.join(uploaded_dir_path, "remote")
|
|
|
baab13 |
+ write_str_to(remote_path, "1", fsuid, fsgid, @DEFAULT_DUMP_DIR_MODE@)
|
|
|
baab13 |
+
|
|
|
baab13 |
+ # abrtd would increment count value and abrt-server refuses to process
|
|
|
baab13 |
+ # problem directories containing 'count' element when PrivateReports is on.
|
|
|
baab13 |
+ count_path = os.path.join(uploaded_dir_path, "count")
|
|
|
baab13 |
+ if os.path.exists(count_path):
|
|
|
baab13 |
+ # overwrite remote_count if it exists
|
|
|
baab13 |
+ remote_count_path = os.path.join(uploaded_dir_path, "remote_count")
|
|
|
baab13 |
+ os.rename(count_path, remote_count_path)
|
|
|
baab13 |
+
|
|
|
baab13 |
+ if not dest:
|
|
|
baab13 |
+ dest = problem_dir_path
|
|
|
baab13 |
+
|
|
|
baab13 |
+ shutil.move(uploaded_dir_path, dest)
|
|
|
baab13 |
+
|
|
|
baab13 |
+ problem.notify_new_path(problem_dir_path)
|
|
|
baab13 |
+
|
|
|
baab13 |
+
|
|
|
baab13 |
if __name__ == "__main__":
|
|
|
baab13 |
|
|
|
baab13 |
# Helper: exit with cleanup
|
|
|
baab13 |
@@ -177,21 +243,17 @@ if __name__ == "__main__":
|
|
|
baab13 |
# or one or more complete problem data directories.
|
|
|
baab13 |
# Checking second possibility first.
|
|
|
baab13 |
if os.path.exists(tempdir+"/analyzer") and os.path.exists(tempdir+"/time"):
|
|
|
baab13 |
- write_str_to(tempdir+"/remote", "1")
|
|
|
baab13 |
- shutil.move(tempdir, abrt_dir)
|
|
|
baab13 |
- problem.notify_new_path(abrt_dir+"/"+os.path.basename(tempdir))
|
|
|
baab13 |
+ validate_transform_move_and_notify(tempdir, abrt_dir+"/"+os.path.basename(tempdir), dest=abrt_dir)
|
|
|
baab13 |
else:
|
|
|
baab13 |
for d in os.listdir(tempdir):
|
|
|
baab13 |
if not os.path.isdir(tempdir+"/"+d):
|
|
|
baab13 |
continue
|
|
|
baab13 |
- write_str_to(tempdir+"/"+d+"/remote", "1")
|
|
|
baab13 |
dst = abrt_dir+"/"+d
|
|
|
baab13 |
if os.path.exists(dst):
|
|
|
baab13 |
dst += "."+str(os.getpid())
|
|
|
baab13 |
if os.path.exists(dst):
|
|
|
baab13 |
continue
|
|
|
baab13 |
- shutil.move(tempdir+"/"+d, dst)
|
|
|
baab13 |
- problem.notify_new_path(dst)
|
|
|
baab13 |
+ validate_transform_move_and_notify(tempdir+"/"+d, dst)
|
|
|
baab13 |
|
|
|
baab13 |
die_exitcode = 0
|
|
|
baab13 |
# This deletes working_dir (== delete_on_exit)
|
|
|
baab13 |
--
|
|
|
baab13 |
1.8.3.1
|
|
|
baab13 |
|