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

fix composer saved graphs target escaping #2587

Merged
merged 2 commits into from
May 16, 2020

Conversation

ploxiln
Copy link
Contributor

@ploxiln ploxiln commented Apr 18, 2020

saved graphs targets were html-escaped in the json response
to fix an XSS vulnerability in #1662

... but that was not really the right place to escape the graph targets,
it broke targets using quotes: #1801 #2334

so effectively revert the original fix, and instead html-escape the
targets just before rendering them in the GraphDataWindow Ext.ListView


TODO:

  • ensure works with python-3 (str(thing) is never the right way :)
  • check what the "dashboard" interface does (it doesn't have this problem, does it allow html injection though?)

@lgtm-com
Copy link

lgtm-com bot commented Apr 20, 2020

This pull request introduces 1 alert and fixes 8 when merging 4c9a083 into 910bdea - view on LGTM.com

new alerts:

  • 1 for Syntax error

fixed alerts:

  • 4 for Unused variable, import, function or class
  • 2 for Comparison between inconvertible types
  • 1 for Superfluous trailing arguments
  • 1 for Client-side URL redirect

saved graphs targets were html-escaped in the json response
to fix an XSS vulnerability in graphite-project#1662

... but that was not really the right place to escape the graph targets,
it broke targets using quotes: graphite-project#1801 graphite-project#2334

so effectively revert the original fix, and instead html-escape the
targets just before rendering them in the GraphDataWindow Ext.ListView

also skip the `str()` around `graph.url`, it's already a string,
in both python2 and python3
same XSS vulnerability as the composer saved user graphs data view had
@ploxiln
Copy link
Contributor Author

ploxiln commented Apr 20, 2020

This is done and tested with both python-2.7 and python-3.7

I tested by pasting <svg/onload=alert()> (thanks #1662) at the end of graph target expressions in these graph data lists:

Screen Shot 2020-04-20 at 6 06 13 PM

Screen Shot 2020-04-20 at 6 05 31 PM

(and I also tested target expressions with double quotes like summarize(..., "sum"))

Before this branch, pasting the svg-alert expression in either place and hitting enter to finish the edit would immediately cause the popup. In the composer, saving and loading the graph and viewing the data targets would no longer cause the popup, but would mess-up the legit double-quotes in the expression, breaking the graph. In the dashboard, saving and loading the dashboard and viewing the data targets would cause the XSS popup (but expressions with double-quotes worked correctly).

After this branch, pasting the svg-alert expression and hitting enter to finish the edit no longer causes popups, saving/loading/viewing does not cause popups in either place, and graphs with quotes work correctly.

Figuring out the ancient extjs parts were admittedly tricky.

@piotr1212 piotr1212 requested a review from cbowman0 April 25, 2020 09:11
@ploxiln
Copy link
Contributor Author

ploxiln commented May 6, 2020

please take a look @deniszh

@deniszh
Copy link
Member

deniszh commented May 6, 2020

Wow, @ploxiln , missed that somehow, sorry! Look good, awesome job!

@deniszh
Copy link
Member

deniszh commented May 6, 2020

If @cbowman0 or others have no objections I'll merge it soon

Copy link
Member

@DanCech DanCech left a comment

Choose a reason for hiding this comment

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

Looks good

@ploxiln
Copy link
Contributor Author

ploxiln commented May 6, 2020

Thanks :)

@piotr1212 piotr1212 merged commit dcb9c36 into graphite-project:master May 16, 2020
@ploxiln ploxiln mentioned this pull request Jun 18, 2020
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.

None yet

4 participants