Skip to content

fix(tee): fail closed on empty TCB-status allowlist in TEE admission#3023

Open
xilosada wants to merge 3 commits into
masterfrom
fix/tee-admission-tcb-fail-closed
Open

fix(tee): fail closed on empty TCB-status allowlist in TEE admission#3023
xilosada wants to merge 3 commits into
masterfrom
fix/tee-admission-tcb-fail-closed

Conversation

@xilosada

Copy link
Copy Markdown
Member

What

TEE admission's TCB-status check was fail-open. When a group's allowed_tcb_statuses is empty — which is the default — the check was skipped entirely, so a node reporting tcb_status = "OutOfDate" was admitted as a ReadOnlyTee member.

This makes it fail closed, via a single shared helper applied at both admission-enforcement sites:

  • crates/context/src/handlers/admit_tee_node.rs (verifier-side, pre-publish)
  • crates/governance-store/src/membership/policy_rules.rs validate_tee_attestation_allowlists (op-apply path, runs on every replicating node)

Rules (tcb_status_allowed, in calimero-governance-store)

  1. Revoked → rejected unconditionally (case-insensitive). Real dcap-qvl verification already bails on Revoked, but the subgroup-reuse path reuses a stored tcb_status string without re-verifying — this guards that path.
  2. Mock path preserved. Mock admission (accept_mock) and the reserved "Mock" status (which the subgroup-reuse path stores while passing is_mock:false) bypass the allowlist. Real dcap-qvl never emits "Mock".
  3. Empty allowlist → secure default {"UpToDate"} (was: skip the check).
  4. Non-empty allowlist → honored exactly (case-sensitive; values are PascalCase from dcap-qvl).

{UpToDate} is deliberately aligned with the mero-tee KMS key-delivery gate (which already enforces UpToDate-only), so a node that admission accepts can also obtain its key. Not broadened to SWHardeningNeeded to preserve that invariant.

Notes

  • No public-surface change to calimero-server-primitives / calimero-tee-attestation → no downstream rev bump. The admin policy default stays vec![]; "empty" now means "use the secure default" at enforcement time, so existing callers passing empty still work and get the safe behavior.
  • Mock/dev fleets and the merobox TEE workflows (all mock_tee/accept_mock) are unaffected.

Tests

  • calimero-governance-store: 386 passed (5 new — empty-allowlist fail-closed regression, non-empty honored, unconditional Revoked reject, mock-path preserved, op-apply validator end-to-end).
  • calimero-context: 199 passed. calimero-node incl. 17 local_governance mock-admission e2e: passed.
  • cargo fmt --check (1.88) + clippy clean.

TEE admission skipped the TCB-status check entirely when a group's
`allowed_tcb_statuses` was empty (the default), so a node reporting
`tcb_status="OutOfDate"` was admitted as a ReadOnlyTee member by default.

Make it fail closed via a shared `tcb_status_allowed` helper applied at both
admission-enforcement sites (verifier-side `admit_tee_node` and the op-apply
`validate_tee_attestation_allowlists`):

- empty allowlist now enforces the secure default {"UpToDate"} (matches the
  mero-tee KMS key-delivery gate, so "admitted => can get key" holds);
- a non-empty allowlist is honored exactly (case-sensitive, PascalCase);
- `Revoked` is rejected unconditionally (case-insensitive) — guards the
  subgroup-reuse path, which reuses a stored tcb_status without re-verifying;
- the mock path is preserved: mock admission (accept_mock) and the reserved
  "Mock" status bypass the allowlist, so mock/dev fleets are unaffected.

No public-surface change; the admin policy default stays `vec![]` (empty now
means "use the secure default" at enforcement time).

@meroreviewer meroreviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 30% | Review time: 58.9s

🟡 1 warnings, 💡 1 suggestions. See inline comments.


🤖 Generated by MeroReviewer | Review ID: review-5275731a

allowed_tcb_statuses: &[String],
tcb_status: &str,
is_mock: bool,
) -> bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mock bypass applies even when is_mock=false and status is "Mock" on the op-apply path

