From 23ed35aa763fe02bad52fbf78b6c6d3f7ed6cc3a Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Sun, 20 Sep 2026 00:04:36 -0700 Subject: [PATCH 1/2] fix(kms): bound compose_hash and instance_id before the policy backend Only `app_id` was length-checked, and both Ethereum backends `padStart` a short hex value to full width before the `readContract` call. A 31-byte compose hash is therefore submitted as - and is indistinguishable from - the whitelisted 32-byte hash whose leading byte is `0x00`. The trusted OS always emits a 32-byte sha256 and either a 20-byte instance id or none, but nothing between the event log and the contract call asserted that. Assert it at `AuthApi::is_app_allowed`, the one point every backend call goes through, and stop `DstackApp.addComposeHash`/`addDevice` from allowlisting `bytes32(0)` - which is exactly what that padding produces - since `_initializeCommon` already declines to seed it. --- dstack/kms/auth-eth/contracts/DstackApp.sol | 7 ++ dstack/kms/auth-eth/test/DstackApp.t.sol | 42 +++++++++++ .../kms/src/main_service/upgrade_authority.rs | 70 +++++++++++++++++++ 3 files changed, 119 insertions(+) diff --git a/dstack/kms/auth-eth/contracts/DstackApp.sol b/dstack/kms/auth-eth/contracts/DstackApp.sol index 6c27e9ccb..8667b6480 100644 --- a/dstack/kms/auth-eth/contracts/DstackApp.sol +++ b/dstack/kms/auth-eth/contracts/DstackApp.sol @@ -145,6 +145,10 @@ contract DstackApp is // Add a compose hash to allowed list function addComposeHash(bytes32 composeHash) external onlyOwner { + // Zero is what a short or absent composeHash left-pads to, and + // {_initializeCommon} already refuses to seed it for that reason. + // Allowlisting it here would make that padding an accept. + require(composeHash != bytes32(0), "invalid compose hash"); allowedComposeHashes[composeHash] = true; emit ComposeHashAdded(composeHash); _emitPolicy("compose-hash", composeHash, true); @@ -173,6 +177,9 @@ contract DstackApp is // Add a device ID to allowed list function addDevice(bytes32 deviceId) external onlyOwner { + // Same reasoning as {addComposeHash}: zero is the padded form of an + // absent deviceId, and {_initializeCommon} skips it. + require(deviceId != bytes32(0), "invalid device ID"); allowedDeviceIds[deviceId] = true; emit DeviceAdded(deviceId); _emitPolicy("device", deviceId, true); diff --git a/dstack/kms/auth-eth/test/DstackApp.t.sol b/dstack/kms/auth-eth/test/DstackApp.t.sol index 8ad8050e7..0fc57faab 100644 --- a/dstack/kms/auth-eth/test/DstackApp.t.sol +++ b/dstack/kms/auth-eth/test/DstackApp.t.sol @@ -391,4 +391,46 @@ contract DstackAppTest is Test { app.addComposeHash(bytes32(uint256(1))); assertTrue(app.allowedComposeHashes(bytes32(uint256(1)))); } + + // Both Ethereum auth backends left-pad a short hex value to the full width + // before calling in, so bytes32(0) is what an absent or truncated + // composeHash/deviceId arrives as. `_initializeCommon` already declines to + // seed either from zero; the setters used to disagree, which meant an owner + // could allowlist exactly the value that padding produces. + function test_AddZeroValuedPolicyEntriesIsRejected() public { + vm.startPrank(owner); + + vm.expectRevert("invalid compose hash"); + app.addComposeHash(bytes32(0)); + + vm.expectRevert("invalid device ID"); + app.addDevice(bytes32(0)); + + vm.stopPrank(); + + assertFalse(app.allowedComposeHashes(bytes32(0))); + assertFalse(app.allowedDeviceIds(bytes32(0))); + + // A boot with no composeHash arrives as bytes32(0) and stays denied. + IAppAuth.AppBootInfo memory bootInfo = IAppAuth.AppBootInfo({ + appId: address(app), + composeHash: bytes32(0), + instanceId: address(0), + deviceId: bytes32(0), + mrAggregated: bytes32(0), + mrSystem: bytes32(0), + osImageHash: bytes32(0), + tcbStatus: "UpToDate", + advisoryIds: new string[](0) + }); + (bool allowed, string memory reason) = app.isAppAllowed(bootInfo); + assertFalse(allowed); + assertEq(reason, "Compose hash not allowed"); + } + + // The initializer's zero-skip is the behaviour the setters now match. + function test_InitializeSkipsZeroValuedPolicyEntries() public view { + assertFalse(app.allowedComposeHashes(bytes32(0))); + assertFalse(app.allowedDeviceIds(bytes32(0))); + } } diff --git a/dstack/kms/src/main_service/upgrade_authority.rs b/dstack/kms/src/main_service/upgrade_authority.rs index 135f49de4..a66239a00 100644 --- a/dstack/kms/src/main_service/upgrade_authority.rs +++ b/dstack/kms/src/main_service/upgrade_authority.rs @@ -49,6 +49,33 @@ pub(crate) fn ensure_app_id_len(app_id: &[u8]) -> Result<()> { Ok(()) } +/// Check the identity widths the policy backends assume, at the point where a +/// `BootInfo` stops being measured bytes and becomes contract arguments. +/// +/// Both Ethereum backends `padStart` a short hex value to the full width before +/// the `readContract` call, and their schemas bound only the maximum. So a +/// 31-byte `compose_hash` is submitted as - and is indistinguishable from - the +/// whitelisted 32-byte hash whose leading byte is `0x00`, and the same holds for +/// a short `instance_id` against `address`. +/// +/// The trusted OS always emits a 32-byte sha256 compose hash and either a +/// 20-byte instance id or none at all (`--no-instance-id`), so this asserts an +/// invariant that already holds rather than changing behaviour. It is asserted +/// here because nothing between the event log and the contract call asserted +/// it, which is the same gap `ensure_app_id_len` closes for `app_id`. `app_id` +/// is re-checked here so that every field reaching a backend is checked in one +/// place. +fn ensure_identity_widths(boot_info: &BootInfo) -> Result<()> { + ensure_app_id_len(&boot_info.app_id)?; + if boot_info.compose_hash.len() != 32 { + bail!("compose_hash must be 32 bytes"); + } + if !boot_info.instance_id.is_empty() && boot_info.instance_id.len() != 20 { + bail!("instance_id must be 20 bytes or empty"); + } + Ok(()) +} + pub(crate) async fn local_kms_boot_info(verifier: &AttestationVerifier) -> Result { let response = app_attest(pad64([0u8; 32])) .await @@ -136,6 +163,7 @@ async fn send_request(req: reqwest::RequestBuilder, url: &s impl AuthApi { pub async fn is_app_allowed(&self, boot_info: &BootInfo, is_kms: bool) -> Result { + ensure_identity_widths(boot_info)?; match self { AuthApi::Dev { dev } => Ok(BootResponse { is_allowed: true, @@ -363,6 +391,48 @@ mod tests { } } + /// Both Ethereum backends left-pad a short value to the full width, so a + /// short `compose_hash` or `instance_id` aliases onto a whitelisted one. + /// Nothing below the backend bounded them, so the KMS refuses to submit + /// them rather than letting `padStart` decide what they mean. + #[rocket::async_test] + async fn short_identities_are_refused_before_they_reach_the_backend() { + let (url, server) = serve(vec![ + r#"{"isAllowed":true,"gatewayAppId":"gateway","reason":"well-formed"}"#, + ]); + let auth = webhook(url); + + let mut aliasing_compose_hash = boot_info(1); + aliasing_compose_hash.compose_hash.truncate(31); + let err = auth + .is_app_allowed(&aliasing_compose_hash, false) + .await + .unwrap_err(); + assert!(err.to_string().contains("compose_hash must be 32 bytes")); + + let mut aliasing_instance_id = boot_info(1); + aliasing_instance_id.instance_id.truncate(19); + let err = auth + .is_app_allowed(&aliasing_instance_id, false) + .await + .unwrap_err(); + assert!(err + .to_string() + .contains("instance_id must be 20 bytes or empty")); + + // `--no-instance-id` deployments legitimately carry none, and a + // well-formed payload still reaches the backend. + let mut no_instance_id = boot_info(1); + no_instance_id.instance_id.clear(); + assert!( + auth.is_app_allowed(&no_instance_id, false) + .await + .unwrap() + .is_allowed + ); + assert_eq!(server.join().unwrap().len(), 1); + } + #[rocket::async_test] async fn repeated_authorization_is_never_served_from_a_decision_cache() { let (url, server) = serve(vec![ From 81173da3b0aebe5863e5fec4019e49f13017f2f4 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Tue, 22 Sep 2026 02:37:59 -0700 Subject: [PATCH 2/2] refactor(kms): trim identity width checks and tests --- dstack/kms/auth-eth/contracts/DstackApp.sol | 5 --- dstack/kms/auth-eth/test/DstackApp.t.sol | 33 ------------------- .../kms/src/main_service/upgrade_authority.rs | 27 +++------------ 3 files changed, 4 insertions(+), 61 deletions(-) diff --git a/dstack/kms/auth-eth/contracts/DstackApp.sol b/dstack/kms/auth-eth/contracts/DstackApp.sol index 8667b6480..7d90f29ef 100644 --- a/dstack/kms/auth-eth/contracts/DstackApp.sol +++ b/dstack/kms/auth-eth/contracts/DstackApp.sol @@ -145,9 +145,6 @@ contract DstackApp is // Add a compose hash to allowed list function addComposeHash(bytes32 composeHash) external onlyOwner { - // Zero is what a short or absent composeHash left-pads to, and - // {_initializeCommon} already refuses to seed it for that reason. - // Allowlisting it here would make that padding an accept. require(composeHash != bytes32(0), "invalid compose hash"); allowedComposeHashes[composeHash] = true; emit ComposeHashAdded(composeHash); @@ -177,8 +174,6 @@ contract DstackApp is // Add a device ID to allowed list function addDevice(bytes32 deviceId) external onlyOwner { - // Same reasoning as {addComposeHash}: zero is the padded form of an - // absent deviceId, and {_initializeCommon} skips it. require(deviceId != bytes32(0), "invalid device ID"); allowedDeviceIds[deviceId] = true; emit DeviceAdded(deviceId); diff --git a/dstack/kms/auth-eth/test/DstackApp.t.sol b/dstack/kms/auth-eth/test/DstackApp.t.sol index 0fc57faab..646bd827e 100644 --- a/dstack/kms/auth-eth/test/DstackApp.t.sol +++ b/dstack/kms/auth-eth/test/DstackApp.t.sol @@ -392,45 +392,12 @@ contract DstackAppTest is Test { assertTrue(app.allowedComposeHashes(bytes32(uint256(1)))); } - // Both Ethereum auth backends left-pad a short hex value to the full width - // before calling in, so bytes32(0) is what an absent or truncated - // composeHash/deviceId arrives as. `_initializeCommon` already declines to - // seed either from zero; the setters used to disagree, which meant an owner - // could allowlist exactly the value that padding produces. function test_AddZeroValuedPolicyEntriesIsRejected() public { vm.startPrank(owner); - vm.expectRevert("invalid compose hash"); app.addComposeHash(bytes32(0)); - vm.expectRevert("invalid device ID"); app.addDevice(bytes32(0)); - vm.stopPrank(); - - assertFalse(app.allowedComposeHashes(bytes32(0))); - assertFalse(app.allowedDeviceIds(bytes32(0))); - - // A boot with no composeHash arrives as bytes32(0) and stays denied. - IAppAuth.AppBootInfo memory bootInfo = IAppAuth.AppBootInfo({ - appId: address(app), - composeHash: bytes32(0), - instanceId: address(0), - deviceId: bytes32(0), - mrAggregated: bytes32(0), - mrSystem: bytes32(0), - osImageHash: bytes32(0), - tcbStatus: "UpToDate", - advisoryIds: new string[](0) - }); - (bool allowed, string memory reason) = app.isAppAllowed(bootInfo); - assertFalse(allowed); - assertEq(reason, "Compose hash not allowed"); - } - - // The initializer's zero-skip is the behaviour the setters now match. - function test_InitializeSkipsZeroValuedPolicyEntries() public view { - assertFalse(app.allowedComposeHashes(bytes32(0))); - assertFalse(app.allowedDeviceIds(bytes32(0))); } } diff --git a/dstack/kms/src/main_service/upgrade_authority.rs b/dstack/kms/src/main_service/upgrade_authority.rs index a66239a00..77306d870 100644 --- a/dstack/kms/src/main_service/upgrade_authority.rs +++ b/dstack/kms/src/main_service/upgrade_authority.rs @@ -49,28 +49,14 @@ pub(crate) fn ensure_app_id_len(app_id: &[u8]) -> Result<()> { Ok(()) } -/// Check the identity widths the policy backends assume, at the point where a -/// `BootInfo` stops being measured bytes and becomes contract arguments. -/// -/// Both Ethereum backends `padStart` a short hex value to the full width before -/// the `readContract` call, and their schemas bound only the maximum. So a -/// 31-byte `compose_hash` is submitted as - and is indistinguishable from - the -/// whitelisted 32-byte hash whose leading byte is `0x00`, and the same holds for -/// a short `instance_id` against `address`. -/// -/// The trusted OS always emits a 32-byte sha256 compose hash and either a -/// 20-byte instance id or none at all (`--no-instance-id`), so this asserts an -/// invariant that already holds rather than changing behaviour. It is asserted -/// here because nothing between the event log and the contract call asserted -/// it, which is the same gap `ensure_app_id_len` closes for `app_id`. `app_id` -/// is re-checked here so that every field reaching a backend is checked in one -/// place. +/// Policy backends left-pad short hex values, so a short field would alias a +/// whitelisted full-width one. fn ensure_identity_widths(boot_info: &BootInfo) -> Result<()> { ensure_app_id_len(&boot_info.app_id)?; if boot_info.compose_hash.len() != 32 { bail!("compose_hash must be 32 bytes"); } - if !boot_info.instance_id.is_empty() && boot_info.instance_id.len() != 20 { + if !matches!(boot_info.instance_id.len(), 0 | 20) { bail!("instance_id must be 20 bytes or empty"); } Ok(()) @@ -391,10 +377,6 @@ mod tests { } } - /// Both Ethereum backends left-pad a short value to the full width, so a - /// short `compose_hash` or `instance_id` aliases onto a whitelisted one. - /// Nothing below the backend bounded them, so the KMS refuses to submit - /// them rather than letting `padStart` decide what they mean. #[rocket::async_test] async fn short_identities_are_refused_before_they_reach_the_backend() { let (url, server) = serve(vec![ @@ -420,8 +402,7 @@ mod tests { .to_string() .contains("instance_id must be 20 bytes or empty")); - // `--no-instance-id` deployments legitimately carry none, and a - // well-formed payload still reaches the backend. + // `--no-instance-id` deployments carry no instance id. let mut no_instance_id = boot_info(1); no_instance_id.instance_id.clear(); assert!(