Keyfob Station — Code Review & Open Issues # Keyfob Station — Code Review & Open Issues Code analysis of the `keyfob-station` Rust workspace (commit `0e0243b` → review fixes → JCShell-driven rework → second review pass). > Companion to **"Ford EoL Key Fob Provisioning System (SCP03)"**. --- ## Summary 11 findings identified during initial code review. **7 fixed** in review pass 1; **2 resolved** by JCShell rework; **1 deferred** to real-card; **1 mock limitation** noted. Second review pass (post-JCShell rework) found **3 CRITICAL** + **7 WARNING** + several **SUGGESTION** items. All CRITICAL and WARNING items fixed. Remaining items are dead-code cleanup or deferred to real-card. **Verification gate:** **64 tests pass**, `clippy -D warnings` clean, `fmt` clean, `self-test` green. --- ## JCShell-Driven Rework (2026-06-15 rev 2) Analysis of the reference JCShell script (`fixtures/jcshell/`) revealed the real provisioning flow differs significantly from the initial implementation. The entire personalization pipeline was reworked. ### What Changed | Area | Before | After | |---|---|---| | **Personalization model** | Flat `PayloadApdu` list (CLA/INS/P1/P2 + data) | Structured `PersonalizationItem` list with DGI tags + S-DEK encryption flag | | **STORE DATA INS** | `0xDA` | `0xE2` (GP personalization) | | **Security level** | `C_MAC_DEC` (0x03) | `C_MAC` only (0x01) — matches `auth mac` | | **Data encryption** | SCP03 C-DEC wraps all payload data | No C-DEC; private key pre-encrypted with S-DEK before DGI wrapping | | **Lifecycle transition** | `SET STATUS` (INS=0xF0, P1=0x0F P2=0x00) | `STORE DATA` P1=0x80 (INS=0xE2) — final block triggers UNPERSONALIZED → FACTORY | | **Post-perso** | Not modeled | Raw `00DB` commands (enable UICC/BLE transport), no SM | | **Mock applet** | `locked: bool` after SET STATUS | `lifecycle: Lifecycle` enum (Unpersonalized → Factory), C-DEC flag tracks EXTERNAL AUTH P1, accepts raw CLA=0x00 | | **Source** | `StaticProvisioningSource` only | + `FileContainerSource` (parses container dir → ProvisioningInput) + `StaticCertificates` | ### New Modules | Module | Purpose | |---|---| | `kf-provision/src/perso.rs` | DGI TLV encoding, S-DEK pre-encryption (AES-CBC/M2/IV=zeros), block splitting (max 245B) | | `kf-source/src/container.rs` | Container directory parser: FESN.txt, irk_keyfob.key (base64), keyfob_private.der (PKCS#8 EC scalar extraction), keyfob_public.der; `StaticCertificates` for ICA/CMS certs | ### DGI Encoding Details **Wire format:** `TAG(2) || length || data` - Length `< 255`: 1-byte direct - Length `≥ 255`: `0xFF` + 2-byte big-endian **Max block data:** 245 bytes (measured from the reference script: blocks split at exactly 245B = 255-byte short APDU limit − 8-byte MAC − 2-byte slack). **P1/P2 protocol:** - P1 = `0x00` for all data blocks, `0x80` for the lifecycle-transition block (last) - P2 = global incrementing counter across all DGIs and split blocks --- ## Second Review Pass Fixes (2026-06-15 rev 3) ### CRITICAL — All Fixed | # | Issue | Fix | |---|---|---| | C1 | **DGI tag indexing panic** — `encode_dgi` indexed `dgi[0]`/`dgi[1]` without length check → could panic on malformed input | Added `ProvisionError::InvalidInput` validation: DGI must be exactly 2 bytes, data non-empty | | C2 | **Empty `personalization_items` silent success** — pipeline would proceed through SCP03 and "succeed" with zero DGIs written | Rejected in `Provisioner::run()` before opening SCP03 channel | | C3 | **Audit TX entries leaked secrets** — `audit.rs` stored full STORE DATA command bytes including IRK, SPID, private key | TX entries now redact data body (header only: CLA/INS/P1/P2/Lc); only metadata retained | ### WARNING — All Fixed | # | Issue | Fix | |---|---|---| | W1 | **Applet auth bypass on empty-data STORE DATA** — empty-data shortcut bypassed C-MAC verification, allowing unauthenticated lifecycle transition | Removed shortcut; general `P1 & 0x80` branch handles it after MAC verification | | W2 | **`max_block_data` dead config** — `ProvisionerConfig` had the field but `split_into_blocks` used hardcoded 245 | Threaded config value through to `split_into_blocks` | | W3 | **SPID parse fragility** — `split('-').nth(1)` breaks if FESN contains a dash | Switched to `rsplit('-').next()` for dash-in-FESN robustness | | W4 | **PersonalizationItem.data not zeroized** — plain `Vec` holding private key scalar | Accepted as known limitation; data flows from `ProvisioningSource` which owns the lifecycle | ### SUGGESTION — Remaining (Low Priority) | # | Issue | Status | |---|---|---| | S1 | `handle_set_status()` + `ins::SET_STATUS` — orphaned dead code from pre-rework | Safe to delete; no callers | | S2 | `lifecycle()` / `is_factory()` accessors — unused | Wire into test assertions or remove | | S3 | `PersonalizationItem.data` not zeroized | Accept or wrap in `ZeroizingVec` later | --- ## Fixed in Initial Review Pass (2026-06-15 rev 1) ### B1 — Audit timestamp was not ISO-8601 ✅ FIXED (HIGH) **Fix:** Added `time` crate (`formatting` + `macros`). Now formats as `[year]-[month]-[day]T[hour]:[minute]:[second]Z`. ### B5 — Applet host-cryptogram compare not constant-time ✅ FIXED (LOW) **Fix:** Changed `!=` to `kf_crypto::ct_eq`. ### B6 — `Provisioner::run` discarded the typed error ✅ FIXED (MED) **Fix:** `run()` now returns `(AuditRecord, Option)`. ### B7 — Host-challenge RNG failure panicked ✅ FIXED (LOW) **Fix:** `generate_host_challenge()` returns `Result` with `ProvisionError::Entropy`. ### B8–B10 — Test/code hygiene ✅ FIXED (LOW) - Removed tautological `assert_eq!(x, x)`. - Cleaned stale comment on `From`. - Consolidated `async-trait` to workspace deps; removed duplicate `serde_json`. --- ## Resolved by JCShell Rework ### B3 — `lock_isd()` P1/P2 need confirming ✅ RESOLVED The real provisioning flow does **not use SET STATUS at all**. Lifecycle transition is triggered by the final STORE DATA block with `P1=0x80`. `lock_isd()` replaced with `lifecycle_transition(p2)`. Simulated applet uses `Lifecycle` enum (`Unpersonalized` → `Factory`). ### B11 — Applet encryption-counter desync on empty-body commands ✅ MOOT With C-MAC only (no C-DEC), there is no data decryption on STORE DATA. The desync path is unreachable. --- ## Still Deferred (Real-Card) ### B2 — R-ENC IV uses non-standard `0x80` marker ⏳ DEFERRED (MED) **Location:** `kf-scp03/src/channel.rs` The `iv[0] |= 0x80` mutation for R-ENC IVs may not match GP SCP03 Amendment D §6.2. Only reached under `SecurityLevel::FULL`, which the pipeline never negotiates (default is `C_MAC` only). **Action:** Drop the `0x80` toggle and verify against a real R-ENC vector when hardware is available. ### B4 — Test vector is self-derived, not vendor-blessed ⏳ DEFERRED (MED) **Location:** `kf-scp03/src/vectors.rs` Session keys/cryptograms are computed by the same code under test. Tests prove internal consistency, not conformance to an authoritative GP vector. **Action:** Swap in official GlobalPlatform SCP03 test vectors when available. De-risked by running against a real JCOP card with known static keys. --- ## Open Questions 1. ~~**ICA/Root certificates (DGI A004/A005)**~~ — **Resolved**: factory-wide static certs, loaded from shared dir (`ica_cert.der`, `cms_root_cert.der`). Verified byte-identical across both fixtures. 2. **SPID derivation** — NOT SHA-1 of FESN (verified). Comes from external source (KLMS or container delivery). Directory name: `-`. 3. **Key Version 255** — Reference script uses KVN=0xFF. Production may differ. Confirm with KLMS. 4. **Container delivery mechanism** — Per-fob via network or batch via USB/media? Affects directory scan logic. --- *Created 2026-06-15. Updated 2026-06-15 rev 3: second review pass complete, all CRITICAL/WARNING fixed, 64 tests green.* id: 1afe8b0b762043189ace149c4fc388b7 parent_id: 283dd54f183a4ce69366648f091336d1 created_time: 2026-06-15T11:08:29.944Z updated_time: 2026-06-15T15:45:54.655Z is_conflict: 0 latitude: 0.00000000 longitude: 0.00000000 altitude: 0.0000 author: source_url: is_todo: 0 todo_due: 0 todo_completed: 0 source: joplin-desktop source_application: net.cozic.joplin-desktop application_data: order: 1781521709944 user_created_time: 2026-06-15T11:08:29.944Z user_updated_time: 2026-06-15T15:45:54.655Z encryption_cipher_text: encryption_applied: 0 markup_language: 1 is_shared: 0 share_id: conflict_original_id: master_key_id: user_data: deleted_time: 0 type_: 1