Prevent session key prefix corruption across workers

.semgrep/session-interface-immutable.yml

@@ -0,0 +1,19 @@
+rules:
+  - id: session-interface-mutation-outside-init
+    patterns:
+      - pattern: self.$ATTR = $VAL
+      - pattern-inside: |
+          class SessionInterface(...):
+              ...
+      - pattern-not-inside: |
+          def __init__(self, ...):
+              ...
+    message: >
+      SessionInterface.$ATTR is mutated outside of __init__.
+      SessionInterface should be immutable after construction; use local
+      variables for per-request state instead.
+    languages: [python]
+    severity: ERROR
+    paths:
+      include:
+        - "**/journalist_app/sessions.py"

Makefile

@@ -127,12 +127,12 @@ zizmor:  ## Lint GitHub Actions workflows.
 lint: check-ruff ansible-config-lint app-lint html-lint shellcheck typelint yamllint zizmor check-strings check-supported-locales check-desktop-files ## Runs all lint checks
 
 # Semgrep is a static code analysis tool to detect security vulnerabilities in Python applications
-# This configuration uses the public "p/r2c-security-audit" ruleset
+# This configuration uses the public "p/r2c-security-audit" ruleset and local custom rules
 .PHONY: semgrep
 semgrep:
 	@command -v semgrep || (echo "Please run 'pip install -U semgrep'."; exit 1)
 	@echo "███ Running semgrep on securedrop/..."
-	@semgrep --exclude "securedrop/tests/" --error --strict --metrics off --max-chars-per-line 200 --verbose --config "p/r2c-security-audit" securedrop
+	@semgrep --exclude "securedrop/tests/" --error --strict --metrics off --max-chars-per-line 200 --verbose --config "p/r2c-security-audit" --config ".semgrep/" securedrop
 	@echo
 
 

securedrop/journalist_app/sessions.py

@@ -2,7 +2,7 @@
 from datetime import UTC, datetime, timedelta
 from json.decoder import JSONDecodeError
 from secrets import token_urlsafe
-from typing import Any
+from typing import Any, Final
 
 from flask import Flask, Request, Response
 from flask import current_app as app
@@ -19,7 +19,9 @@
 class ServerSideSession(CallbackDict, SessionMixin):
     """Baseclass for server-side based sessions."""
 
-    def __init__(self, sid: str, token: str, lifetime: int = 0, initial: Any = None) -> None:
+    def __init__(
+        self, sid: str, token: str, lifetime: int = 0, initial: Any = None, is_api: bool = False
+    ) -> None:
         def on_update(self: ServerSideSession) -> None:
             self.modified = True
 
@@ -33,7 +35,7 @@ def on_update(self: ServerSideSession) -> None:
         self.sid = sid
         self.token: str = token
         self.lifetime = lifetime
-        self.is_api = False
+        self.is_api = is_api
         self.to_destroy = False
         self.to_regenerate = False
         self.modified = False
@@ -80,13 +82,15 @@ def regenerate(self) -> None:
 
 
 class SessionInterface(FlaskSessionInterface):
+    """Class is required to be immutable for consistent state across mod_wsgi workers."""
+
     def _generate_sid(self) -> str:
         return token_urlsafe(32)
 
-    def _get_signer(self, app: Flask) -> URLSafeTimedSerializer:
+    def _get_signer(self, app: Flask, salt: str) -> URLSafeTimedSerializer:
         if not app.secret_key:
             raise RuntimeError("No secret key set")
