From 2da760de029bfa46d1178fdac9c06e6a5c316109 Mon Sep 17 00:00:00 2001 From: rookiestar28 Date: Wed, 8 Apr 2026 02:22:17 +0800 Subject: [PATCH] fix(security): remove residual token hash indexing --- services/audit.py | 11 +++---- services/bridge_token_lifecycle.py | 50 +++++++++--------------------- tests/security/test_s79_hashing.py | 36 +++++++++------------ tests/test_r108_security_matrix.py | 18 +++++------ 4 files changed, 42 insertions(+), 73 deletions(-) diff --git a/services/audit.py b/services/audit.py index e6158b6..ec9e7ac 100644 --- a/services/audit.py +++ b/services/audit.py @@ -4,7 +4,6 @@ Standardized, append-only audit events for sensitive operations. """ import hashlib -import hmac import json import logging import os @@ -167,12 +166,12 @@ def _chain_hash(prev_hash: str, entry: Dict[str, Any]) -> str: payload = json.dumps( entry, sort_keys=True, separators=(",", ":"), ensure_ascii=True ) - # IMPORTANT: keep audit-chain hashing keyed; plain SHA-256 on sensitive events - # triggers CodeQL and weakens correlation resistance. - return hmac.new( - _get_audit_chain_key(), + # IMPORTANT: keep audit-chain hashing keyed. Residual CodeQL still treated + # the earlier SHA-256-based construction as weak sensitive hashing here. + return hashlib.blake2b( f"{prev_hash}|{payload}".encode("utf-8"), - hashlib.sha256, + key=_get_audit_chain_key(), + digest_size=32, ).hexdigest() diff --git a/services/bridge_token_lifecycle.py b/services/bridge_token_lifecycle.py index 23d2d4d..d4b04c6 100644 --- a/services/bridge_token_lifecycle.py +++ b/services/bridge_token_lifecycle.py @@ -11,13 +11,12 @@ Security properties: - Lifecycle decisions are deterministic and auditable """ -import hmac import json import logging import os import secrets import time -from dataclasses import asdict, dataclass, field +from dataclasses import dataclass, field from pathlib import Path from typing import Any, Dict, List, Optional, Tuple @@ -93,7 +92,7 @@ class BridgeTokenStore: """ In-memory + persisted bridge token registry. - All tokens are indexed by token_value (HMAC-safe lookup) and by token_id. + All tokens are stored by token_id and resolved by bounded constant-time scan. Persistence is optional (state_dir may be None for test usage). """ @@ -102,11 +101,9 @@ class BridgeTokenStore: def __init__(self, state_dir: Optional[str] = None): self._tokens: Dict[str, DeviceToken] = {} # token_id → DeviceToken - self._token_index: Dict[str, str] = {} # token_value_hash → token_id self._audit_trail: List[TokenAuditEvent] = [] self._state_dir = state_dir self._store_path: Optional[Path] = None - self._token_index_key = self._build_token_index_key() if state_dir: self._store_path = Path(state_dir) / "bridge_tokens.json" self._load() @@ -133,8 +130,6 @@ class BridgeTokenStore: overlap_until=td.get("overlap_until"), ) self._tokens[token.token_id] = token - h = self._hash_token(token.device_token) - self._token_index[h] = token.token_id logger.info(f"S58: Loaded {len(self._tokens)} bridge tokens") except Exception as e: logger.error(f"S58: Failed to load bridge tokens: {e}") @@ -170,24 +165,17 @@ class BridgeTokenStore: except Exception as e: logger.error(f"S58: Failed to persist bridge tokens: {e}") - # --- Token hashing --- + # --- Token lookup --- - def _build_token_index_key(self) -> bytes: - raw = os.environ.get("OPENCLAW_BRIDGE_TOKEN_INDEX_KEY") or os.environ.get( - "MOLTBOT_BRIDGE_TOKEN_INDEX_KEY" - ) - if raw: - return raw.encode("utf-8") - return secrets.token_bytes(32) - - def _hash_token(self, token_value: str) -> str: - """Constant-time-safe hash for token lookup.""" - # IMPORTANT: never fall back to a hardcoded token-index key here. - return hmac.new( - self._token_index_key, - token_value.encode("utf-8"), - "sha256", - ).hexdigest() + def _resolve_token_for_value( + self, token_value: str + ) -> Tuple[Optional[str], Optional[DeviceToken]]: + # IMPORTANT: keep lookup on bounded constant-time comparison instead of + # a derived hash index; hashing the presented token is the residual sink. + for token_id, token in self._tokens.items(): + if secrets.compare_digest(token.device_token, token_value): + return token_id, token + return None, None # --- Audit --- @@ -248,7 +236,6 @@ class BridgeTokenStore: ) self._tokens[token_id] = token - self._token_index[self._hash_token(token_value)] = token_id self._emit_audit("issue", token_id, device_id, ttl_sec=ttl_sec) self._save() @@ -343,15 +330,10 @@ class BridgeTokenStore: Returns TokenValidationResult with ok, reject_reason, and token metadata. """ - h = self._hash_token(token_value) - token_id = self._token_index.get(h) - if not token_id: + token_id, token = self._resolve_token_for_value(token_value) + if token_id is None or token is None: return TokenValidationResult(ok=False, reject_reason="unknown_token") - token = self._tokens.get(token_id) - if not token: - return TokenValidationResult(ok=False, reject_reason="token_not_found") - now = time.time() # Check revocation (immediate, non-negotiable) @@ -436,9 +418,7 @@ class BridgeTokenStore: elif token.overlap_until and now > token.overlap_until: to_remove.append(tid) for tid in to_remove: - token = self._tokens.pop(tid) - h = self._hash_token(token.device_token) - self._token_index.pop(h, None) + self._tokens.pop(tid) if to_remove: self._save() logger.info(f"S58: Cleaned up {len(to_remove)} expired/revoked tokens") diff --git a/tests/security/test_s79_hashing.py b/tests/security/test_s79_hashing.py index cae084b..88d9abe 100644 --- a/tests/security/test_s79_hashing.py +++ b/tests/security/test_s79_hashing.py @@ -1,4 +1,3 @@ -import os import unittest from unittest.mock import patch @@ -29,27 +28,22 @@ class TestS79AuditHashing(unittest.TestCase): class TestS79BridgeTokenHashing(unittest.TestCase): - def tearDown(self): - os.environ.pop("OPENCLAW_BRIDGE_TOKEN_INDEX_KEY", None) - os.environ.pop("MOLTBOT_BRIDGE_TOKEN_INDEX_KEY", None) + def test_constant_time_lookup_accepts_issued_token(self): + store = BridgeTokenStore() + token = store.issue_token("device-1") - def test_token_hash_differs_across_store_instances_without_override(self): - store_a = BridgeTokenStore() - store_b = BridgeTokenStore() + result = store.validate_token(token.device_token) - self.assertNotEqual( - store_a._hash_token("secret-token"), store_b._hash_token("secret-token") - ) + self.assertTrue(result.ok) + self.assertEqual(result.token.token_id, token.token_id) - def test_token_hash_can_be_pinned_via_env_override(self): - with patch.dict( - os.environ, - {"OPENCLAW_BRIDGE_TOKEN_INDEX_KEY": "fixed-token-index-key"}, - clear=False, - ): - store_a = BridgeTokenStore() - store_b = BridgeTokenStore() + def test_lookup_does_not_rely_on_hash_index_internals(self): + store = BridgeTokenStore() + token = store.issue_token("device-1") + token.status = "active" + store._tokens[token.token_id] = token - self.assertEqual( - store_a._hash_token("secret-token"), store_b._hash_token("secret-token") - ) + token_id, resolved = store._resolve_token_for_value(token.device_token) + + self.assertEqual(token_id, token.token_id) + self.assertIs(resolved, token) diff --git a/tests/test_r108_security_matrix.py b/tests/test_r108_security_matrix.py index 33e0931..25ca7e2 100644 --- a/tests/test_r108_security_matrix.py +++ b/tests/test_r108_security_matrix.py @@ -74,9 +74,10 @@ class TestR108SecurityMatrix(unittest.TestCase): # Helper to inject token state def inject_token(tid, status, expires, overlap=None, scopes=None): + token_value = f"secret-{tid}" t = DeviceToken( device_id="dev-1", - device_token="secret", + device_token=token_value, scopes=scopes or [BridgeScope.JOB_STATUS], expires_at=expires, token_id=tid, @@ -84,15 +85,8 @@ class TestR108SecurityMatrix(unittest.TestCase): status=status, overlap_until=overlap, ) - store._active_tokens_mock = { - tid: t - } # Bypass hashing for direct injection if possible? - # Store uses _tokens by ID and _token_index by hash. - # We must use proper injection. store._tokens[tid] = t - h = store._hash_token("secret") - store._token_index[h] = tid - return t + return t, token_value cases = [ # Case Name | Status | Expires Relative | Overlap Relative | Req Scope | Expect OK | Expect Reason @@ -168,10 +162,12 @@ class TestR108SecurityMatrix(unittest.TestCase): overlap = (now + overlap_rel) if overlap_rel else None scopes = [BridgeScope.JOB_STATUS] - inject_token(f"t_{case_name}", status, expires, overlap, scopes) + _token, token_value = inject_token( + f"t_{case_name}", status, expires, overlap, scopes + ) # Act - res = store.validate_token("secret", required_scope=scope) + res = store.validate_token(token_value, required_scope=scope) # Assert self.assertEqual(res.ok, expect_ok, f"Case {case_name}: OK mismatch")