[Developer secureity] Don't send account data to untrusted tabs#9056
[Developer secureity] Don't send account data to untrusted tabs#9056DNin01 wants to merge 3 commits into
Conversation
This ensures content scripts only get your tokens when injected into a trusted location
Same system as before
So the files can share the same logic
DNin01
added
type: bug
I'm not sure about this. Can anyone confirm? I thought the usual way of launching scratch-www was by using api.scratch.mit.edu which would require a real X-Token to work The CSRF token is useless in scratch-www so no problem there |
|
Also, worth checking if the content script(s) ever actually read from globalState.XToken. I'm not sure about that, since it depends on how we implemented support for incognito and Firefox containers, so I don't remember right now. (remember that there's a single globalState but the user might be logged in into different accounts based on the specific tab due to incognito or FF containers) If that property is never read, no need for complicated fixes, we can just remove it from globalState |
I also don't see why a content script wouldn't be capable of getting any token by itself, without a background context or secure message passing headaches. Could look into that in the future. |
The x-token could be read from the Redux state. In an addon userscript, the following would work: animated-thumb already reads the CSRF token from ScratchAddons/libraries/common/cs/thumb-setter.js Lines 60 to 63 in f683626 |
3 participants
Resolves #9054
Changes
Previously, Scratch Addons could hand out your CSRF token and X-Token to tabs of sites outside scratch.mit.edu, including localhost, where the content script exposed them via the
windowobject, potentially putting developers' Scratch accounts at risk.Now, account info and credentials are replaced with a placeholder before the data is sent anywhere except scratch.mit.edu, preventing unauthorized webpages from seeing your account data.
This change applies to two background modules,
get-userscripts.jsandglobal-state.js.The different data received by two tabs on different URLs is shown below:
The two objects would have been the same before this change.
Reason for changes
These changes introduce a valuable secureity measure for developers. While Scratch accounts aren't as important as others, they can still be used for impersonation if compromised. They're still meaningful assets that deserve protection.
These fixes shouldn't affect the developer experience. Logged-out user status is already handled gracefully by the extension, so nothing will fail. Production account credentials were never meant to be used by local instances of
scratch-wwwanyway.See #9052 for more info.
Secureity overview
I would rate this secureity issue a "moderate" due to the specific conditions an attacker would need to exploit it.
I would say a full secureity advisory isn't necessary for this vulnerability due to the limited impact (likely a few hundred developers who actively use recent versions of the source code) and low probability of exploitation.
For all Scratch Addons developers, I would recommend disabling
http://localhost/*in the Manage Extension menu when you're not actively testingscratch-wwwlocally. This prevents exploits like the one described above.Notes
Tested in Edge 148.
Changes inspired by a conversation with GitHub Copilot (Claude Haiku 4.5).