Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security Fix for Cross-site Scripting (XSS) - huntr.dev #29

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

huntr-helper
Copy link

https://huntr.dev/users/d3v53c has fixed the Cross-site Scripting (XSS) vulnerability 🔨. Think you could fix a vulnerability like this?

Get involved at https://huntr.dev/

Q | A
Version Affected | ALL
Bug Fix | YES
Original Pull Request | 418sec#1
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/npm/zingchart-react/1/README.md

User Comments:

📊 Metadata *

zingchart-react is vulnerable to Cross-Site Scripting (XSS).

Bounty URL: https://www.huntr.dev/bounties/1-npm-zingchart-react

⚙️ Description *

Cross-Site Scripting (XSS) attacks are a type of injection, in which malicious scripts are injected into otherwise benign and trusted websites. XSS attacks occur when an attacker uses a web application to send malicious code, generally in the form of a browser side script, to a different end user. Flaws that allow these attacks to succeed are quite widespread and occur anywhere a web application uses input from a user within the output it generates without validating or encoding it.

💻 Technical Description *

Cross-Site Scripting (XSS) attacks are mitigated by sanitizing the user inputs before rendering, thereby preventing malicious execution.

🐛 Proof of Concept (PoC) *

Open https://github.com/zingchart/zingchart-react
Open link in about https://www.zingchart.com/docs/integrations/react
Open in Sandbox https://codesandbox.io/s/zingchart-react-wrapper-example-dxfc9?from-embed
Insert the xss payload in any of the values field in series in Simple.jsx. EX:

values: [4, '><img src=x onerror=alert(1)>', 3, 4, 5, 3, 5, 4, 11]

XSS payload will get executed.

🔥 Proof of Fix (PoF) *

Before:

poc-before

After:

poc-after

👍 User Acceptance Testing (UAT)

poc-test

After the fix, functionality is unaffected.

d3v53c and others added 3 commits November 29, 2020 04:38
Cross-Site Scripting (XSS) : issue fix by sanitizing strings before rendering
Copy link
Contributor

@mercurio mercurio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get this to replicate, perhaps there have been changes to the core library since this was posted that resolve this issue.

This also doesn't seem to be the right approach, I would think you'd want to sanitize user input, not what the programmer has entered as the configuration for a chart.

@JamieSlome
Copy link

@d3v53c - any thoughts on the above comment?

@d3v53c
Copy link

d3v53c commented Dec 7, 2021

I couldn't get this to replicate, perhaps there have been changes to the core library since this was posted that resolve this issue.

Let me take a closer look, probably the core library fixed it already.

This also doesn't seem to be the right approach, I would think you'd want to sanitize user input, not what the programmer has entered as the configuration for a chart.

User input should be sanitized, I concur, there's no argument in that. Also, insecure handling of such inputs should be sanitized too. If the issue is still persisting, I would argue that this is such a scenario where the core library is accepting an input (either developer config or user input, for all I know somebody could accept a user input and then pass it as a configuration), leading to a potential security breach. Anyway, please let me take a look on this issue and update here.

@JamieSlome
Copy link

@d3v53c - appreciate your contribution here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants