XSS vulnerabilities in from, to and range parameters.

[rsdoiel]

There is an XSS vunerablity in the "from", "to" and "range" GET parameters for URLs accessing IRStats2 1.0.1 as well as IRStats2 1.1
under EPrints 3.3.

Example paths

    /cgi/stats/report/requests?from=&range=</script><svg/onLoad=prompt(0)>
    /cgi/stats/report/requests?from=&to=</script><svg/onLoad=prompt(0)>
    /cgi/stats/report/requests?from=&range=</script><svg/onLoad=prompt(0)>
    /cgi/stats/report/requests?from=</script><svg/onLoad=prompt(0)>
    /cgi/stats/report/requests?from=</script><svg/onLoad="console.log('Bad news')">

Before processing "from", "to", "range" the values need to be sanitized (e.g. the < and > stripped at a minimum). Not sure where this should happen in the Perl code.

Is IRStats2 being maintained (last update appears to be 2017 and two pull requests are unanswered)?

Thanks,

Robert

[jesusbagpuss]

Hi @rsdoiel ,
The method EPrints::Plugin::Stats::Context::to_json is used in EPrints::Plugin::Stats::View::Table to embed values into a call to some javascript, which looks like where the issue is.

The real fix would be to validate the values on their way in, in EPrints::Plugin::Stats::Context::parse_context.

At a glance, the list of @FIELDS at the top of the Context module could all be validatable by looking things up against the config, or simple RegExps (e.g. for dates).

Does that help initially?

I can't comment on the maintenance of this module.
There is a fork of IRStats in the eprintug area (https://github.com/eprintsug/irstats2/) that looks to be ahead of this one, but I'm not sure which, if either is the most maintained.

PS There's also this comment: https://github.com/eprints/irstats2/blob/master/lib/plugins/EPrints/Plugin/Stats/Context.pm#L365 ..!?

[mpbraendle]

Regarding maintenance, we had implemented locally implemented many GUI changes especially for better usability, responsive GUI and improving accessibility, as well as new reports for citation, journal/publisher and Open Access statistics.
See https://www.zora.uzh.ch/cgi/stats/report for a live demo.
Some of the code was contributed back here:
https://github.com/eprintsug/OpenAccessType
https://github.com/eprintsug/IRStats2-Citation

[jesusbagpuss]

More investigation:
For 'range', it can be \d+[dmy], or a year \d{4}.
In EPrints::Plugin::Stats::Utils there are some validation routines, but these apparently aren't always invoked, or don't strip 'bad' values.

[jesusbagpuss]

(just some notes for reference):
Params read in from request:
https://github.com/eprints/irstats2/blob/master/lib/plugins/EPrints/Plugin/Stats/Context.pm#L96

• could sanitise them all here (html entities-wise at least).

Fields referenced during parse_context:

• irs2report: ???
• set_name: use part of this logic: EPrints::Plugin::Stats::Context::has_valid_set
• set_value: as above
• from: \d{4} or \d{6} or \d{8}
• to: \d{4} or \d{6} or \d{8}
• range: year or \d+[dmy]
• datatype: what EPrints::Plugin::Stats::Processor have as their 'provides' param. Could use something like EPrints::Plugin::Stats::Handler->{processors_map}->{"$datatype"}
• datafilter: values set in config, things like archive full_text open_access (or undef). Possibly not used as a request param
• grouping: I think EPrints::Plugin::Stats::Sets::set_exists may be able to validate this value (which is also called by EPrints::Plugin::Stats::Context::has_valid_set).
• cache: is this used?

NB ran out of time reviewing these - anyone else feel free to chip in, or I'll continue when I get a spare moment!

[rsdoiel]

I'll figure out a simple patch, make a diff and attach it here (hopefully next week).

My current plan it to strip '<' and '>' in the linked loop example for processing the session variables. That'll solve the injection risk. Thank you everyone for the help.

All the best,

Robert

[mpbraendle]

In many cases we use https://metacpan.org/pod/CGI::IDS for securing CGI scripts . This library covers many more attack vectors than the one with '<' '>' and is worth having a look at it.

[rsdoiel]

Will take a look, thanks Robert

[jesusbagpuss]

@rsdoiel - just wondering if you got anywhere with this? I could take a look today at creating some validation methods.

[rsdoiel]

Good Morning John, No haven't had a chance to get back to this yet (a few other non-EPrints issues are my priority at the moment). Hopefully this week or next. It's a low priority for me at the moment but is something I will need to address as the security ticket open where I work will need to be resolved either by fixing it or disable IRStats2. Thanks, Robert

…

On Tue, Feb 4, 2020 at 4:43 AM John Salter ***@***.***> wrote: @rsdoiel <https://github.com/rsdoiel> - just wondering if you got anywhere with this? I could take a look today at creating some validation methods. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#95?email_source=notifications&email_token=AABJIPNUGI2FL355P3RR6KDRBFPIBA5CNFSM4KK3RA22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKXPXWY#issuecomment-581893083>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABJIPP7YCIUTYXRDWXEUQLRBFPIBANCNFSM4KK3RA2Q> .

[jenswitzel]

Did anyone fix this? CGI::IDS does not "ring the bell"!

[rsdoiel]

I don't think this has been solved, I know I haven't had a chance to pursue the suggested fixes. EPrints for us is in maintenance only mode. Caltech Library is not doing active development inside EPrints or with the EPrints plugin system. Our mid-range plans is to have a stats system that works across our mix of repository systems so ultimately I think IRStats will be replaced at Caltech Library. All the best, Robert

[jenswitzel]

Thanks. I fixed it with CGI::IDS, parsing the filter parameters.

[jenswitzel]

Here is a solution: https://github.com/eprintsug/IDS

[liamgh]

Did a bit of PHP inspired regex to solve this issue and came up with this:
In Context.pm at about line 92:

	# Then check URI parameters, priority over the rest
	foreach( $session->param() )
	{
		next if( !$FIELDSMAP{$_} ); 
		my $value = $session->param( "$_" );
		$value =~ s/[<>\/\\;=\&\?\%\'[:cntrl:]]//gm;
		$self->{$_} = $value;
	}

Not perfect but seems to filter out trouble makers.

[graingert]

EPrints::Plugin::Stats::Context::to_json should probably make HTML safe json by default:

json-ld/json-ld.org#390
https://github.com/ierror/django-js-reverse/blob/master/django_js_reverse/core.py#L124-L131
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-15486

[graingert]

@liamgh ^

[jesusbagpuss]

Possible fixes added to https://github.com/eprintsug/irstats2/tree/issue-17-xss