The tcb_status_allowed function allows any node to pass the TCB gate if its stored tcb_status string equals "Mock" — regardless of is_mock. On the op-apply path (validate_tee_attestation_allowlists), is_mock is always false, so the only guard is the string comparison. This means a malicious or buggy node that somehow gets tcb_status = "Mock" stored in a MemberJoinedViaTeeAttestation op (e.g. via a crafted op replayed from a mock fleet onto a real fleet, or a compromised verifier) will bypass the TCB allowlist on every replicating node. The PR description acknowledges this as intentional for the subgroup-reuse path, but the trust boundary relies entirely on accept_mock being enforced upstream at admission time — if that gate is ever bypassed or a stored op is replayed cross-fleet, the "Mock" string becomes a permanent bypass token. Consider documenting this invariant more explicitly as a // SAFETY: comment, or adding a check that "Mock" is only accepted when the group policy has accept_mock = true (readable from the store on the op-apply path).

Suggested fix:

In `validate_tee_attestation_allowlists`, pass the group's `accept_mock` flag to `tcb_status_allowed` instead of hardcoding `false`, so the mock bypass is gated on the stored policy rather than solely on the string value. Alternatively, add a `// SAFETY:` comment explaining why cross-fleet replay of a stored `"Mock"` status is impossible in practice.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 4efed9f by implementing your primary suggestion: accept_mock is now threaded into tcb_status_allowed instead of relying solely on the "Mock" string.

accept_mock is readable from the stored TeeAdmissionPolicy at both call sites:

  • op-apply (validate_tee_attestation_allowlists): added accept_mock to TeeAllowlistPolicy, populated from the policy read via read_required_tee_admission_policy.
  • admit_tee_node: passes policy.accept_mock.

The mock branch is now if (is_mock || tcb_status == "Mock") && accept_mock. So a stored "Mock" status replayed onto a real fleet (accept_mock = false) no longer bypasses the gate — it falls through to the empty-allowlist secure default (UpToDate) and is rejected. The "Mock" string is no longer a permanent bypass token; it only passes on a fleet that actually opted into mock. On admit_tee_node this is also defense-in-depth behind the existing upstream is_mock && !accept_mock rejection.

New test tcb_status_gate_mock_requires_accept_mock proves a stored "Mock" is rejected when accept_mock = false (both empty and non-empty allowlists), and validate_allowlists_empty_tcb_enforces_secure_default now asserts the same through the op-apply entry point.

if is_mock || tcb_status == TCB_STATUS_MOCK {
return true;
}
if allowed_tcb_statuses.is_empty() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Empty allowlist secure-default does not apply when is_mock=true with a non-"Mock" status

If is_mock=true and tcb_status is anything other than "Revoked" (e.g. "OutOfDate"), the function returns true at line 44 before reaching the empty-allowlist default. This is intentional for the admit_tee_node path (mock is gated by accept_mock), but the interaction is subtle: a caller that passes is_mock=true with a real-looking status like "OutOfDate" will be admitted unconditionally. The existing test tcb_status_gate_preserves_mock_path only tests tcb_status = "Mock" with is_mock=true, not tcb_status = "OutOfDate" with is_mock=true. Adding a test for that case would make the intended behavior explicit.

Suggested fix:

Add a test: `assert!(tcb_status_allowed(&[], "OutOfDate", true));` with a comment explaining this is intentional (mock path bypasses TCB check entirely, gated upstream by `accept_mock`).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 4efed9f. tcb_status_gate_preserves_mock_path now includes the is_mock=true + non-"Mock" status case you described:

// The is_mock flag bypasses the TCB check entirely — even a real-looking
// non-"Mock" status like OutOfDate is admitted when is_mock=true on a
// mock-accepting policy (the TCB allowlist does not apply to the mock path).
assert!(tcb_status_allowed(&[], "OutOfDate", true, true));

