From 4ad3c5efcf348efbe8e517e1441be2eec0f4e4ab Mon Sep 17 00:00:00 2001 From: Klaas van Schelven Date: Fri, 29 Aug 2025 13:11:28 +0200 Subject: [PATCH] Hardening of Temporary-Directory Usage Defends against certain forms of local privilege escalation, i.e. understood to be defense in depth rather than a security issue given the recommended ways of deploying (docker container or in a single-use single-server) Fix #174 See https://github.com/python/cpython/pull/23901 --- .bandit | 3 - LICENSE | 3 + bsmain/future_python.py | 52 ++++++++++++++++ bsmain/utils.py | 102 ++++++++++++++++++++++++++++++++ bugsink/app_settings.py | 3 +- bugsink/settings/development.py | 4 +- ingest/views.py | 3 +- snappea/foreman.py | 5 +- snappea/models.py | 4 +- snappea/settings.py | 7 ++- 10 files changed, 174 insertions(+), 12 deletions(-) create mode 100644 bsmain/future_python.py create mode 100644 bsmain/utils.py diff --git a/.bandit b/.bandit index c1c24be..4a6b1fa 100644 --- a/.bandit +++ b/.bandit @@ -1,7 +1,4 @@ [bandit] -# Skip B108 ("hardcoded temp dir"), see https://github.com/bugsink/bugsink/issues/174 -skips = B108 - # Exclude any file named tests.py anywhere under the tree exclude = tests.py diff --git a/LICENSE b/LICENSE index 7ed9139..8203ade 100644 --- a/LICENSE +++ b/LICENSE @@ -16,6 +16,9 @@ Portions of this software are licensed as follows: Please refer to the license of the respective component in your package manager's repository. +* The following files are licensed under the Python Software Foundation License + * bsmain/future_python.py + * Content outside of the above mentioned directories or restrictions above is available under the license as defined below: diff --git a/bsmain/future_python.py b/bsmain/future_python.py new file mode 100644 index 0000000..fa5c66e --- /dev/null +++ b/bsmain/future_python.py @@ -0,0 +1,52 @@ +# A backport of a not-yet-released version of Python's os.makedirs +# +# License: Python Software Foundation License +# +# From: +# https://github.com/python/cpython/pull/23901 as per +# https://github.com/python/cpython/pull/23901/commits/128ff8b46696c26e2cea5609cf9840b9425dcccf +# +# Note on stability: os.makedirs has not seen any changes after Python 3.7 up to +# 3.13 (3.14 is in pre-release, so unlikely to see changes). This means that the +# current code can be used as a "extra feature" drop in for at least those versions. + +from os import path, mkdir, curdir + + +def makedirs(name, mode=0o777, exist_ok=False, *, recursive_mode=False): + """makedirs(name [, mode=0o777][, exist_ok=False][, recursive_mode=False]) + + Super-mkdir; create a leaf directory and all intermediate ones. Works like + mkdir, except that any intermediate path segment (not just the rightmost) + will be created if it does not exist. If the target directory already + exists, raise an OSError if exist_ok is False. Otherwise no exception is + raised. If recursive_mode is True, the mode argument will affect the file + permission bits of any newly-created, intermediate-level directories. This + is recursive. + + """ + head, tail = path.split(name) + if not tail: + head, tail = path.split(head) + if head and tail and not path.exists(head): + try: + if recursive_mode: + makedirs(head, mode=mode, exist_ok=exist_ok, + recursive_mode=True) + else: + makedirs(head, exist_ok=exist_ok) + except FileExistsError: + # Defeats race condition when another thread created the path + pass + cdir = curdir + if isinstance(tail, bytes): + cdir = bytes(curdir, 'ASCII') + if tail == cdir: # xxx/newdir/. exists if xxx/newdir exists + return + try: + mkdir(name, mode) + except OSError: + # Cannot rely on checking for EEXIST, since the operating system + # could give priority to other errors like EACCES or EROFS + if not exist_ok or not path.isdir(name): + raise diff --git a/bsmain/utils.py b/bsmain/utils.py new file mode 100644 index 0000000..4d63ae8 --- /dev/null +++ b/bsmain/utils.py @@ -0,0 +1,102 @@ +import os +import stat +import logging + +from .future_python import makedirs + + +PRIVATE_MODE = 0o700 +GLOBALLY_WRITABLE_MASK = 0o002 + + +logger = logging.getLogger("bugsink.security") + + +class B108SecurityError(Exception): + pass + + +def b108_makedirs(path): + """ + Create (or validate) an app working directory with B108-style hardening against local privilege escalation: + + * Create without if-exists checks to avoid TOCTOU (makedirs(..., exist_ok=True)). + + * Final directory invariants: + 1. owned by the current uid + 2. private mode (700) + + * Path invariants (from the leaf up to the first root-owned ancestor, which is assumed to be secure): + 1. every segment is owned by the current uid + 2. no symlinks anywhere (somewhat redundant given "owned by us", but we're playing safe) + + This removes the risk of being redirected into unintended locations (symlink/rename tricks) and of leaking data + into attacker-controlled files or directories. + + ### Backwards compatibility notes + + On already running systems, directories may have been created with laxer permissions. We simply warn about those, + (rather than try to fix the problem) because in the general case we cannot determine where the "Bugsink boundary" + is (e.g. we wouldn't want to mess with $HOME's permissions, which is what would happen if we simply apply the "chmod + for current uid" rule all the way up). + + ### Further notes: + + * Our model for file-based attack vectors is simply: inside the 700 dir, you'll be good no matter what. In other + words: no analogous checks at the file level. + + * This function implements post-verification (i.e. "in theory it's too late"); since it operates at the dir-level we + believe "in practice it's in time" (you might trick us into writing a directory somewhere, but right after it'll + fail the check and no files will be written) + """ + makedirs(path, mode=PRIVATE_MODE, exist_ok=True, recursive_mode=True) + my_uid = os.getuid() + + # the up-the-tree checks are unconditional (cheap enough, and they guard against scenarios in which an attacker + # previously created something in the way, so we can't skip because os.makedirs says "it already exists") + + # separate from the "up-the-tree" loop b/c the target path may not be root. + st = os.lstat(path) + if st.st_uid != my_uid: + raise B108SecurityError(f"Target path owned by uid other than me: {path}") + + if (st.st_mode & 0o777) != PRIVATE_MODE: + # NOTE: warn-only to facilitate a migration doesn't undo all our hardening for post-migration/fresh installs, + # because we still check self-ownership up to root. + logger.warning( + "SECURITY: Target path does not have private mode (700): %s has mode %03o", path, st.st_mode & 0o777) + + current = path + while True: + st = os.lstat(current) + + if st.st_uid == 0: + # we stop checking once we reach a root-owned dir; at some level you'll "hit the system boundary" which is + # secure by default (or it's compromised, in which case nothing helps us). We work on the assumption that + # this boundary is correctly setup, e.g. if it's /tmp it will have the sticky bit set. + break + + if stat.S_ISLNK(st.st_mode): + raise B108SecurityError("Symlink in path at %s while creating %s" % (current, path)) + + # if not stat.S_ISDIR(st.st_mode): not needed, because os.makedirs would trigger a FileExistsError over that + + if st.st_uid != my_uid: + # (avoiding tripping over root is implied by the `break` in the above) + raise B108SecurityError("Parent directory of %s not owned by my uid or root: %s" % (path, current)) + + if (current != path) and (st.st_mode & GLOBALLY_WRITABLE_MASK): # skipped for target (more strict check above) + # note: in practice this won't trigger for "plain migrations" i.e. ones where no manual changes were made, + # because the pre-existing code created with 0o755; still: it's a good check to have. + # + # note: we don't additionally check on group-writable because we don't want to make too many assumptions + # about group setup (e.g. user private groups are common on Linux) + logger.warning("SECURITY: Parent directory of target path %s is globally writeable: %s", path, current) + + parent = os.path.dirname(current) + + if parent == current: # reached root + # weird that this would not be root-owned (break above) but I'd rather not hang indefinitely for that. + break + + current = parent diff --git a/bugsink/app_settings.py b/bugsink/app_settings.py index fe68237..5bd1ce4 100644 --- a/bugsink/app_settings.py +++ b/bugsink/app_settings.py @@ -66,7 +66,8 @@ DEFAULTS = { "MAX_HEADER_SIZE": 8 * _KIBIBYTE, # Locations of files & directories: - "INGEST_STORE_BASE_DIR": "/tmp/bugsink/ingestion", + # no_bandit_expl: the usage of this path (via get_filename_for_event_id) is protected with `b108_makedirs` + "INGEST_STORE_BASE_DIR": "/tmp/bugsink/ingestion", # nosec "EVENT_STORAGES": {}, # Security: diff --git a/bugsink/settings/development.py b/bugsink/settings/development.py index 31ea1c9..713f688 100644 --- a/bugsink/settings/development.py +++ b/bugsink/settings/development.py @@ -64,7 +64,9 @@ if not I_AM_RUNNING == "TEST": SNAPPEA = { "TASK_ALWAYS_EAGER": True, # at least for (unit) tests, this is a requirement "NUM_WORKERS": 1, - "PID_FILE": "/tmp/snappea.pid", # for development: a thing to 'tune' to None to test the no-pid-check branches. + + # no_bandit_expl: development setting, we know that this is insecure + "PID_FILE": "/tmp/snappea.pid", # nosec B108 } EMAIL_HOST = os.getenv("EMAIL_HOST") diff --git a/ingest/views.py b/ingest/views.py index 719e6e1..332999c 100644 --- a/ingest/views.py +++ b/ingest/views.py @@ -38,6 +38,7 @@ from releases.models import create_release_if_needed from alerts.tasks import send_new_issue_alert, send_regression_alert from compat.timestamp import format_timestamp, parse_timestamp from tags.models import digest_tags +from bsmain.utils import b108_makedirs from .parsers import StreamingEnvelopeParser, ParseError from .filestore import get_filename_for_event_id @@ -633,7 +634,7 @@ class IngestEnvelopeAPIView(BaseIngestAPIView): raise ParseError("event_id in envelope headers is not a valid UUID") filename = get_filename_for_event_id(envelope_headers["event_id"]) - os.makedirs(os.path.dirname(filename), exist_ok=True) + b108_makedirs(os.path.dirname(filename)) return MaxDataWriter("MAX_EVENT_SIZE", open(filename, 'wb')) # everything else can be discarded; (we don't check for individual size limits, because these differ diff --git a/snappea/foreman.py b/snappea/foreman.py index 518f95c..15e7727 100644 --- a/snappea/foreman.py +++ b/snappea/foreman.py @@ -19,6 +19,7 @@ from django.utils._os import safe_join from sentry_sdk_extensions import capture_or_log_exception from performance.context_managers import time_to_logger from bugsink.transaction import durable_atomic, get_stat +from bsmain.utils import b108_makedirs from . import registry from .models import Task @@ -88,8 +89,7 @@ class Foreman: signal.signal(signal.SIGTERM, self.handle_signal) # We use inotify to wake up the Foreman when a new Task is created. - if not os.path.exists(self.settings.WAKEUP_CALLS_DIR): - os.makedirs(self.settings.WAKEUP_CALLS_DIR, exist_ok=True) + b108_makedirs(self.settings.WAKEUP_CALLS_DIR) self.wakeup_calls = INotify() self.wakeup_calls.add_watch(self.settings.WAKEUP_CALLS_DIR, flags.CREATE) @@ -145,6 +145,7 @@ class Foreman: # as per the above: not bullet proof, and non-critical, hence also: not a reason to crash on this. logger.error("Startup: Ignored Error while checking PID file", exc_info=e) + # Note: no b108_makedirs here yet, because we can't assume a self-owned containing directory (see #195) os.makedirs(os.path.dirname(self.settings.PID_FILE), exist_ok=True) with open(self.settings.PID_FILE, "w") as f: f.write(str(pid)) diff --git a/snappea/models.py b/snappea/models.py index 12bbca3..c2895ad 100644 --- a/snappea/models.py +++ b/snappea/models.py @@ -2,6 +2,7 @@ import os from django.db import models from django.utils._os import safe_join +from bsmain.utils import b108_makedirs from .settings import get_settings from . import thread_uuid @@ -47,8 +48,7 @@ class Stat(models.Model): def wakeup_server(): wakeup_file = safe_join(get_settings().WAKEUP_CALLS_DIR, thread_uuid) - if not os.path.exists(get_settings().WAKEUP_CALLS_DIR): - os.makedirs(get_settings().WAKEUP_CALLS_DIR, exist_ok=True) + b108_makedirs(get_settings().WAKEUP_CALLS_DIR) if not os.path.exists(wakeup_file): with open(wakeup_file, "w"): diff --git a/snappea/settings.py b/snappea/settings.py index 5d8c3e6..07f933e 100644 --- a/snappea/settings.py +++ b/snappea/settings.py @@ -4,8 +4,11 @@ from django.conf import settings DEFAULTS = { "TASK_ALWAYS_EAGER": False, - "PID_FILE": "/tmp/snappea.pid", - "WAKEUP_CALLS_DIR": "/tmp/snappea.wakeup", + # no_bandit_expl: monitored in #195 + "PID_FILE": "/tmp/snappea.pid", # nosec + + # no_bandit_expl: the usage of this path (in the foreman) is protected with `b108_makedirs` + "WAKEUP_CALLS_DIR": "/tmp/snappea.wakeup", # nosec "NUM_WORKERS": 4,