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

Graph: Fix XSS vulnerability with series overrides #25401

Merged
merged 2 commits into from Jun 8, 2020

Conversation

dprokop
Copy link
Member

@dprokop dprokop commented Jun 5, 2020

This fixes possible XSS vulnerability when specifying series alias (i.e. test data or elastic search).

The problem is caused by the bs-typeahead directive which evals the select options passed to it. We are using an old version of ng-strap which allows only an array of strings to be passed as available options. The alias in Elastic and TestData query editor allows providing special characters, so, for instance, specifying alias as <img src onerror="alert(document.cookie)"> creates a possible XSS attack vector.

The solution I'm proposing here is:

  1. Adding validation to the alias fields in the corresponding query editors. This will prevent users from inputting insecure values. This will fix core datasources.
  2. Escaping aliases when rendering series overrides typeahead. This may break overrides that have <>&" special characters in it. But only in a situation, when someone actually selects such override as then the override will contain escaped string, which will not match any series when Graph overrides are applied. This will fix and possibly break external query editors of external data sources that allow alias confg.

I'm afraid of bumping the ng-strap as we have version that is modified.

Copy link
Contributor

@peterholmberg peterholmberg left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@hugohaggmark hugohaggmark left a comment

Choose a reason for hiding this comment

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

LGTM!

@dprokop dprokop merged commit c53435f into master Jun 8, 2020
1 check passed
Frontend Platform Backlog automation moved this from In Review to Done Jun 8, 2020
@dprokop dprokop deleted the fix-series-overrides-alias-xss branch June 8, 2020 15:13
aknuds1 pushed a commit that referenced this pull request Jun 30, 2020
* Fix XSS vulnerability with Graph series overrides

* Update public/app/plugins/datasource/testdata/partials/query.editor.html
@eriktews
Copy link

eriktews commented Apr 6, 2021

Unfortunately the bug introduced with this XSS fix is still present. For example when Grafana is used with OpenHAB and InfluxDB, then some labels may contain quotes ("") and when someone tries to add a time series overwrite and selects a value based on the typeahead suggestions, the overwrite has the wrong name and won't work correctly.

To reproduce, just add a dummy data source with a series named foo123<>" and then try to add a time series overwrite for a dashboard plotting this data.

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

Successfully merging this pull request may close these issues.

None yet

4 participants