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

Fixed XSS #67

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

Fixed XSS #67

wants to merge 2 commits into from

Conversation

Aravindha1234u
Copy link

📊 Metadata *

Bounty URL: https://huntr.dev/bounties/1-npm-dhtmlx-gantt

⚙️ Description *

dhtmlx-gantt javascript library renders json data into charts. Which does not care about data encoding before rendering into charts. I have escaped the HTML tags and other using javascript prefined function.

💻 Technical Description *

I have escaped all text will rendering the data

🐛 Proof of Concept (PoC) *

Payload : <img src=x onerror=alert(document.location)>

add this to task information.

🔥 Proof of Fix (PoF) *

image

👍 User Acceptance Testing (UAT)

image

@Aravindha1234u
Copy link
Author

@AlexKlimenkov Can you take a look at this fix.

@AlexKlimenkov
Copy link
Contributor

Hi @Aravindha1234u !

Thank you for your suggestion!

This is a design solution and is not considered XSS issue.

dhtmlxGantt is used as a part of a client-server web application, where gantt is one of the components used on the client-side.

The fact that you can enter a value that will execute javascript on the page opened by you is not the issue by itself. There is no way to prevent you from executing any arbitrary code on the page opened by you since you can always open the browser console and run code there.
It becomes an XSS vulnerability only when you are able to execute that code on pages opened by other users. This is not possible until the backend is introduced.

Our current position is that it's the responsibility of the backend server to sanitize/clean the data either before saving it to the DB or before passing it to the client-side where it's consumed by gantt.

If the backend is implemented safely such an XSS attack is not possible.
We address it in our documentation:
https://docs.dhtmlx.com/gantt/desktop__app_security.html
https://docs.dhtmlx.com/gantt/desktop__howtostart_dotnet_core.html#applicationsecurity
The current behavior is something that the developer should keep in mind, but not an issue by itself.

As for why dhtmlxGantt doesn't clean the data by default, it's due to how it's used.
This is one of the typical usage scenarios - our users, developers - should be able to place all kinds of images, inputs, or other html blocks into various elements of gantt. By the design, templates of dhtmlxGantt should allow placing custom HTML in the markup.

If you want to add an extra layer of protection, you can redefine templates to strip html before output and/or escape data before saving https://docs.dhtmlx.com/gantt/desktop__app_security.html#xssattacks
But the first step should be to sanitize the data on the backend since it will prevent XSS attacks globally for the whole application, not only via dhtmlxGantt.

@Aravindha1234u
Copy link
Author

Aravindha1234u commented Apr 29, 2021

Hi @AlexKlimenkov, I got it. I just fixed for disclosure which was opened by another researcher in huntr.dev, so that might able for claim for bounty, if you merge my PR

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.

2 participants