fix(security): enigma review findings CTO-4878/4881/4882/4884/4885/4886/4889

.github/workflows/pre-commit.yml

@@ -13,5 +13,4 @@ jobs:
       run: |
         pip install pre-commit==2.21.0
         pre-commit install --install-hooks --overwrite --config .pre-commit-config-gh-action.yaml
-        pre-commit autoupdate --config .pre-commit-config-gh-action.yaml
         pre-commit run --all-files --show-diff-on-failure --color always --config .pre-commit-config-gh-action.yaml

.github/workflows/unit-tests.yml

@@ -26,7 +26,7 @@ jobs:
       run: python -m scripts.clone_access_modules
     - name: Ensure access modules dependencies
       run: |
-        pip install -r Access/access_modules/requirements.txt --no-cache-dir --ignore-installed
+        pip install -r Access/access_modules/requirements.txt -c requirements.txt --no-cache-dir
     - name: Lint code base
       run: |
         pip install "setuptools<81"

.pre-commit-config-gh-action.yaml

@@ -2,7 +2,7 @@
 # See https://pre-commit.com/hooks.html for more hooks
 repos:
 - repo: https://github.com/pre-commit/pre-commit-hooks
-  rev: v4.4.0
+  rev: f71fa2c1f9cf5cb705f73dffe4b21f7c61470ba9  # v4.4.0
   hooks:
   - id: check-merge-conflict
   - id: trailing-whitespace
@@ -19,7 +19,7 @@ repos:
     files: \.(json|json.sample)$
     args: [--autofix, --no-sort-keys]
 - repo: https://github.com/macisamuele/language-formatters-pre-commit-hooks
-  rev: v2.6.0
+  rev: 4a72877d96875c49769109cf7881b44b8ad4e423  # v2.6.0
   hooks:
   - id: pretty-format-yaml
     args: [--autofix]

Access/tests/test_security_batch3.py

@@ -0,0 +1,96 @@
+""" regression tests: F-033 mark_revoked status, F-034 password history, F-036 CID """
+import re
+from types import SimpleNamespace
+from unittest import mock
+import pytest
+from django.contrib.auth.models import User as AuthUser
+from django.contrib.auth.hashers import make_password
+from django.test import RequestFactory
+from Access import views
+from Access.models import StoredPassword
+from Access.validators import RepeatedValidator
+from EnigmaAutomation import settings
+
+
+def _ops_request(query):
+    request = RequestFactory().get("/access/markRevoked", query)
+    perm = SimpleNamespace(label="ALLOW_USER_OFFBOARD")
+    request.user = mock.MagicMock()
+    request.user.is_authenticated = True
+    request.user.user.permissions = [perm]
+    return request
+
+
+def test_mark_revoked_success_returns_200():
+    """ F-033: success must not be reported as 403 """
+    mapping = mock.MagicMock(request_id="r1")
+    with mock.patch.object(
+            views.UserAccessMapping, "get_unrevoked_accesses_by_request_id",
+            return_value=[mapping]):
+        response = views.mark_revoked(_ops_request({"requestId": "r1"}))
+    assert response.status_code == 200
+    mapping.revoke.assert_called_once()
+
+
+def test_mark_revoked_failure_returns_500():
+    """ an exception during revoke is an error status, not 200 """
+    with mock.patch.object(
+            views.UserAccessMapping, "get_unrevoked_accesses_by_request_id",
+            side_effect=RuntimeError("boom")):
+        response = views.mark_revoked(_ops_request({"requestId": "r1"}))
+    assert response.status_code == 500
+
+
+def test_mark_revoked_module_without_username_is_400():
+    """ missing username on a module revoke is a client error """
+    response = views.mark_revoked(_ops_request({"requestId": "module-ssh"}))
+    assert response.status_code == 400
+
+
+@pytest.fixture(name="auth_user")
+def fixture_auth_user(db):
+    """ django auth user """
+    del db
+    return AuthUser.objects.create(username="pwuser")
+
+
+def _store(user, *passwords):
+    for pw in passwords:
+        StoredPassword.objects.create(
+            user=user, password=make_password(pw, hasher="pbkdf2_sha256"))
+
+
+@pytest.mark.parametrize("history", [1, 3, 4])
+def test_recent_password_is_rejected_even_with_short_history(auth_user, history):
+    """ F-034: [:3:-1] skipped the check when <=4 passwords were stored """
+    _store(auth_user, *[f"Old-pass-{i}!" for i in range(history)])
+    from django.core.exceptions import ValidationError
+    with pytest.raises(ValidationError):
+        RepeatedValidator().validate(f"Old-pass-{history - 1}!", auth_user)
+
+
+def test_new_password_is_accepted(auth_user):
+    """ a password not in the history passes """
+    _store(auth_user, "Old-pass-0!")
+    assert RepeatedValidator().validate("Brand-new-1!", auth_user) is None
+
+
+def test_password_older_than_three_is_allowed(auth_user):
+    """ only the last 3 are blocked """
+    _store(auth_user, "p-a-1!", "p-b-2!", "p-c-3!", "p-d-4!")
+    assert RepeatedValidator().validate("p-a-1!", auth_user) is None
+
+
+def test_password_changed_does_not_store_duplicates(auth_user):
+    """ the same password saved twice must produce one row """
+    validator = RepeatedValidator()
+    validator.password_changed("Same-pass-1!", auth_user)
+    validator.password_changed("Same-pass-1!", auth_user)
+    assert StoredPassword.objects.filter(user=auth_user).count() == 1
+
+
+def test_cid_generator_is_unpredictable_and_unique():
+    """ F-036: CID comes from uuid4 (os.urandom), not random.random() """
+    ids = {settings.CID_GENERATOR() for _ in range(200)}
+    assert len(ids) == 200
+    assert all(re.fullmatch(r"[0-9a-f]{32}", i) for i in ids)

