fix(migtd): enforce platform TCB floors in policy v2#937
Open
MichalTarnacki wants to merge 1 commit into
Open
Conversation
MichalTarnacki
commented
Jul 3, 2026
Contributor
| # | Assessed Severity | Bug | Call Chain / Location | Fix | Classification | Disposition Reason |
|---|---|---|---|---|---|---|
| 1 | Medium | PolicyProperty::evaluate_string greater-or-equal uses raw lexicographic compare on tcb_date | verify_peer_cert -> authenticate_remote -> MigPolicy::evaluate -> PolicyProperty::evaluate_string | Parse to chrono::NaiveDate before compare; reject non-canonical formats. | weakness | Why this is a weakness, not an exploitable true positive: the finding's lexicographic arithmetic is real, but no attacker-controlled input can reach that comparison in a non-canonical form. The left operand is never a free-form string; it is the binary R_TCB_DATE field read as a u64 and rendered through chrono's %Y-%m-%dT%H:%M:%SZ, which always emits zero-padded fixed-width digits, and its value is anchored to the Intel-signed root CA via quote verification, so neither the VMM nor a remote peer can place 2024-9-1 there. The right operand is a policy/collateral field that only materializes after the ECDSA signature over the raw bytes is verified and the peer chain is pinned to the same root CA, so it cannot be forged across a trust boundary either. The only way a non-canonical date enters the compare is a human typing it into a hand-edited, then-signed source file (the *_raw.json policy or td_identity.json), which is a mistake by the already-fully-trusted policy producer rather than an exploit primitive. That makes it a robustness gap, not a remotely reachable vulnerability |
| 2 | Critical | TcbStatus/ServtdTcbStatus rank() maps OutOfDate==UpToDate; ALWAYS_ALLOW includes | verify_peer_cert -> authenticate_policy -> evaluate_tcb_status | Give OutOfDate rank<UpToDate; remove OutOfDate from ALWAYS_ALLOW; require explicit policy opt-in. | true_positive | This is a true positive, but mischaracterized, The §5.1 advisory wording ("destination platform should be up to date") and the §5.3.2 normative SVN comparison (table 5-3: integer arrays use array-greater-or-equal and each element "shall be equal or greater-or-equal than the reference", with the design guide's own example policy listing sgxtcbcomponents/pcesvn/tdxtcbcomponents) are both confirmed. The report's third claim — that the TcbStatus string ranking is "impl-invented with no normative ordering" — is false: the same design guide states verbatim "uptodate == swhardeningneeded == outofdate >= configurationneeded == ... > revoked", that "outofdate must be accepted as uptodate because the time stamp is not trusted", and that "revoked should not be accepted", which is exactly what rank() and the ALWAYS_ALLOW/ALWAYS_DENY sets implement. Because of that, the report's stated mechanism and proposed fix are both wrong: OutOfDate is accepted by the ALWAYS_ALLOW early return before rank()-based >= is ever reached, so giving OutOfDate a lower rank changes nothing, and removing it from ALWAYS_ALLOW would contradict the specification. The genuine defect the finding points at is real and worth fixing: v2 adopted the permissive tcbStatus half of the spec while dropping the protective numeric platform-SVN floor that v1 still enforces. The residual exposure that keeps this a weakness rather than Critical is that the floor is policy-driven per the spec, so it only takes effect once the signed v2 policy actually carries the platform SVN baselines. |
| 3 | High | v2 evaluate_policy_forward/backward/common return Ok(()) when block is None | -> PolicyData::evaluate_policy_forward | Treat None as Err(PolicyIncomplete) or require at least one constraint per direction. | true_positive | The core claim is correct: evaluate_policy_forward/backward/common returned Ok(()) when the policy block was None, so an absent block silently skipped mandatory TCB and engine checks — a genuine fail-open that the design guide's acceptance procedure does not sanction (absence-permissive rules exist only for fmspc and the CRL numbers, never for TCB status or engine). However, the finding's RATLS_PEER_CERT / T1 "directly reachable from untrusted peer" framing is wrong: the evaluated policy is the local, signature-verified get_verified_policy(), not peer-supplied input, so it is not remotely exploitable and not Critical. |
Policy v2 evaluation populated platform tcb_status and FMSPC from quote supplemental data, but it did not carry the raw platform SVN component arrays or PCE SVN into PolicyEvaluationInfo, so a policy that wanted to enforce per-component platform TCB floors had no data to evaluate. The date comparison path also relied on lexicographic ordering without rejecting non-canonical timestamps. Extend PolicyEvaluationInfo with TDX TCB components, SGX TCB components and PCE SVN, plumb those fields from mig_policy quote supplemental data, and teach PlatformPolicy to evaluate tdxtcbcomponents, sgxtcbcomponents and pcesvn with fail-closed behavior when the policy requires them. Keep the v2 platform check aligned with the v1 security version floor by floor-matching raw component arrays instead of relying only on the permissive tcbStatus string. Also require canonical fixed-width ISO-8601 UTC timestamps before using string greater-or-equal comparisons, and add regression tests for the new platform TCB component and PCE SVN policy checks. Signed-off-by: Michal Tarnacki <[email protected]> Co-authored-by: GitHub Copilot <[email protected]>
sgrams
reviewed
Jul 7, 2026
sgrams
left a comment
Contributor
There was a problem hiding this comment.
Let me check the documentation and return with more comments
Comment on lines
+154
to
+161
| /// The platform TDX TCB components (16-byte SVN array from the quote) | ||
| pub tdx_tcb_components: Option<[u8; 16]>, | ||
|
|
||
| /// The platform SGX TCB components (16-byte SVN array from the quote) | ||
| pub sgx_tcb_components: Option<[u8; 16]>, | ||
|
|
||
| /// The platform PCE SVN | ||
| pub pce_svn: Option<u16>, |
Contributor
There was a problem hiding this comment.
Let's review this against MigTD Design Guide before merging
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.