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

XSS fixes #17

Open
jesusbagpuss opened this issue Jan 11, 2021 · 6 comments
Open

XSS fixes #17

jesusbagpuss opened this issue Jan 11, 2021 · 6 comments

Comments

@jesusbagpuss
Copy link

@goetzk - with your release, thought it worth highlighting this:
eprints#95

I'll add some code today for review

@jesusbagpuss
Copy link
Author

jesusbagpuss commented Jan 11, 2021

See experimental fixes on https://github.com/eprintsug/irstats2/tree/issue-17-xss

Notes:

  • some parts of IRStats (cgi scripts) seem to allow for date formats such as yyyy/mm/dd. The parameter validation methods added to the branch above don't allow this.
  • I think the method EPrints::Plugin::Stats::Utils::range_to_offset is un-used, and possible should be removed?

@jesusbagpuss
Copy link
Author

jesusbagpuss commented Jan 11, 2021

There may be other changes/improvements in how the cgi scripts read params - possibly replacing methods with a call to the Context module.

@jesusbagpuss
Copy link
Author

NB I've also spotted that the base_url param in cgi/stats/set_finder (called from javascript) needs work.

@cziaarm
Copy link

cziaarm commented Jan 25, 2021

'xcuse my lack of ability in the dark-arts, but is dealing with the paramaters at the point they are written into the script tag

https://github.com/eprints/irstats2/blob/master/lib/plugins/EPrints/Plugin/Stats/Context.pm#L320

an overly simple fix?

It doesn't stop all sorts of cruft getting injected into various bits of html, but might keep it from getting executed?

@cziaarm
Copy link

cziaarm commented Jan 25, 2021

...Unless someone manages to escape from an attribute and shoehorn an entire script tag in there... I think the more fulsome approach is probably the wisest on reflection. As you were...

@jesusbagpuss
Copy link
Author

jesusbagpuss commented Apr 29, 2021

Some params get passed via the javascript ajax params/context.

Below is hopefully a definitive list. The 'context' params get sanitised in the proposed code in https://github.com/eprintsug/irstats2/blob/issue-17-xss/lib/plugins/EPrints/Plugin/Stats/Context.pm.

The validation methods could be lifted into the EPrints::Plugin::Stats::Util module to make calling them from the cgi scripts more sensible.

  • 'context' params:
    • datatype
    • datafilter
    • grouping
    • set_name
    • set_value
    • range
    • to
    • from
  • limit
  • q (searching for sets)
  • base_url
  • view
  • top
  • title_phrase
  • referer (https://github.com/eprintsug/irstats2/edit/issue-17-xss/cgi/stats/browse - although that may be a redundant file?)
  • container_id (could try to normalise these - to all start with 'irstats2_'?):
    • spark_[eprintid]
    • irstats2_summary_page_downloads
    • irstats2_summary_page_countries
    • stats_browse_test !?
    • irstats2_downloads
    • irstats2_top5
    • irstats2_container_[xxxxxx]
    • downloads
    • hits
    • deposits
    • total_fulltext
    • total_openaccess
    • lm_downloads (L M == last month)
    • lm_deposits
    • ratio_fulltext
    • ratio_openaccess

THis: https://github.com/eprintsug/irstats2/blob/issue-17-xss/cgi/stats/get#L78-L83 will need to understand expected params that are outside the context params.

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

No branches or pull requests

3 participants