Note the gate gained an accept_mock parameter (see the warning thread above), so the mock bypass now requires accept_mock = true. The complementary negative case — is_mock=true with OutOfDate but accept_mock=false → rejected — is asserted in the new tcb_status_gate_mock_requires_accept_mock test, making the intended behavior explicit in both directions.

@meroreviewer

meroreviewer Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Documentation Review

The following documentation may need updates based on the changes in this PR:

  • 🟡 architecture/auto-follow.html: Files matching crates/governance-store/** were changed but architecture/auto-follow.html was not updated (per source_to_docs_mapping).
  • 🟡 architecture/crates/context.html: Files matching crates/context/** were changed but architecture/crates/context.html was not updated (per source_to_docs_mapping).
  • 🟡 architecture/local-governance.html: Files matching crates/governance-store/** were changed but architecture/local-governance.html was not updated (per source_to_docs_mapping).
  • 🟡 architecture/membership-and-leave.html: Files matching crates/context/** were changed but architecture/membership-and-leave.html was not updated (per source_to_docs_mapping).

Address MeroReviewer findings on the shared `tcb_status_allowed` gate:

- The reserved "Mock" status bypassed the TCB allowlist on the op-apply
  path regardless of the group's `accept_mock` policy, so a stored "Mock"
  status replayed cross-fleet acted as a permanent bypass token. Thread the
  group's stored `accept_mock` (already available from `TeeAdmissionPolicy`
  at both call sites) into `tcb_status_allowed` and gate the mock bypass on
  it. A replayed "Mock" on a real fleet (accept_mock=false) now falls
  through to the secure default and is rejected. This also makes the
  admit_tee_node path defense-in-depth behind its upstream
  `is_mock && !accept_mock` rejection.

- Add a test for is_mock=true with a non-"Mock" status (OutOfDate),
  documenting that the mock path bypasses the TCB check entirely, plus a
  test proving the accept_mock gating rejects a stored "Mock" on a real
  fleet.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

E2E Rust Apps Failed

One or more E2E workflows (scaffolding-e2e, xcall-example) failed.

Please check the workflow logs for more details.

@meroreviewer meroreviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MeroReviewer

Reviewed by 2 agents | Quality score: 56% | Review time: 55.8s

🟡 1 warnings. See inline comments.


🤖 Generated by MeroReviewer | Review ID: review-92415358

if tcb_status.eq_ignore_ascii_case(TCB_STATUS_REVOKED) {
return false;
}
if (is_mock || tcb_status == TCB_STATUS_MOCK) && accept_mock {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mock bypass activates on is_mock=true regardless of tcb_status

The condition (is_mock || tcb_status == TCB_STATUS_MOCK) && accept_mock returns true for any tcb_status value (including strings that look like real statuses) when is_mock=true and accept_mock=true. On the admit_tee_node path, is_mock comes from the caller's request payload. If an attacker can set is_mock=true on a request to a mock-accepting fleet, they bypass the TCB allowlist entirely — even with a non-"Mock" status like "OutOfDate". The test tcb_status_gate_preserves_mock_path explicitly asserts tcb_status_allowed(&[], "OutOfDate", true, true) == true, confirming this is intentional, but the design note says the mock bypass should apply to the mock path, not to arbitrary statuses. On the admit_tee_node path, the upstream is_mock && !accept_mock gate only rejects mock on non-mock fleets; on a mock-accepting fleet, is_mock=true with any tcb_status passes. This means a node that somehow sets is_mock=true with tcb_status="OutOfDate" is admitted on a mock fleet. The fix should restrict the is_mock branch to only bypass when tcb_status == TCB_STATUS_MOCK, not for arbitrary statuses.

Suggested fix:

Change the mock bypass condition to: `if (is_mock && tcb_status == TCB_STATUS_MOCK || tcb_status == TCB_STATUS_MOCK) && accept_mock { return true; }` — i.e., require `tcb_status == TCB_STATUS_MOCK` even when `is_mock=true`, so the bypass is scoped to the reserved mock status rather than any status the caller supplies.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions Bot added the Stale label Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants