fix: prevent concurrent TURNKEY_INIT_MESSAGE_CHANNEL from establishing multiple channels#119
Open
leeland-turnkey wants to merge 2 commits intomainfrom
Conversation
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.
Description:
Fixes ENG-3785
This is from the Audit here: https://cantina.xyz/code/0b2d181e-b8dd-4aeb-bbff-23a5fe8be69d/findings/22
Problem
The
TURNKEY_INIT_MESSAGE_CHANNELbootstrap handler isasync. If twoinit messages arrive before the first
awaitresolves (e.g. from alegitimate parent and a malicious sibling iframe), both invocations can
pass the outer
ifcheck beforeturnkeyInitController.abort()is evercalled. This allows multiple origins to establish a message channel and
register a port — including overwriting the legitimate parent's port.
Fix
Add a
channelEstablishedboolean flag that is checked and setsynchronously before the first
await. Because JavaScript issingle-threaded, this check-and-set is atomic: any concurrent invocation
will see the flag already
trueand return immediately, regardless ofwhether the first handler is still suspended at an
await.Why both HTML and JS files are changed
The four frames split across two implementation patterns:
export-and-signandimportuse modular source files(
src/event-handlers.js,src/index.js) that are bundled by webpack.authandexportembed their bootstrapping logic directly ininline
<script>tags insideindex.html/index.template.html.The same vulnerability existed in all four, so the same fix was applied
to both patterns.
Tests
Three unit tests added to
export-and-signcovering:awaitresolves is ignored (the core regression test)is also ignored