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

@@ -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
 

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:
 

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

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

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:

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")

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

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))

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"):

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,