Access/validators.py

@@ -53,7 +53,7 @@ def validate(self, password, user=None):
         if user is None:
             return None
 
-        user_list = StoredPassword.objects.filter(user=user).order_by('-id')[:3:-1]
+        user_list = StoredPassword.objects.filter(user=user).order_by('-id')[:3]
         password_list = []
         for user in user_list:
             password_list.append(user.password)
@@ -70,13 +70,18 @@ def password_changed(self, password, user=None):
         if user is None:
             return None
 
-        hashed_password = make_password(password, salt=None, hasher='pbkdf2_sha256')
-        saved_password = StoredPassword.objects.filter(user=user, password=hashed_password).first()
-        if saved_password is None:
-            saved_password = StoredPassword()
-            saved_password.user = user
-            saved_password.password = hashed_password
-            saved_password.save()
+        # make_password salts randomly, so the hash can never be matched by a
+        # lookup; compare against the most recent stored hash instead.
+        latest = StoredPassword.objects.filter(user=user).order_by('-id').first()
+        if latest is not None and check_password(
+                password, latest.password, setter=None,
+                preferred='pbkdf2_sha256'):
+            return None
+        saved_password = StoredPassword()
+        saved_password.user = user
+        saved_password.password = make_password(
+            password, salt=None, hasher='pbkdf2_sha256')
+        saved_password.save()
 
     def get_help_text(self):
         return _(

Access/views.py

@@ -608,7 +608,7 @@ def mark_revoked(request):
             username = request.GET.get("username")
             if not username:
                 json_response["error"] = "Username is invalid!"
-                status = 403
+                status = 400
                 return JsonResponse(json_response, status=status)
             access_tag = request_id.split("-", 1)[1]
             user = User.get_user_from_username(username=username)
@@ -636,7 +636,8 @@ def mark_revoked(request):
     except Exception as ex:
         logger.exception("Error Revoking User Access, Error: %s", traceback.format_exc())
         json_response["error"] = "Error Revoking User Access"
-    return JsonResponse(json_response, status=403)
+        status = 500
+    return JsonResponse(json_response, status=status)
 
 
 @login_required

Dockerfile

@@ -9,7 +9,7 @@ ENV DJANGO_SETTINGS_MODULE=EnigmaAutomation.settings
 RUN DEBIAN_FRONTEND=noninteractive \
   apt-get update -y \
   && apt-get install --no-install-recommends -y \
-  openssh-client curl procps git vim gcc linux-libc-dev libc6-dev build-essential \
+  openssh-client procps git gcc linux-libc-dev libc6-dev build-essential \
   && apt-get clean \
   && apt-get autoremove -y
 

EnigmaAutomation/settings.py

@@ -15,8 +15,7 @@
 import json
 from pathlib import Path
 import os
-import time
-import random
+import uuid
 import django
 from django.utils.translation import gettext
 django.utils.translation.ugettext = gettext
@@ -90,7 +89,7 @@
     "axes",
 ]
 CID_GENERATE = True
-CID_GENERATOR = lambda: f"{time.time()}-{random.random()}"
+CID_GENERATOR = lambda: uuid.uuid4().hex
 CID_HEADER = "X_CORRELATION_ID"
 CID_GENERATE = True
 CID_CONCATENATE_IDS = True