OPAC is vulnerable to Cross-site scripting attacks.

[Václav Jansa]

We have received email from ABUSE team, for vulnerability of our OPAC

I tried it on PINES catalog with same output

https://gapines.org/eg/opac/browse?qtype=title&blimit=10%22%3E%3Cscript%3Ealert(150)%3C/script%3E

Our EG version is 3.1.4

Od: "UT Information Security Office" <email address hidden>
Komu: <email address hidden>
Předmět: [cuni #8528] [ISOTicket:2181455] UT/ISO -- Verified Vulnerable
Web Page [195.113.63.102 - CUNI.CZ]
Datum: 01/04/2019 07:37:11

=========================================================
The following alert is the product of the Dorkbot service
created by UT Austin: https://security.utexas.edu/dorkbot
=========================================================

The Information Security Office at the University of Texas at Austin
has found the following web page to be vulnerable to a high-risk application
attack:

HOST: 195.113.63.102 [mojzis.jabok.cuni.cz]
DATE: 2019-03-31 22:17:09 CST/CDT

GET:
https://www.jabok.cuni.cz/eg/opac/browse?qtype=title&blimit=10%22%3E%3Cscri[..]
<https://www.jabok.cuni.cz/eg/opac/browse?qtype=title&blimit=10%22%3E%3Cscript%3Ealert(150)%3C/script%3E&bterm=1&search-submit-go=Proch%C3%A1zet&locg=1>

ATTACK DETAILS:
This page is vulnerable to Cross-site scripting attacks.

Cross-site scripting attacks, in general, are an issue because
they are enabling attacks. Specially-crafted malicious URLs can
steal authentication tokens/cookies when a logged-in user visits them,
giving the attacker full access to that user's account in the application.
Reflected XSS attacks, in particular, are a concern as they can be used to
socially engineer a user into clicking on what appears to be a
legitimate URL.

** Please note that the Dorkbot service will re-check this page in the next
30-days to help verify remediation for you. **

Please also consider the following:

- Web application security testing should be performed regularly,
   especially for any public web applications. This includes
   tracking application inventory, general code review and vulnerability
   assessments using web application security testing tools.

- All input received by the web server should be checked before
   it is processed. The best method is to remove all unwanted input and
   accept only expected input. For example, ensure angle brackets are
   not allowed in any input to any Web page fields. Additionally, no
   syntactic input should be allowed. Syntactic input can come from
   databases, other servers, etc. All input into a Web application must
   be filtered to ensure the delivery of clean content to individuals using
   your service.

- Other References:

   OWASP Guide to Building Secure Web Applications and Web Services
https://www.owasp.org/index.php/Category:OWASP_Guide_Project

   UT-Austin: Minimum Security Standards for Application Development and
Administration
https://security.utexas.edu/policies/standards_application

Please let us know if you believe any of this information to be inaccurate
so that we can be of better service in the future.

We hope this information is helpful.

Information Security Office
The University of Texas at Austin
<email address hidden> | 512.475.9242
http://security.utexas.edu
=======================================
https://www.facebook.com/utaustiniso
https://twitter.com/UT_ISO
=======================================

[Jeff Davis (jdavis-sitka)]

Confirmed on 3.1.7, and it looks like the user input is not sanitized in master. Fix forthcoming.

[Jeff Davis (jdavis-sitka)]

Fix pushed to branch user/jeffdavis/lp1822630-browse-xss in the security repo. Looks like a simple case of a CGI param being embedded in HTML without being sanitized with "param | html".

[Dan Wells (dbw2)]

I was about to roll over the 3.3.0 RC to full release when I noticed this bug report earlier today. A quick scan of the code seems to show this isn't our only instance of this issue. For example, here is another:

https://ulysses.calvin.edu/eg/opac/results?qtype=keyword&query=test&detail_record_view=1%22%3E%3Cscript%3Ealert(150)%3C/script%3E&fi%3Atype=&fi%3Azform=&locg=1

I ran the following as a quick and dirty search for such cases:

ack "value=.\[%[^|(]+%\]"

This finds quite a few more contenders. Most are of the type "classxyz.id" (i.e. from a FM object), and shouldn't present an issue, since they are naturally sanitized. There are at least a half-dozen other cases which look pretty likely to cause problems, and another dozen or two which deserve close scrutiny. My simple regex is also not exhaustive. (In particular, it excludes any interpolation with an open parens, to exclude translation false positives. This exclusion could be done more elegantly.)

So, I guess I have two questions. 1) How much time do we want to spend combing over the code to correct as many cases as we can all at once? 2) Do we delay 3.3.0 until this bug is solved?

My answers are:
1) As much time as we need to feel confident, which could be measured in weeks. Many of these cases are very old (e.g. six years old for the original case), so I think we do more harm than good to rush to judgment at this late stage.

2) Because of my answer to #1, I am inclined to *not* delay the 3.3.0 release for this bug, but proceed as normal, then do a 3.3.1 security release with all the other security releases (which might end up close to the next point release anyway).

I would appreciate any further opinions, but recognizing that it is an impossible situation to wait for input on how long to wait, I might go with my gut at any point :)

[Jeff Davis (jdavis-sitka)]

I've been combing through the OPAC templates for additional instances of raw CGI.param values being embedded in HTML as-is, either directly or after being assigned to a variable. (This approach is much narrower than Dan's regex.) There are a lot of cases where the raw value is tested but not output to HTML, and some other cases where it is passed to a function like mkurl that takes care of the sanitizing. I have pushed an additional commit to the above branch fixing the few other cases I found that actually require cleanup. I definitely can't promise I caught everything, but hopefully it covers the low-hanging fruit.

