NNS FE Dapp XSS vulnerability Incident Retrospective - Friday August 19, 2022

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.
15 Likes

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.

2 Likes

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?

2 Likes

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…

4 Likes

This documentation is a good resource: Web App Development Security Best Practices | Internet Computer Home

1 Like

React is sanitized/escaped by default https://reactjs.org/docs/introducing-jsx.html#jsx-prevents-injection-attacks

2 Likes

Does that document need updating? It already says the following but it sounds like that wasn’t enough.

Recommendation

  • Use a web framework that has a secure templating mechanism such as Svelte to avoid XSS. This is used e.g. in the NNS dApp project.

  • Don’t use insecure features of the framework, such as e.g. @html in Svelte.

1 Like

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!

It does already say the following (updated my originally comment)

  • Don’t use insecure features of the framework, such as e.g. @html in Svelte.
1 Like

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 :smiley:

3 Likes

Thanks for the kind words :pray:. I’ll send a “nice catch!” to the security team again

4 Likes

so did you use @html{} in svelte for displaying the proposal title or why was there an issue?

EDIT: just saw you linked the commit

yep, still ashamed of it. just noticing your comment gives me bad vibes for the day.

oh man i’m sorry … as already mentioned, we all make mistakes and learn from them! no need to be ashamed or having bad day at all :relaxed:

3 Likes

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.

2 Likes

Not unverifiable. As mentioned in my post:

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.

Hope that answer your question and concern?

1 Like

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 :slight_smile:

I’m with you, don’t have better idea either. Thanks for the feedback, glad it answered it.

1 Like