diff --git a/CHANGELOG.md b/CHANGELOG.md index cf6c0f36..5d12d661 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ Format: [Semantic Versioning](https://semver.org/). Spec versions follow `MAJOR. ### Fixed +- **`sign.verify_record()` now validates `max_age_seconds` and `max_future_skew_seconds` the same way `provenance.verify_record()` does.** `provenance.verify_record()` rejects a malformed freshness bound via `_check_seconds()`, added per the review on #164. `sign.verify_record()`, the original Trust Record verifier, never received the same hardening: it checked only `max_future_skew_seconds < 0`, and `max_age_seconds` was compared against unvalidated. Passing `max_age_seconds=-1` — a value a caller might use meaning "no bound," since `None` is the documented way to disable the check — rejected every record, including one issued the same second, as `record is stale`, naming the record rather than the misconfigured argument. `_check_seconds()` is now defined once in `sign.py` and shared: `sign.verify_record()` calls it directly, and `provenance.verify_record()` imports it, passing its own `ProvenanceError` via a new `exc` parameter so each keeps its existing public error type. + - **`provenance.tool_catalog_hash()` now refuses a malformed `tools` list instead of crashing.** Reached primarily through `check_tool_catalog(record, tools)`, the function the module's own docstring calls "the step that catches a live attack," because `tools` there is what the MCP server actually returned, i.e. the untrusted party this check exists to catch. The function iterated `tools` and called `.get(...)` on each entry with no check that `tools` was a list or that its entries were objects, so a malformed response (an entry that is a string, `None`, a number, or `tools` itself not being a list) raised an unhandled `AttributeError` or `TypeError` instead of the documented `ProvenanceError`. A server that is misbehaving maliciously or just buggily and is exactly the source `tools` has no reason to trust its shape. Fixed with an explicit `isinstance` check on `tools` and on each of its entries, naming the offending index. - **`content_marking.build_assertion()` and `content_marking.verify_assertion()` now refuse record bytes that aren't a JSON object, instead of crashing.** Both functions called `.get(...)` on the result of `json.loads(record_bytes)` without checking it was a dict first. Valid JSON is not always an object that is an array, a string, a number, `null`, and a bool are all valid top-level JSON and `record_bytes` is exactly the kind of externally-sourced input this is likely to happen to: `build_assertion()` takes whatever bytes a caller hands it, and `verify_assertion()`'s docstring is explicit that its `record_bytes` are "the record bytes actually retrieved from its URL," i.e. a network response the caller does not control. Either function raised an unhandled `AttributeError` instead of the documented `ContentMarkingError`. `verify_assertion()` had a second, related gap: its second parse of `record_bytes` (after the hash check) was not wrapped in the try/except its first parse-adjacent check uses, so genuinely malformed (non-JSON) bytes that happened to hash-match raised `json.JSONDecodeError` instead of `ContentMarkingError` too. Both functions now check `isinstance(record, dict)` after parsing, and `verify_assertion()`'s second parse now catches `ValueError` the same way its hash-computation path already implicitly required valid bytes to reach. diff --git a/src/agentrust_trace/sign.py b/src/agentrust_trace/sign.py index f214cd71..5664d336 100644 --- a/src/agentrust_trace/sign.py +++ b/src/agentrust_trace/sign.py @@ -276,7 +276,28 @@ def _b64url_decode(value: str, *, field: str) -> bytes: return base64.urlsafe_b64decode(padded) except (binascii.Error, ValueError) as exc: raise ValueError(f"{field} is not valid base64url: {exc}") from exc - +def _check_seconds( + name: str, value: Any, *, optional: bool = False, exc: type[Exception] = ValueError +) -> None: + """Reject a malformed freshness-policy input instead of silently acting on it. + + A verifier's age/skew policy is configuration, and a wrong one fails in the + direction that matters: ``max_age_seconds=-1`` is not a stricter bound, it + classifies every record ever issued as stale, and a caller who meant to + disable the bound (``None``) would see a uniform refusal with no error + naming the cause. ``bool`` is excluded explicitly because it is a subclass + of ``int`` in Python, so ``True`` would otherwise pass as one second. + + Shared by :func:`sign.verify_record` and :func:`provenance.verify_record`, + which pass their own exception type via *exc* so each keeps its existing + public error type (``ValueError`` and ``ProvenanceError`` respectively). + """ + if optional and value is None: + return + if isinstance(value, bool) or not isinstance(value, int): + raise exc(f"{name} must be an integer, got {type(value).__name__}") + if value < 0: + raise exc(f"{name} must be non-negative, got {value}") def sign_record(record: dict[str, Any], key: Ed25519PrivateKey) -> dict[str, Any]: """Return a copy of *record* with ``cnf.jwk`` populated and a ``signature`` field added. @@ -486,8 +507,10 @@ def verify_record( ) # Freshness: bound the age of the record against its issued-at timestamp. - if max_future_skew_seconds < 0: - raise ValueError("max_future_skew_seconds must be non-negative") + # Both bounds are verifier configuration, not record data, and a malformed + # one is checked before it is used -- see `_check_seconds`. + _check_seconds("max_future_skew_seconds", max_future_skew_seconds) + _check_seconds("max_age_seconds", max_age_seconds, optional=True) iat = record.get("iat") if not isinstance(iat, int) or isinstance(iat, bool): raise ValueError("record has no valid integer 'iat' for freshness check")