fix(vehicle): harden signed-command session authentication#99
Conversation
… session path Per an independent security audit of the signed-command/session layer: - Verify a session_info reply's session_info_tag HMAC (bound to the client's own sent request uuid as TAG_CHALLENGE) before trusting any field of it, including its whitelist status. VCSEC's wire-level request_uuid echo is typically empty and is not itself authentication, so acceptance never gates on it. A bad/absent tag raises SessionInfoAuthenticationFault without mutating session state. - Session.commit clamps the anti-replay counter to max(current, incoming) within an epoch, refuses a same-epoch clock regression, and only resets on an epoch change - closing a client-side rollback/DoS window. - The AES-GCM (BLE) response path now rejects a replayed response counter via SignedCommandResponseReplayed, defending against a captured encrypted response being replayed back at the client. None of this affects the cloud (HMAC/TLS) transport's security posture or allows unauthorized actuation on either transport; it closes BLE-specific spec-conformance gaps bounded at DoS/response-spoofing under an active on-path attacker. tests/test_session_info_authentication.py adds coverage the BLE-mocked command tests structurally can't provide (they pre-mark sessions ready and mock _send wholesale): a golden-vector test pinned to protocol.md's own published handshake vectors, unit tests for tag/counter/clock handling, and end-to-end tests through the real (unmocked) BLE handshake state machine.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97740b955f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| session = self._sessions[msg.from_destination.domain] | ||
| info = SessionInfo.FromString(msg.session_info) | ||
| shared_key, hmac_key, session_info_key = session.keys_for(info.publicKey) |
There was a problem hiding this comment.
When a forged or corrupt session_info carries an empty or non-P-256 publicKey, this line derives the ECDH key before any tag validation can run, so cryptography raises ValueError instead of the new SessionInfoAuthenticationFault. That escapes callers that follow this library's TeslaFleetError handling contract, causing a bad handshake packet to abort BLE/Fleet command recovery with an unexpected ordinary exception rather than being discarded as an authentication failure; check tag presence first and wrap invalid public keys as SessionInfoAuthenticationFault.
Useful? React with 👍 / 👎.
1 participant
Intent
Harden the Tesla signed-command session layer per an independent security audit of python-tesla-fleet-api (report at data/signed-cmd-audit-python/report.md). The cloud/HMAC transport is fine as-is (TLS to Tesla neutralizes the risk); the gaps are BLE-specific spec-conformance deviations from teslamotors/vehicle-command's protocol.md, bounded at DoS/response-spoofing under an active on-path BLE attacker - none permit unauthorized vehicle actuation (ECDH command signing prevents that regardless). Implemented three fixes as one cohesive PR: (1) authenticate session_info replies by verifying the session_info_tag HMAC (keyed by HMAC(K, "session info"), bound to the client's own sent request_uuid as TAG_CHALLENGE) BEFORE trusting any field including counter/epoch/clock_time/publicKey/whitelist-status - previously Session.update() adopted every field unconditionally with no authentication at all; (2) counter/clock monotonicity - within an epoch the counter now clamps to max(current, incoming) and a same-epoch clock regression is refused, only resetting on an epoch change, replacing an unconditional overwrite that could be wound backward by a stale/replayed session_info; (3) AES-GCM (BLE) response-counter replay dedup via a new SignedCommandResponseReplayed exception, so a captured encrypted response can't be replayed back at the client - the cloud/HMAC response path is intentionally left as-is since it isn't authenticated at all today (same low-risk-over-TLS posture as before). Deliberately did NOT add a node-tesla-fleet-api-style guard on the wire-level request_uuid echo field - VCSEC legitimately leaves it empty on real hardware (memory constraints) and gating acceptance on it would be a regression (that was a HIGH-severity bug in a sibling port); the new tag-based authentication never depends on that field being populated, only cross-checking it when present. Added tests/test_session_info_authentication.py (21 tests) since the existing BLE-mocked test harness (tests/ble_mocked_transport.py) pre-marks sessions ready and mocks _send wholesale, so this path was previously completely untested: includes a golden-vector test pinned byte-for-byte against protocol.md's own published handshake worked example (independent verification against the spec, not just internal consistency), unit tests for bad/absent/mismatched tags, counter/clock rollback refusal, response-counter replay, and - importantly - a test proving a valid tag with an EMPTY wire-level request_uuid is still accepted (proving we never regress into the sibling port's bug), plus two tests driving the real (unmocked) BLE handshake/_send/_await_response state machine end-to-end. All existing tests (440) still pass; pyright strict and ruff are clean. This is a public repo so no internal-planning/report/PR-number references belong in shipped code comments - I kept comments scoped to a single WHY constraint each, per project convention.
What Changed
session_inforeplies before trusting session state, while preserving valid VCSEC replies with an empty echoed request UUID.Risk Assessment
✅ Low: The replay cache is now bounded, request-scoped, epoch-reset, and populated only after successful AES-GCM authentication; no remaining material issues were found in the branch diff.
Testing
The focused security suite and complete runtime suite passed, and the evidence transcript directly demonstrates golden-vector authentication, preservation of legitimate empty-UUID BLE handshakes, rejection of forged session information, and response replay protection. No visual artifact was captured because this is a headless Python protocol library with no rendered user interface.
Evidence: Signed-command security protocol transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (2) ✅
tesla_fleet_api/tesla/vehicle/commands.py:652- Replay detection uses a session-wide high-water mark (response_counter <= last_response_counter). The protocol scopes reuse detection to counters previously used for the same request and explicitly permits multiple responses to arrive out of order. After accepting counter 9, this rejects a legitimate unused counter 8 as replayed. Track authenticated counters per request rather than enforcing global monotonic ordering; add a decreasing-but-unused counter case.🔧 Fix: Scope response replay detection per request
1 warning still open:
tesla_fleet_api/tesla/vehicle/commands.py:698- Every authenticated AES response permanently adds a unique(request_hash, counter)tuple, and entries are cleared only when the vehicle epoch changes. Long-lived BLE sessions performing regular state reads can therefore grow this set without bound. Retain counters only while their request can still receive responses, or use a bounded recent-request cache.🔧 Fix: Bound authenticated response replay cache
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
Inspectedgit diff 742a804a91a7bef264e006e660426534d2b438c7..b8bce43f7b94485a5916a87dfb5a423fd28484d8to map the intended security behavior to implementation and tests.uv run pytest -q tests/test_session_info_authentication.pyuv run pytest -q testsCaptured a named protocol transcript usinguv run pytest -vvfor the published golden handshake vector, valid empty UUID behavior, forged handshake rejection through the real BLE state machine, successful real BLE handshake with an empty UUID, and encrypted-response replay rejection.Verifiedgit status --shortremained clean after testing.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.