During an internal security review an XSS vulnerability was discovered in the NNS FE dapp. The vulnerability was caused by an unsanitized label that contains the title of the proposal.
Actions
The following actions were performed:
Sanitization fix was quickly implemented and tested to verify that the attack is no longer possible
Code review and build verifiability checks were performed in a private repository
The fix was deployed in production
The code was merged into a public repository after 2h quiet time (no issues discovered or reported)
Checking the hash of the deployed wasm is possible as described in the upgrade proposal. The commit is also tagged as “Proposal-76202”.
What went wrong originally?
The bug slipped through the tests and code review.
Where did we get lucky?
The bug was found by DFINITY security engineers while auditing the SNS features currently in development.
What went right?
The rollout of the fix was smooth.
Impact
No impact on the existing users.
The security team performed an inspection of all submitted proposals and no malformed proposal titles were detected
As the session lasts max. 30 min, no users have the old dapp version running
Action Items
The entire codebase was inspected for sanitization checks and they were in place for all other fields. Moving forward, a code review process will be hardened with respect to adding new displayed fields to make sure such issue never happens again.
Furthermore, a more frequent regular security audit will be performed on system dapps.
So someone could have named their proposal an html tag that loaded a malicious script that voted for the logged in user? Just trying to understand as this sounds like a sanitation that any dapp developer might need to implement.
Yes. In my opinion it would not have been that straight forward as such a short answer. Someone would have had to found and know how to inject such a script, would have had to retro-engineer the minified js on mainnet to find which function is the one to vote, found a way to also prepare the parameters that the function need and finally put together and execute everything without the user noticing it but, yes duable for an attacker. Being said, don’t misunderstand my answer, I don’t want to minimize it. We took and take that incident more than seriously.
To be honest with you, given the incident, I would be quite pretentious to give anyone advice right now but, agree. If any dapp developer display proposal in their apps, sanitizing any of the related user input fields (e.g. title, description, payload) is probably for the best.
As a broad example (Examples for Instruction not accusation) Could someone put a tag in their post on dscvr that send stoic funds?
Do you have a good example of “sanitizing” the display of content coming from a dapp?
This might be a great place for a best practice and generic library for wrapping things like react/svelt. Maybe those platforms have built in xss support?
Impostor syndrome hits quite hard right now so, instead of answering quickly here, maybe I/we can prepare and share a (blog) post about one or two strategies with Svelte to prevent XSS on the frontend side?
PS.: in the documentation I think there is also a best practice somewhere, trying to find it…
Yes and no, good point. No the document is correct unless you use {@html ...} in Svelte or dangerouslySetInnerHTML in React, so in that sense yes an update would be nice. I will check next week with editorial team to add a sentence about these. Thanks for the input!
There’s a thing with GA pilots that fly solo and have a rough flight / bad landing, where an experienced instructor will take them out on a flight ASAP, so that the pilot regains their trust in the process, so don’t worry too much about it, send a “nice catch!” to whoever caught the vulnerability, and rock on! There’s a lot of us on team you, right now
I’m wondering how security patches like this can be applied in the future. I mean deploying a secret version of an open sourced canister… completely unverifiable. How can be blessed by the broad community? How other projects handle this?
In case of github if the collaborators can convince the community to vote yes blindly then collaborators basically can deploy anything. Also the number of developers who can see the patch should be kept as low as possible. So eventually very few trusted people can deploy anything.
No offence please, I’m just curious what are the plans / options to balance between security and decentralization.
As mentioned in the proposal, the process followed the Security Patch Policy and Procedure that was adopted in proposal 48792 (which also reference forum post 11069) - i.e. this proposal defines the plans / options to handle such critical incident.
Thanks for the link of the security policy, I missed that! Also I thought the patch was a simple wasm update proposal.
Yes, you answered my concern, it seems we have to accept the tradeoff: security patches require trust and centralised decisions which can be verified later. Not a big surprise, I don’t have better idea either.
I hoped there is a tricky and wow solution. Luckily you and dfinity teams have my full trust
. I’ll send a “nice catch!” to the security team again
