fix(dmrtd): reject an unprotected success on a secure-messaging session

[itsbalamurali]

Closes #22. Found by cubic on #21; that PR only reformatted the adjacent line, so this is the actual fix rather than a lint change.

The hole

MrtdSM::unprotect skipped verification whenever the response body was empty:

if rapdu.status == StatusWord::SM_DATA_MISSING
    || rapdu.status == StatusWord::SM_DATA_INVALID
    || rapdu.data.as_ref().is_none_or(|d| d.is_empty())
{
    return Ok(rapdu.clone());   // no MAC check, no SSC advance
}

A protected exchange is answered with DO'99' and DO'8E' even when the command returns no data. So a response carrying a bare status word has been authenticated by nobody — and MrtdApi::transceive_cmd sees SUCCESS and returns it as a legitimate result.

Concretely: for any protected command whose response is empty — SELECT, MSE:Set AT — anyone able to modify the channel can strip the secure-messaging wrapper, return a bare 9000, and have the reader record the command as having succeeded with no MAC ever checked. "Nothing to check" is not "checked and fine", and the guard could not tell the two apart.

What changed

An unprotected success is now an error. That is the shape with no legitimate meaning on an SM session.

Unprotected errors still pass through, deliberately. That is how the chip reports that secure messaging itself failed (6987/6988) or that it rejected the command before applying SM at all. It cannot be authenticated either — but it is the only diagnosis the caller gets, and MrtdApiError::ChipStatus carries the status word up to PassportError::code, where the wrong-BAC-key mapping (63CF → security status not satisfied) depends on it. Turning those into an opaque SM failure would trade this bug for a worse one: every wrong-key read would stop saying why.

A status-bearing response that carries a body is now verified like any other, including when the status claims SM already failed. A status word sitting outside the MAC is not evidence about the bytes inside it.

The SSC is left alone on both paths, so a session that somehow continued cannot do so against a counter the chip does not share.

Tests

unprotect_passthrough_on_empty_body asserted the old behaviour, so it is replaced by unprotect_rejects_an_unprotected_success. Two more cover what the fix depends on: that neither path advances the SSC, and that an SM-error status carrying a body is still MAC-checked.

681 dmrtd tests pass, including the protect/unprotect loopback that exercises the normal path end to end.

Risk

This is a behavioural change against real hardware, and worth a device test before release rather than after. A compliant chip always wraps its responses, so a compliant chip is unaffected — but if any passport in the field answers a protected exchange with a bare 9000, reads that currently succeed will now fail closed. That is the correct direction for a security fix, and it is still a change in what works.

Note for merge order: #21 touches the same line and will conflict with whichever of the two lands second. Trivial to rebase either way.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Fixes a secure-messaging bypass in dmrtd: a bare success (9000 with no data) on a protected session is now rejected. Unprotected errors still bubble up, any response with a body is MAC-checked, and the SSC is not advanced in either case.

• Bug Fixes

  • MrtdSM::unprotect now returns an error for unprotected SUCCESS on SM sessions.
  • Unprotected error status words (e.g., 6987, 6988) pass through for diagnosis without advancing the SSC.
  • Always MAC-check when a body is present, regardless of the status word.

• Migration

  • Non‑compliant passports that answer protected commands with a bare 9000 will now fail; run a device test before release.

^{Written for commit 32ace76. Summary will update on new commits.}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 494982b9-cb90-49e3-8b20-12b3e098c6f7

📥 Commits

Reviewing files that changed from the base of the PR and between 2b7367f and 32ace76.

📒 Files selected for processing (1)

• crates/dmrtd/src/proto/mrtd_sm.rs

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Secure-messaging responses with a successful status but no message authentication data are now rejected.
  • Responses with other status codes and no body pass through without advancing secure-messaging state.
  • Responses containing a body continue to undergo authentication and decryption, including when reporting a secure-messaging failure.

Walkthrough

MrtdSM::unprotect now rejects successful responses without secure-messaging data, preserves non-success passthrough behavior without advancing SSC, and validates present bodies regardless of status. Tests cover rejection, SSC preservation, and MAC validation.

Changes

Secure messaging validation

Layer / File(s)	Summary
Response validation and regression coverage crates/dmrtd/src/proto/mrtd_sm.rs	unprotect rejects bodyless SUCCESS responses, preserves non-success passthrough behavior without advancing SSC, and verifies responses containing secure-messaging objects even when their status indicates failure. Tests cover these cases.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

I’m a rabbit guarding the MAC,
No empty success slips through the stack.
The SSC stays still,
Errors bend to its will,
And signed bodies keep answers on track.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: rejecting an unprotected success during secure messaging.
Description check	✅ Passed	The description is directly related to the secure-messaging fix and its behavioral changes.
Linked Issues check	✅ Passed	The changes match #22 by rejecting bare success, preserving error passthrough, MAC-checking bodies, and avoiding SSC advancement.
Out of Scope Changes check	✅ Passed	The patch stays within the secure-messaging fix and test updates, with no clear unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/sm-passthrough

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cubic-dev-ai]

No issues found across 1 file

_{Re-trigger cubic}