Address audit findings in export-and-sign/import#117
Conversation
ethankonk
commented
Mar 4, 2026
ethankonk
commented
- Constant-time string comparison (timingSafeEqual) for org ID and enclave quorum public key checks to prevent timing side-channel attacks
- In-place key map mutation instead of spread-copy to avoid multiplying key material references on the V8 heap
- Zero sensitive Uint8Array buffers (secretKey, privateKeyBytes, pkcs8) on all code paths including error paths via finally blocks
- Origin validation on incoming postMessage events using allowedOrigin captured during the TURNKEY_INIT_MESSAGE_CHANNEL handshake
- Use captured parent origin as targetOrigin in sendMessageUp instead of wildcard '*' to prevent message eavesdropping
- Reduce embedded key localStorage TTL from 48h to 4h to limit exposure window if storage is compromised
- Add tests for all new security behaviors: timing-safe comparison, parent origin capture/send, TTL enforcement, key clearing with buffer zeroing, and origin-scoped postMessage
ethankonk
force-pushed
the
ethan/security-audit
branch
from
a07ea64 to
d46f620
Compare
ethankonk
force-pushed
the
ethan/security-audit
branch
from
d46f620 to
1a8dac4
Compare
ethankonk
changed the title
andrewkmin
requested review from
andrewkmin and
Copilot
and removed request for
Copilot
andrewkmin
left a comment
andrewkmin
left a comment
There was a problem hiding this comment.
a couple gaps in tkintern's implementation 😛
main takeaways:
- reduce the scope of this to exclude origin checks (perhaps in a separate PR)
- on constant time helper function: let's DRY it up, fix/test the implementation, and move it to
shared - we could explore creating a new class for the inMemoryKeys object and override the
deletemethod (instead of callingzeroKeyEntry()). don't feel too strongly here
| let diff = 1; | ||
| const len = Math.min(aBuf.length, bBuf.length); | ||
| for (let i = 0; i < len; i++) { | ||
| diff |= aBuf[i] ^ bBuf[i]; |
There was a problem hiding this comment.
This one is a valid finding -- we don't do anything with the diff that we mutate
| let diff = 1; // already know they differ | ||
| const len = Math.min(aBuf.length, bBuf.length); | ||
| for (let i = 0; i < len; i++) { | ||
| diff |= aBuf[i] ^ bBuf[i]; |
There was a problem hiding this comment.
same here -- we should also DRY this up (write once, then import)
| */ | ||
| function initMessageEventListener(HpkeDecrypt) { | ||
| return async function messageEventListener(event) { | ||
| // SECURITY: Validate event.origin against the allowlist captured at init time. |
There was a problem hiding this comment.
let's remove this from the scope of this change for now
| event.data["type"] == "TURNKEY_INIT_MESSAGE_CHANNEL" && | ||
| event.ports?.[0] | ||
| ) { | ||
| // SECURITY: Capture the parent origin from the init handshake. |
There was a problem hiding this comment.
same with this. I think we can ticket it / consider tackling in a separate workstream
| if (enclaveQuorumPublic !== TURNKEY_SIGNER_ENCLAVE_QUORUM_PUBLIC_KEY) { | ||
| // SECURITY: Use constant-time comparison to prevent timing side-channel | ||
| // attacks that could leak the enclave quorum public key byte-by-byte. | ||
| if ( |
There was a problem hiding this comment.
this is public information; we can remove the timingSafeEqual check
| let diff = 1; | ||
| const len = Math.min(aBuf.length, bBuf.length); | ||
| for (let i = 0; i < len; i++) { | ||
| diff |= aBuf[i] ^ bBuf[i]; |
There was a problem hiding this comment.
same comment as in the other iframe: let's revisit this logic
| * @param {string} b | ||
| * @returns {boolean} true if strings are equal | ||
| */ | ||
| function timingSafeEqual(a, b) { |
There was a problem hiding this comment.
actually on second thought, we can fix this implementation and eventually throw it into https://github.com/tkhq/frames/tree/main/shared
| // remove the message event listener that was added in the DOMContentLoaded event | ||
| messageListenerController.abort(); | ||
|
|
||
| // Capture the parent origin for use as targetOrigin in sendMessageUp, |
There was a problem hiding this comment.
want to roll this out separately ideally (just to reduce blast radius)
| // The decrypted payload is a raw 32-byte P-256 private key scalar. | ||
| const decrypted = await decryptBundle(bundle, organizationId, HpkeDecrypt); | ||
| keyBytes = | ||
| decrypted instanceof Uint8Array ? decrypted : new Uint8Array(decrypted); |
There was a problem hiding this comment.
ah, this accentuates the need for type safety :')
| } | ||
| } finally { | ||
| // SECURITY: Zero the raw private key bytes immediately after encoding. | ||
| // The encoded `key` is a JS string (hex or base58) which we cannot zero, |
There was a problem hiding this comment.
maybe we shouuuld rearchitect such that no private key material is ever a JS string? will have to think about this one; ultimately if it's displayed as a string in an iframe (which it sort of needs to be in order to be a functional export iframe), then we don't gain much from this
2 participants
