Prevent xss

[StephenDsouza90]

This PR is in reference to the issue #2520 for preventing an Cross-site scripting (XSS) attack when saving the dashboard.

It uses the Django's django.utils.html.strip_tags() function to sanitize html input elements, in this case, the name of the dashboard, to prevent the XSS attack.

Currently my approach is to apply the sanitization on the fields that are inputted from the html template and rendered on the graphite web page compared to doing it via a middleware that would inspect HttpRequest objects - this IMO would mostly likely be a little inefficient. Sanitizing on the fields that require it is more faster.

The Dashboard save functionality has a possible security issue allowing for an XSS attack, this PR fixes it. In the future, the sanitize utility function can be used for other input fields that require it.

[codecov-commenter]

Codecov Report

Merging #2620 into master will decrease coverage by 0.00%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #2620      +/-   ##
==========================================
- Coverage   80.21%   80.20%   -0.01%     
==========================================
  Files          87       87              
  Lines        9335     9342       +7     
  Branches     1988     1989       +1     
==========================================
+ Hits         7488     7493       +5     
- Misses       1564     1565       +1     
- Partials      283      284       +1     

Impacted Files	Coverage Δ
webapp/graphite/dashboard/views.py	98.54% <66.66%> (-0.72%)	⬇️
webapp/graphite/util.py	89.83% <100.00%> (+0.12%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ed7dd7...53dbd30. Read the comment docs.

[deniszh]

Cool! Thanks a lot, @StephenDsouza90 , for fixing this.
Ignore DeepSource, looks like it went crazy again, analyzing whole file and not your changes.

[DanCech]

The problem isn't in the dashboard save per-se, it is caused by the dashboard name being displayed in an unsafe manner. Stripping potential "bad" sequences from the dashboard name isn't a bad idea, but to properly fix the XSS we'll also need to make sure that we html-escape the name everywhere it's displayed.

[StephenDsouza90]

The problem isn't in the dashboard save per-se, it is caused by the dashboard name being displayed in an unsafe manner. Stripping potential "bad" sequences from the dashboard name isn't a bad idea, but to properly fix the XSS we'll also need to make sure that we html-escape the name everywhere it's displayed.

Hi @DanCech thanks for the quick feedback!

I'm still a bit unclear on what more needs fixing, if you can point me in the right direction and guide me.

I understand the problem isn't in the dashboard save functionality, but rather the "name" that's being rendered that allows for an XSS attack, but this is exactly what i've tried to tackle - I strip out potential "bad" sequences in the name and in the dashboard's state before it gets saved to the database. This should render the name everywhere correctly since the name will be fetched from the database which is the source of truth and it's saved in a safe manner.

For an attacker trying to inject XSS into the name will be the only person who will be affected by XSS for that current state and session, unless he/she refreshes the page - for other end-users/clients, it shouldn't affect them as what gets saved into the database will be "safe" and that's what will be rendered to them.

So still wondering if this solution is good enough? Or maybe if you could give me a scenario where this attack can still be exploited, I could try to work around that.

[StephenDsouza90]

Hi,

I'm still awaiting feedback and next steps on how to move forward with this PR based on my last comment. Would be great if one of you could point me in the right direction.

[DanCech]

Basically whenever we display the name it should be html-encoded so that any "special" characters are encoded as html entities rather than interpreted as html.

The docs for the strip_tags function explicitly state that it does not produce strings that are html-safe https://docs.djangoproject.com/en/3.0/ref/utils/#django.utils.html.strip_tags, and that html entities should be escaped when displaying the string.

[StephenDsouza90]

Hi @DanCech

Thanks again for the feedback, you're right, my bad!

I just checked there are two ways to solve this. We can go with Mozilla Bleach that does it in a clean way:

>>> import bleach

>>> bleach.clean('an <script>evil()</script> example')
u'an &lt;script&gt;evil()&lt;/script&gt; example'

Or we can go with Django's escape() function:

Returns the given text with ampersands, quotes and angle brackets encoded for use in HTML. The input is first coerced to a string and the output has mark_safe() applied.

>>> from django.utils.html import escape

>>> escape('an <script>evil()</script> example')
'an &lt;script&gt;evil()&lt;/script&gt; example'

With bleach, the only disadvantage is that we'll be adding yet another library, which might not be a good thing.

I'll go ahead with the escape() for now unless there is any objection.

[DanCech]

The escaping needs to be done when we're actually inserting the name into the html on the client side, so you'd want to use the htmlEncode function in dashboard.js to do that for dynamic content and a python library when we're building html server-side.

[ploxiln]

a possibly helpful example of escaping in the wrong place vs escaping in the right place: #2587

[StephenDsouza90]

Thanks @ploxiln and @DanCech for pointing me in the right direction! Appreciate it. I'll be working on this later today. More commit fixes coming soon in the PR!

[StephenDsouza90]

The escaping needs to be done when we're actually inserting the name into the html on the client side, so you'd want to use the htmlEncode function in dashboard.js to do that for dynamic content and a python library when we're building html server-side.

If I understand @DanCech correctly, would the following be a good enough solution? i.e. to htmlEncode the dashboard name at the time of input in dashboard.js just before it gets saved in the backend - So what gets saved is what gets rendered? Something like this:

function saveDashboard() {
       <...>
       <...>
       if (button == 'ok') {
         // html-encode dashboard name from input
         text = htmlEncode(text);

         // hmtl-encode dashboard name
         setDashboardName(text);

         // save html-encoded dashboard name
         sendSaveRequest(text);
       }
       <...>
       <...>
}

graphite-web/webapp/content/js/dashboard.js

Lines 2723 to 2737 in 0ed7dd7

	function saveDashboard() {
	Ext.Msg.prompt(
	'Save Dashboard',
	'Enter the name to save this dashboard as',
	function (button, text) {
	if (button == 'ok') {
	setDashboardName(text);
	sendSaveRequest(text);
	}
	},
	this,
	false,
	(dashboardName) ? dashboardName : ''
	);
	}

[DanCech]

You don't want to store the html-encoded dashboard name in the database, because then when you load it and html-encode it again you'll mangle it. eg if you have a dashboard name like Testing > TestDash it'll html-encode to Testing > TestDash and that will be rendered correctly. If you store the encoded string in the database then next time you load that dashboard it'll be html-encoded as Testing > TestDash (because & html-encodes to & and will literally display in the browser as Testing > TestDash. You want to store the actual name in the database and just html-encode it at the point it's inserted into an html document (either on the server side or in the client).

[StephenDsouza90]

Thanks @DanCech and @ploxiln for pointing me in the right direction. Really appreciate it and thanks for answering my questions to get a better understanding of what solution is required.

I added a commit for the fix, let me know if it's inline to the solution you had in mind.