(There's an interesting case in parts/result/facets.tt2 -- two CGI.param values are assigned to variables near the top and then those variables are used in complicated ways. I think no sanitizing is necessary here, so my commits don't touch it, but it would be good to have at least one additional pair of eyes on that one.)

I vote for fixing what we can fix quickly now, and doing a more thorough audit over time.

[Jason Stephenson (jstephenson)]

I vote for not letting this hold up the release. If we let open security bugs hold up releases, then we never would release.

I also propose that we organize some kind of security hackfest where we (i.e. the core developers) take time to address the known security issues. If we don't do this soon, then we never will.

[Linda Jansova (skolkova-s)]

Jeff, Dan and Jason, have you already have a chance to have a look at a possible fix (or fixes)?

Is there a chance for the upcoming 3.3.2 (and 3.2.7 and 3.1.13 as the other stable releases) to become security fix releases?

Thank you for any updates to this issue!

[Jeff Davis (jdavis-sitka)]

We've had a potential fix for this bug for several months; it needs to be reviewed, signed off, and committed (fortunately it's a clean backport). I've targeted the bug for the next round of point releases, so hopefully it will get the attention it needs now.

Note that we have another pending XSS-related security fix (bug 1559239) which has been signed off and just needs to be committed.

[Linda Jansova (skolkova-s)]

Thank you, Jeff! As I am not in the security team, I cannot see the other activities going on (such as the other bug you have pointed to), so I appreciate very much your update!

[Chris Sharp (chrissharp123)]

I can confirm Jeff's branch working great to avoid the issue. Signoff branch available to RMs for the next point releases is available in the security repo:

user/csharp/lp1822630-browse-xss

[Jason Stephenson (jstephenson)]

I am going to have a look at this one on a couple of virtual machines. First, I want to reproduce it, and second, I want to test the fix.

[Jason Stephenson (jstephenson)]

I have added my signoff in the user/dyrcona/lp1822630-browse-xss branch in the security repository.

The fix works for me. Incidentally, recent versions of Chrome seem to prevent the specific URL in the description from "working," i.e. I did not get the alert to pop up. It did, however, happen in Firefox quite reliably.

[Jason Stephenson (jstephenson)]

I know that I've added my signoff for this, but some additional testing indicates that 3.2, and probably 3.1, may need a different version of the place_hold_result.tt2 patch. We're getting an internal server error with this patch applied after placing a hold. I"m not sure, yet, how much of this is due to code drift and how much could be a local customizaiton. I do know that backing out the change to place_hold_result.tt2 resolves it, and we have not customized that template.

I should also rebuild one of my virtual machines to more thoroughly test this with master.

[Jason Stephenson (jstephenson)]

Well, I now feel bad for not testing this branch thoroughly enough. I get the internal server error on master, too, when placing a hold in the OPAC. I'm going to look at all of the affected interfaces and see if I can come up with fixes for the holds and any others that may be broken by this patch.

[Jeff Davis (jdavis-sitka)]

I have a fix for that issue but it didn't make it into the branch, sorry about that. I've pushed the fix to security branch user/jeffdavis/lp1822630-browse-xss. I haven't run into any other issues.

[Jason Stephenson (jstephenson)]

I have tested Jeff Davis' latest commit and it resolves the issue for me. I independently made the same change before seeing that he had updated his branch.

I've added Jeff's commit with my signoff to my security branch: user/dyrcona/lp1822630-browse-xss.

I think this is ready to go. I haven't had any trouble with the other interfaces touched by the branch.

Thanks, Jeff!

[Linda Jansova (skolkova-s)]

It seems that the solution is on its way to 3.3.3 (and the other two releases of previous versions) - thank you very much for that!

[Jeff Davis (jdavis-sitka)]

It looks like this signed-off security fix has failed to make it into yet another release. Can we get it committed, please?

[Jason Stephenson (jstephenson)]

Jeff,

Security releases require a level of coordination that is not always possible with the monthly patch releases. We want the code committed all at the same time with the releases tagged and a security release announcement made. This is done in an effort not to blind side our users and to help protect those sites that cannot upgrade immediately.

It currently being summer in the northern hemisphere makes the situation more difficult with people on vacations and others buried in work. There were 4 people working over the past two days on the releases announced today. If you would like to participate in those efforts to help see that patches like this make it into releases, I'm sure your efforts would be most appreciated.

Jason

[Václav Jansa (vaclav-jansa)]

Hello team,
what is current status of this security issue. Will be this security patch implemented to 3.3.4 or 3.4.0?
We are under pressure from our CSIRT team (our main installation is hosted on academic network).

Jeff, can you please share patches directly to me? I am not member of security team. But I will try to patch our production manually, unless patch will be merged to main source tree.

Thanks for patching.

Best regards

Vaclav Jansa

[Jason Stephenson (jstephenson)]

See my previous comment. If more people would help with monthly releases, then security fixes would be released in a more timely manner.

You can always patch your systems with any branches at any time. I do. I recommend using git for that, so that you can maintain your local branch with patches and keep it up to date with mainline.

[Linda Jansova (skolkova-s)]

Jason, but it seems we do not have access to the security repository. It is probably available solely to Evergreen security team... That's why we are asking :-).

[Jason Stephenson (jstephenson)]

Linda, you are correct and I forgot about that. I will personally make an effort to see that this and one other security fix are added to the releases scheduled for next Wednesday (2019-09-18).

Unfortunately, the releases almost always fall on days where I have meetings, and next Wednesday is no exception, so that makes it harder for me to participate.

[Linda Jansova (skolkova-s)]

Thank you very much, Jason! We do appreciate your commitment to the issue :-)!