-        return URLSafeTimedSerializer(app.secret_key, salt=self.salt)
+        return URLSafeTimedSerializer(app.secret_key, salt=salt)
 
     """Uses the Redis key-value store as a session backend.
 
@@ -109,79 +113,89 @@ def __init__(
         self.redis = redis
         self.lifetime = lifetime
         self.renew_count = renew_count
-        self.key_prefix = key_prefix
-        self.api_key_prefix = "api_" + key_prefix
-        self.salt = salt
-        self.api_salt = "api_" + salt
+
+        # set "immutable" session base parameters
+        self.web_key_prefix: Final = key_prefix
+        self.web_salt: Final = salt
+
+        self.api_key_prefix: Final = "api_" + key_prefix
+        self.api_salt: Final = "api_" + salt
+
         self.header_name = header_name
         self.new = False
 
-    def _new_session(self, is_api: bool = False, initial: Any = None) -> ServerSideSession:
+    def _new_session(
+        self, salt: str, is_api: bool = False, initial: Any = None
+    ) -> ServerSideSession:
         sid = self._generate_sid()
-        token: str = self._get_signer(app).dumps(sid)  # type: ignore
-        session = ServerSideSession(sid=sid, token=token, lifetime=self.lifetime, initial=initial)
+        token: str = self._get_signer(app, salt).dumps(sid)  # type: ignore
+        session = ServerSideSession(
+            sid=sid, token=token, lifetime=self.lifetime, initial=initial, is_api=is_api
+        )
         session.new = True
-        session.is_api = is_api
         return session
 
     def open_session(self, app: Flask, request: Request) -> ServerSideSession | None:
         """This function is called by the flask session interface at the
         beginning of each request.
         """
         is_api = request.path.split("/")[1] == "api"
+        key_prefix = self.api_key_prefix if is_api else self.web_key_prefix
+        salt = self.api_salt if is_api else self.web_salt
 
         if is_api:
-            self.key_prefix = self.api_key_prefix
-            self.salt = self.api_salt
             auth_header = request.headers.get(self.header_name)
             if auth_header:
                 split = auth_header.split(" ")
                 if len(split) != 2 or split[0] != "Token":
-                    return self._new_session(is_api)
+                    return self._new_session(salt, is_api=is_api)
                 sid: str | None = split[1]
             else:
-                return self._new_session(is_api)
+                return self._new_session(salt, is_api=is_api)
         else:
             sid = request.cookies.get(app.session_cookie_name)
         if sid:
             try:
-                sid = self._get_signer(app).loads(sid)
+                sid = self._get_signer(app, salt).loads(sid)
             except BadSignature:
                 sid = None
         if not sid:
-            return self._new_session(is_api)
+            return self._new_session(salt, is_api=is_api)
 
-        val = self.redis.get(self.key_prefix + sid)
+        val = self.redis.get(key_prefix + sid)
         if val is not None:
             try:
                 data = self.serializer.loads(val.decode("utf-8"))
-                token: str = self._get_signer(app).dumps(sid)  # type: ignore
-                return ServerSideSession(sid=sid, token=token, initial=data)
+                token: str = self._get_signer(app, salt).dumps(sid)  # type: ignore
+                return ServerSideSession(sid=sid, token=token, initial=data, is_api=is_api)
             except (JSONDecodeError, NotImplementedError):
-                return self._new_session(is_api)
+                return self._new_session(salt, is_api=is_api)
         # signed session_id provided in cookie is valid, but the session is not on the server
         # anymore so maybe here is the code path for a meaningful error message
         msg = gettext("You have been logged out due to inactivity.")
-        return self._new_session(is_api, initial={"_flashes": [("error", msg)]})
+        return self._new_session(salt, is_api=is_api, initial={"_flashes": [("error", msg)]})
 
     def save_session(  # type: ignore[override]
         self, app: Flask, session: ServerSideSession, response: Response
     ) -> None:
         """This is called at the end of each request, just
         before sending the response.
         """
+        key_prefix = self.api_key_prefix if session.is_api else self.web_key_prefix
+        salt = self.api_salt if session.is_api else self.web_salt
+
         domain = self.get_cookie_domain(app)
         path = self.get_cookie_path(app)
         if session.to_destroy:
             initial: dict[str, Any] = {"locale": session.locale}
             if session.flash:
                 initial["_flashes"] = [session.flash]
-            self.redis.delete(self.key_prefix + session.sid)
+            self.redis.delete(key_prefix + session.sid)
             if not session.is_api:
                 # Instead of deleting the cookie and send a new sid with the next request
                 # create the new session already, so we can pass along messages and locale
-                session = self._new_session(False, initial=initial)
-        expires = self.redis.ttl(name=self.key_prefix + session.sid)
+                session = self._new_session(salt, is_api=False, initial=initial)
+        expires = self.redis.ttl(name=key_prefix + session.sid)
         if session.new:
             session["renew_count"] = self.renew_count
             expires = self.lifetime
@@ -194,15 +208,15 @@ def save_session(  # type: ignore[override]
         samesite = self.get_cookie_samesite(app)
         val = self.serializer.dumps(dict(session))
         if session.to_regenerate:
-            self.redis.delete(self.key_prefix + session.sid)
+            self.redis.delete(key_prefix + session.sid)
             session.sid = self._generate_sid()
-            session.token = self._get_signer(app).dumps(session.sid)  # type: ignore
+            session.token = self._get_signer(app, salt).dumps(session.sid)  # type: ignore
         if session.new or session.to_regenerate:
-            self.redis.setex(name=self.key_prefix + session.sid, value=val, time=expires)
+            self.redis.setex(name=key_prefix + session.sid, value=val, time=expires)
         elif session.modified:
             # To prevent race conditions where session is delete by an admin in the middle of a req
             # accept to save the session object if and only if already exists using the xx flag
-            self.redis.set(name=self.key_prefix + session.sid, value=val, ex=expires, xx=True)
+            self.redis.set(name=key_prefix + session.sid, value=val, ex=expires, xx=True)
         if not session.is_api and (session.new or session.to_regenerate):
             response.headers.add("Vary", "Cookie")
             response.set_cookie(
@@ -216,8 +230,8 @@ def save_session(  # type: ignore[override]
             )
 
     def logout_user(self, uid: int) -> None:
-        for key in self.redis.keys(self.key_prefix + "*") + self.redis.keys(
-            "api_" + self.key_prefix + "*"
+        for key in self.redis.keys(self.web_key_prefix + "*") + self.redis.keys(
+            self.api_key_prefix + "*"
         ):
             found = self.redis.get(key)
             if found:

securedrop/tests/test_journalist_session.py

@@ -347,6 +347,120 @@ def test_session_api_logout(journalist_app, test_journo, redis):
         assert resp.status_code == 403
 
 
+# test a standard login from API, then requests to the web interface, which should fail
+def test_session_api_breakout(journalist_app, test_journo, redis):
+    # Given a test client and a valid journalist user
+    with journalist_app.test_client() as app:
+        # When sending a valid login request and asking an API token
+        resp = app.post(
+            url_for("api.get_token"),
+            data=json.dumps(
+                {
+                    "username": test_journo["username"],
+                    "passphrase": test_journo["password"],
+                    "one_time_code": TOTP(test_journo["otp_secret"]).now(),
+                }
+            ),
+            headers=get_api_headers(),
+        )
+
+        # Then the token is issued successfully with the correct attributed
+        assert resp.json["journalist_uuid"] == test_journo["uuid"]
+        assert resp.status_code == 200
+        token = resp.json["token"]
+        sid = _check_sig(token, journalist_app, api=True)
+
+        # When querying the web index
+        resp = app.get(url_for("main.index"), headers=get_api_headers(token))
+        # Then the request fails and the user is redirected to the login page
+        assert resp.status_code == 302
+        assert (redis.get(journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is None
+
+        # API session is still present:
+        assert (redis.get("api_" + journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is not None
+
+        # When sending a logout request using the API token
+        resp = app.post(url_for("api.logout"), headers=get_api_headers(token))
+        # Then it is successful
+        assert resp.status_code == 200
+        # Then the token and the corresponding payload no longer exist in redis
+        assert (redis.get("api_" + journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is None
+
+        # When sending an authenticated request with the deleted token
+        resp = app.get(url_for("api.get_current_user"), headers=get_api_headers(token))
+        # Then the request is unsuccessful
+        assert resp.status_code == 403
+
+
+# Test that session has correct prefix following web login/logout then api login sequence
+def test_session_prefix_consistent(journalist_app, test_journo, redis):
+    # Given a test client and a valid journalist user
+    with journalist_app.test_client() as app:
+        # When sending a correct login request
+        login_journalist(
+            app, test_journo["username"], test_journo["password"], test_journo["otp_secret"]
+        )
+        # Then check session
+        session_cookie = _session_from_cookiejar(app.cookie_jar, journalist_app)
+        assert session_cookie is not None
+
+        sid = _check_sig(session_cookie.value, journalist_app)
+        assert (redis.get(journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is not None
+
+        # When sending a logout request from a logged in journalist
+        resp = app.post(url_for("main.logout"), follow_redirects=False)
+        # Then it redirects to login
+        assert resp.status_code == 302
+        # Then the session no longer exists in redis
+        assert (redis.get(journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is None
+
+        # Then a request to the index redirects back to login
+        resp = app.get(url_for("main.index"), follow_redirects=False)
+        assert resp.status_code == 302
+
+        # When sending a valid login request and asking an API token
+        resp = app.post(
+            url_for("api.get_token"),
+            data=json.dumps(
+                {
+                    "username": test_journo["username"],
+                    "passphrase": test_journo["password"],
+                    "one_time_code": TOTP(test_journo["otp_secret"]).now(),
+                }
+            ),
+            headers=get_api_headers(),
+        )
+
+        # Then the token is issued successfully with the correct attributed
+        assert resp.json["journalist_uuid"] == test_journo["uuid"]
+        assert resp.status_code == 200
+        token = resp.json["token"]
+        sid = _check_sig(token, journalist_app, api=True)
+
+        # the API redis key exists
+        assert (redis.get("api_" + journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is not None
+        # but its web counterpart does not
+        assert (redis.get(journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is None
+
+        # And vice versa: When sending a logout request using the API token
+        resp = app.post(url_for("api.logout"), headers=get_api_headers(token))
+        # Then it is successful
+        assert resp.status_code == 200
+        # Then the token and the corresponding payload no longer exist in redis
+        assert (redis.get("api_" + journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is None
+        # then logging in via the web again...
+        login_journalist(
+            app, test_journo["username"], test_journo["password"], test_journo["otp_secret"]
+        )
+        # Then check session
+        session_cookie = _session_from_cookiejar(app.cookie_jar, journalist_app)
+        assert session_cookie is not None
+
+        sid = _check_sig(session_cookie.value, journalist_app)
+        assert (redis.get(journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is not None
+        assert (redis.get("api_" + journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is None
+
+
 # Test a few cases of valid session token with bad signatures
 def test_session_bad_signature(journalist_app, test_journo):
     # Given a test client and a valid journalist user