Fixed an XSS vulnerability when setting SVG icon

[diwangs]

I've discovered a stored XSS vulnerability in src/svgbundle.ts:

Vulnerability Details:
Severity: [High/Critical – Stored XSS can have a significant impact. Adjust based on your assessment]
The function SvgIconRegistry.registerIconFromSvg() is used to register and replace icons within the SVG registry, such as the default checkbox icon. However, the user of the library might inject a malicious SVG into the site, which cause an attacker to be able to execute arbitrary script under the domain (XSS)

Steps to Reproduce:
Let's say we have an SVG in this form:

<svg >
  <a href='javascript:alert(window.origin)'>
    <text>hello</text>
  </a>
</svg>

Such SVG could be registered under the registry with this script:

import { SvgRegistry } from 'survey-core'
SvgRegistry.registerIconFromSvg("v2check", maliciousSvg)

When we display a checkbox in the survey, after the user clicks them, an alert would show up. suggesting that the script has been successfully executed.

Here's a screenshot example if we display the checkbox using the example in the Surveyjs documentation:

Suggested Fix:
Before an SVG got registered into the registry, it is best to sanitize them first, using a library such as DOMPurify to prevent script execution. I have implemented a simple patch to fix the vulnerability using this method.

[andrewtelnov]

@diwangs We will take a look and comeback. We don't want to add a new library into our package just to call a one function from it.

Thank you,
Andrew

[tsv2013]

Hello @diwangs ! Thank you for the contribution!
Am I right that you mean fix the possibility to inject malicious code via JavaScript on a page? Like below:

SvgRegistry.registerIconFromSvg("v2check", maliciousSvg)

But if you are a programmer and you have access to a code (can pass some code) that will be executed on a client machine, you can inject any malicious code you want?
If this is the only case you wanted to fix than I'm afraid it's not a XSS vulnerability, you are talking about a "criminal programmer" that can inject any malicious code on your site. And tnis PR can't prevent it.

Please correct me if I'm wrong.

[diwangs]

Thank you for your response @tsv2013 !

In a threat model where the programmer is the attacker, you are correct. The programmer could devise a more straightforward way to attack the victim.

However, I would argue that it is not the only threat model that could be applied since you could call SvgRegistry.registerIconFromSvg("v2check", maliciousSvg) in many different context. I'll try to illuminate 2:

1. Malicious icon pack: The developer could've used a third party icon library to theme the survey. In this scenario, the developer is honest, but unknowingly includes malicious content from other party.
2. User-defined icon: The developer could built a survey that takes user icon file then save and display them on later stage of that survey. This might not make sense for the "v2check" example, but it does for something like "logo" where the developer would program a way to let user uploads and display their own logo. In this case, the developer is honest and so does the user, but the user could be tricked into including malicious content.

One alternative approach aside from this PR is just to make a clear warning in the docs and let the survey maker sanitize the SVG themselves. However, I think sanitizing it in this library is a better way to prevent it since it is the lowest common denominator.

Let me know what you think

[diwangs]

As for the concern for unnecessarily adding library @andrewtelnov, one alternative approach is to build our own lightweight sanitizer (using something like regex) that only handles SVG.

[tsv2013]

@diwangs My apologies for a delayed responce.
In our current state of things these cases

• Malicious icon pack
• User-defined icon
  can be implemented by a programmer only. As you told earlier - a programmer could devise a more straightforward way to attack the victim.

That's why I'd prefer not to add a new dependency and not to write addional code right now.

In the future when we'll support a custom icon pack or allow users to load custom SVG icons your concern will definitely make sence. And we'll need to solve this probles somehow.