[framework] Add Nitro enclave attestation support#20103
Conversation
There was a problem hiding this comment.
Stale comment
Aptos Security Bugbot has reviewed your changes, there are still 2 issues that need to be addressed from previous scan.
Open findings:
- Gas underpricing enables compute-heavy verification at a discount
- The example's TEE gate is replayable and does not bind the table signer to the attestation
Sent by Cursor Automation: Security Review Bot
| let payout = balance - fee; | ||
| let pay_coins = coin::extract<AptosCoin>(&mut table_info.escrow, balance); | ||
| let fee_coin = coin::extract<AptosCoin>(&mut pay_coins, fee); | ||
| coin::merge<AptosCoin>(&mut table_info.fee_pool, fee_coin); |
There was a problem hiding this comment.
Table fees locked in pool
Medium Severity
Leave settlement moves the 5% fee into TableInfo.fee_pool, but no entry function withdraws or transfers those coins to the table operator. Documented table fees accumulate in the resource and stay unusable for the TEE table account.
Reviewed by Cursor Bugbot for commit d65fb97. Configure here.
| vector::for_each_ref(&leaving_players, |addr| { | ||
| if (table::contains(&table_info.pending_leave, *addr) && table::contains(&table_info.balances, *addr)) { | ||
| let balance = *table::borrow(&table_info.balances, *addr); | ||
| if (balance > 0) { | ||
| let fee = (balance * FEE_BPS) / BPS_DENOM; | ||
| let payout = balance - fee; | ||
| let pay_coins = coin::extract<AptosCoin>(&mut table_info.escrow, balance); | ||
| let fee_coin = coin::extract<AptosCoin>(&mut pay_coins, fee); | ||
| coin::merge<AptosCoin>(&mut table_info.fee_pool, fee_coin); | ||
| coin::deposit(*addr, pay_coins); | ||
| event::emit(PlayerLeft { table: table_addr, player: *addr, payout, fee }); | ||
| }; | ||
| table::remove(&mut table_info.balances, *addr); | ||
| table::remove(&mut table_info.pending_leave, *addr); |
There was a problem hiding this comment.
Medium severity: players can be locked in escrow indefinitely after requesting leave. request_leave only records intent, and actual payout happens only if the table later calls settle_leaving_players with that address in leaving_players. A malicious or unavailable table can therefore leave exit requests unprocessed forever, trapping the player's APT in escrow with no player-initiated recovery path.
| let table = borrow_global_mut<TableInfo>(table_addr); | ||
| assert!(table::contains(&table.balances, player_addr), error::invalid_argument(EPLAYER_NOT_AT_TABLE)); | ||
| table::add(&mut table.pending_leave, player_addr, true); | ||
| event::emit(LeaveRequested { table: table_addr, player: player_addr }); |
There was a problem hiding this comment.
Duplicate leave request aborts
Medium Severity
request_leave always calls table::add on pending_leave, which aborts when the player key is already present. A second leave request (retry, double submit, or client bug) fails the transaction instead of being a no-op.
Reviewed by Cursor Bugbot for commit 96a6dcc. Configure here.
| let doc_opt = verify_and_parse_attestation_with_roots( | ||
| attestation_doc, | ||
| trusted_root_certs, | ||
| unix_time_secs, | ||
| ); | ||
| if (doc_opt.is_none()) { | ||
| return false | ||
| }; | ||
|
|
||
| let doc = doc_opt.extract(); | ||
| user_data_equals(&doc, expected_user_data) |
There was a problem hiding this comment.
Medium severity: this helper authorizes any Nitro enclave that can choose the expected user_data. After verify_and_parse_attestation_with_roots(...) succeeds, the only policy check here is user_data_equals(&doc, expected_user_data). user_data is caller-controlled input to GetAttestationDocument, so this does not pin PCRs, debug mode, or an attested public key. Any contract that uses verify_attestation_user_data*() as its gate, including the new poker example, can therefore be satisfied by an attacker-built Nitro enclave that simply emits the same user_data, defeating the intended "expected enclave" authorization model.
| let table_signer_addr = signer::address_of(table); | ||
| assert!(table_signer_addr == table_addr, error::permission_denied(ENOT_TABLE_OWNER)); |
There was a problem hiding this comment.
High severity: post-registration authority is not bound to the attested enclave. register_table verifies an attestation once, but TableInfo stores no attested public_key, PCR, or other enclave identity, and the privileged paths later authorize solely by signer::address_of(table) == table_addr. In the shipped flow, that signer is just TABLE_PRIVATE_KEY supplied on the parent host, so anyone who compromises the ordinary table account key can arbitrarily call settle_hand / settle_leaving_players and redirect player escrow without ever controlling the enclave that was originally attested.
| @@ -0,0 +1,638 @@ | |||
| // Copyright (c) Aptos Foundation | |||
| /// Parsed attestation document fields (when validation succeeds). | ||
| struct AttestationDoc has copy, drop, store { | ||
| /// NitroSecureModule identifier (e.g. NSM module ID). | ||
| module_id: vector<u8>, |
There was a problem hiding this comment.
nit: nsm_module_id?
| /// NitroSecureModule identifier (e.g. NSM module ID). | ||
| module_id: vector<u8>, | ||
| /// Unix timestamp in milliseconds when the document was created. | ||
| timestamp: u64, |
There was a problem hiding this comment.
I think it is better to give units in field name: unix_timestamp_millis - seems safer and more exlicit
| root_certs: vector<vector<u8>>, | ||
| } | ||
|
|
||
| const ETRUSTED_ROOTS_ALREADY_INITIALIZED: u64 = 1; |
There was a problem hiding this comment.
For all errors there have to be doc strings so error messages look nice, consider using new `assert!(..., "Your message here"), see https://aptos-labs.github.io/move-book/abort-and-assert.html#abort-messages - maybe no need for constants unless you have some off chain client that needs to parse these.
| /// fun parse_and_use(attestation_doc: vector<u8>) { | ||
| /// let doc_opt = aws_nitro_utils::verify_and_parse_attestation(attestation_doc); | ||
| /// if (doc_opt.is_some()) { | ||
| /// let doc = doc_opt.extract(); |
There was a problem hiding this comment.
extract should be destroy_some
|
|
||
| /// Verifies and parses an AWS Nitro Enclave attestation document against | ||
| /// caller-provided DER-encoded root certificates. | ||
| public native fun verify_and_parse_attestation_with_roots( |
There was a problem hiding this comment.
Same here, keep native private, if you need public version wrap + feature flag.
| } | ||
|
|
||
| fn option_none(context: &SafeNativeContext) -> SafeNativeResult<Value> { | ||
| let enum_option = context.get_feature_flags().is_enum_option_enabled(); |
There was a problem hiding this comment.
it has been enabled, just use Value::struct_(Struct::pack_variant(OPTION_NONE_TAG, vec![]))
| fn option_some(context: &SafeNativeContext, value: Value) -> SafeNativeResult<Value> { | ||
| let enum_option = context.get_feature_flags().is_enum_option_enabled(); | ||
| Ok(if enum_option { | ||
| Value::struct_(Struct::pack_variant(OPTION_SOME_TAG, vec![value])) |
There was a problem hiding this comment.
same as below, enum_option is always true now
| let unix_time_secs = safely_pop_arg!(args, u64); | ||
| let mut trusted_root_certs = vec![]; | ||
| for root in safely_pop_arg!(args, Vec<Value>) { | ||
| trusted_root_certs.push(root.value_as::<Vec<u8>>()?); |
There was a problem hiding this comment.
Do we have a bound on how many roots are passed in here? We charge only after, but this can loop for a while.
| * Same as verify_attestation, but uses caller-provided DER root certificates and an explicit | ||
| * Unix timestamp for certificate validity checks. | ||
| **************************************************************************************************/ | ||
| fn native_verify_attestation_with_roots( |
There was a problem hiding this comment.
This is a copy paste of native below, with only difference as we check is_ok and there we convert to option. Why does this need to be a native? Instead, Move code can use result.is_some() simply?
| if (table::contains(&table_info.pending_leave, *addr) && table::contains(&table_info.balances, *addr)) { | ||
| let balance = *table::borrow(&table_info.balances, *addr); | ||
| if (balance > 0) { | ||
| let fee = (balance * FEE_BPS) / BPS_DENOM; |
There was a problem hiding this comment.
Low severity: very large player balances can make exit settlement revert and trap funds. The fee is computed as (balance * FEE_BPS) / BPS_DENOM using checked u64 arithmetic. Once a player's chip balance exceeds u64::MAX / 500 (36,893,488,147,419,103 octas, about 368.9M APT), that multiplication aborts before any payout happens, so settle_leaving_players cannot process that player and their escrow remains stuck on-chain.
| TrustedRoots[@aptos_framework].root_certs, | ||
| unix_time_secs, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Medium severity: caller-controlled validation time lets on-chain modules accept expired attestations. These new _from_store helpers expose unix_time_secs to Move callers, and the native uses that value directly for X.509 validity checks. A module that passes a stale but in-range timestamp can therefore make an attestation whose signing certificate is expired at chain time verify again, reopening authorization or replay of old attestations. The top-level verify_attestation* wrappers pin consensus time, but this newly exposed API surface does not.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cd7ff02. Configure here.
| let pub_key = NitroPublicKey::new(attestation_doc_signing_cert.public_key())?; | ||
| validate_cose_signature(&pub_key, &cose_sign_1_decoded)?; | ||
|
|
||
| Ok(decoded_attestation_doc) |
There was a problem hiding this comment.
Stale attestation documents accepted
Medium Severity
validate_and_parse_attestation_doc_with_roots checks the signing certificate only against unix_time_secs and never compares the decoded document’s millisecond timestamp to consensus time. A previously issued document can stay valid for replay while its chain is still within cert validity, which weakens ACE-style and key-release policies that rely on this helper alone.
Reviewed by Cursor Bugbot for commit cd7ff02. Configure here.
There was a problem hiding this comment.
Aptos Security Bugbot has reviewed your changes, there are still 4 issues that need to be addressed from previous scan.
Open findings:
- This helper authorizes any Nitro enclave that can choose the expected
user_data - Post-registration authority is not bound to the attested enclave
- Players can be locked in escrow indefinitely after requesting leave
- Very large player balances can make exit settlement revert and trap funds
Sent by Cursor Automation: Security Review Bot




Description
Supersedes #18801.
Adds AWS Nitro attestation verification support to
aptos_framework::aws_nitro_utils. The Move API keeps trusted Nitro root certificates in framework-managed on-chain storage and wraps deterministic native verification that takes explicit trusted roots plus an explicit Unix timestamp.The native implementation validates the attestation document certificate chain, verifies the COSE signature, and exposes parsed PCR, public key, user data, and nonce fields so Move policy can authorize TEE jobs before ACE-style key release flows.
Also updates the poker Move example to use Shelby/TEE-style table registration semantics: the table enclave must submit a Nitro attestation whose
user_datais bound tob"APTOS_POKER_TABLE_V1" || bcs(table_address). The example clients and docs now describe the chain-managed Nitro root store and real attestation document path.How Has This Been Tested?
cargo check -p aptos-framework-natives --libcargo build -p aptos --bin aptostarget/debug/aptos move test --package-dir aptos-move/framework/aptos-framework --skip-fetch-latest-git-deps --filter aws_nitrocargo test -p aptos-move-examples test_poker -- --nocapturegit diff --checkrustfmt --check --config skip_children=true aptos-move/framework/natives/src/aws_nitro_utils.rs aptos-move/framework/natives/src/lib.rs aptos-move/move-examples/tests/move_unit_tests.rsKey Areas to Review
aptos_framework::aws_nitro_utilsaptos-move/framework/natives/src/aws_nitro_utils.rsverify_attestation_user_data*user_dataType of Change
Which Components or Systems Does This Change Impact?
Note
High Risk
Adds security-critical attestation and PKIX/COSE verification in the VM and framework-managed trust roots; misconfiguration or bugs could allow bogus TEE authorization or break legitimate enclave flows.
Overview
Adds
aptos_framework::aws_nitro_utils, a new framework module for verifying AWS Nitro COSE attestation documents on-chain. Governance (or testnet-only helpers) can store trusted Nitro root DER certs; public APIs verify against that store or caller-supplied roots and consensus time, with optional parsing of PCRs, nonce,user_data, and public key plus helpers likeverify_attestation_user_dataandverify_attestation_matches.VM natives implement certificate-chain validation (webpki), COSE signature checks, and structured
AttestationDocreturn values. Gas for verify vs verify-and-parse is calibrated from a ~5KB attestation benchmark. New crypto crates and a criterion bench support the native path.The poker example is extended so table registration requires a valid attestation whose
user_dataisAPTOS_POKER_TABLE_V1 || bcs(table_address), with JS clients, a Go Nitro attester, andLOCAL_AWS_E2E.mddocumenting localnet + AWS smoke flow. Move example tests gain a larger-stacktest_pokerharness.Reviewed by Cursor Bugbot for commit cd7ff02. Bugbot is set up for automated code reviews on this repo. Configure here.