MARC Editor XSS vulnerability

Bug #1902965 reported by James Fournie on 2020-11-05
260
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Evergreen
Critical
Unassigned
3.4
Critical
Unassigned
3.5
Critical
Unassigned
3.6
Critical
Unassigned

Bug Description

MARC 856 fields are vulnerable to cross-site scripting.

=856 40$u"><script>alert(document.cookie');</script>

Will execute the javascript. This would make it possible for me to steal an auth token if someone is logged into the OPAC or staff client.

James Fournie (jfournie) wrote :

This also affects the $y and maybe other MARC fields that render to HTML

information type: Public → Private Security
Changed in evergreen:
status: New → Confirmed
importance: Undecided → Critical
Changed in evergreen:
assignee: nobody → Jeff Davis (jdavis-sitka)
Jeff Davis (jdavis-sitka) wrote :

Branch user/jeffdavis/lp1902965-marc-856-filter in the security repo fixes the issue by applying the TT2 "html" filter to the URI href, link, and note before using them. I'm not sure if escaping HTML entities will cause problems for certain 856 URLs.

I haven't checked whether other MARC fields have the same vulnerability, but it seems likely.

Changed in evergreen:
assignee: Jeff Davis (jdavis-sitka) → nobody
tags: added: pull
tags: added: pullrequest
removed: pull
James Fournie (jfournie) wrote :

Unfortunately it seems the demo.evergreencatalog.com I was testing on upgraded to the new OPAC since I reported this so the playing field has changed -- but on there, 245 and 100 fields are vulnerable there. Will try to test some more things

James Fournie (jfournie) wrote :

Here's some more fields I found that seem vulnerable:
010$a
245$abp

Also supercat HTML output is vulnerable (/opac/extras/supercat/retrieve/html/record/<id>)
260$c
260$b

Here's what I'm using to generate Evil MARC Records:
https://gist.github.com/jamesrf/1179f75ec51da878d0577a21575bffa5

Jeff Davis (jdavis-sitka) wrote :

In my testing, 245$abp are also vulnerable in search results (but not if Show More Details is enabled).

The following fields are vulnerable in record summary, in addition to 010$a and 245$abp:
250$a
260$abc
740$a

Changed in evergreen:
assignee: nobody → Jeff Davis (jdavis-sitka)
Jeff Davis (jdavis-sitka) wrote :

New security branch user/jeffdavis/lp1902965-filter-marc-fields has the following fixes:

- Filter a long list of MARC fields in OPAC search results and record display.
- Apply the same filters in the Bootstrap OPAC.
- Change format of rights element in MARC21slim2ATOM.xsl to avoid XSS in SuperCat record display.

Changed in evergreen:
assignee: Jeff Davis (jdavis-sitka) → nobody
Jason Boyer (jboyer) wrote :

Hi, I have a bootstrap fix for Jeff's branch at security/user/jboyer/lp1902965_marc_xss ... but I've been stuck trying to figure out how to correct the instance I've noticed in the 245a in the 3.6+ Angular staff catalog.

To see what I mean, edit a record to add <i>Yo, Italics!</i> to a 245 a, then load the record by id. You should see that message in italics in the Record Summary at the top of the page. If you instead *search* for the record (by title, author, whatever) and click it in the search results, you *will not* see the title italicized. It will instead show the literal <i> </i> tags in the summary as you would expect. I believe this is because Display Fields are used when performing searches and not when loading a record directly but I'm not having a lot of luck tracking down where the difference is in the client.

I think this is one of the few remaining situations like this in the client but this feels like a game of whack-a-mole that we're likely to eventually lose. I wonder if we may have to eventually alter the way things work at a lower level to really put an end to this.

Changed in evergreen:
milestone: none → 3.7-beta
Changed in evergreen:
milestone: 3.7-beta → none
Changed in evergreen:
milestone: none → 3.7-beta
Changed in evergreen:
assignee: nobody → Jason Stephenson (jstephenson)
Jeff Davis (jdavis-sitka) wrote :

The attached SQL file will insert a bad MARC record (Nine Hundred Grandmothers by R. A. Lafferty) into biblio.record_entry; after insertion, retrieving the bad record by ID should show a bunch of alert dialogs unless the fix is applied.

Jason Stephenson (jstephenson) wrote :

I have pushed my signoff of Jeff's and Jason's commits to security/collab/dyrcona/lp1902965_marc_xss.

I noticed that italics and emphasis tags did have an effect in the Angular Staff catalog view. However, script tags were unable to open alerts. I didn't explore this much farther, so that bears looking into. I would be content to consider that a different bug in order to get this into a release sooner.

tags: added: signedoff
Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
tags: added: needsreleasenote
Jason Stephenson (jstephenson) wrote :

I have pushed backport branches for rel_3_5 and rel_3_4 to the security repository:

collab/dyrcona/lp1902965_marc_xss-rel_3_5

and

collab/dyrcona/lp1902965_marc_xss-rel_3_4

Jason Stephenson (jstephenson) wrote :

I was planing to include this in this week's releases. The code back ports cleanly to 3.6, 3.5, and 3.4 if you skip the bootstrap commits for 3.5 and 3.4

However, the code appears to have a conflict with the read more/accordion code in master, and I do not feel competent to resolve it.

I've removed the release targets from the bug and added the needsrepatch tag.

tags: added: needsrepatch
removed: pullrequest signedoff
Changed in evergreen:
milestone: 3.7-beta → none
Changed in evergreen:
assignee: nobody → Jeff Davis (jdavis-sitka)
Jeff Davis (jdavis-sitka) wrote :

Thanks for testing, Jason.

Security branch user/jeffdavis/lp1902965-filter-marc-fields-2 includes a revised version of the TPAC changes to accommodate the Read More accordion. (Please note I have force-pushed changes to this branch since it was first created a few days ago.) The previous branch should be used for 3.6 and earlier releases.

In this new branch, the accordion macro takes care of filtering, so any test that gets accordion'd shouldn't need an additional html filter.

It appears that the accordion does not exist in the Bootstrap OPAC, so there is some code drift happening between the OPACs. Some adjustments to HTML filtering will be necessary if and when the accordion does make it into Bootstrap.

It is frustrating to have to redo a security fix because it has languished while new features are committed.

Changed in evergreen:
assignee: Jeff Davis (jdavis-sitka) → nobody
milestone: none → 3.7-beta
Changed in evergreen:
assignee: nobody → Jason Stephenson (jstephenson)
Jason Stephenson (jstephenson) wrote :

I have not been able to reproduce the bug on master this morning.

I added a record that should trigger this bug to a concerto data set and added an item. I have not been able to find this record using the catalog search feature in order to open it with an affected interface.

I'm removing myself from this bug so someone else can have a look.

Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
tags: added: pullrequest
removed: needsrepatch
Galen Charlton (gmc) wrote :

I've pushed to security/user/gmcharlt/lp1902965 a signoff of Jeff's latest along with a follow-up fixing an issue that it revealed in the accordianization of notes.

At this moment, I believe that the branches to base release-cutting:

3.7-beta: user/gmcharlt/lp1902965 (or more strictly speaking, cherry-picks from that)
3.4: security/collab/dyrcona/lp1902965_marc_xss-rel_3_4
3.5: security/collab/dyrcona/lp1902965_marc_xss-rel_3_5
3.6: security/collab/dyrcona/lp1902965_marc_xss-rel_3_6

Jane Sandberg (sandbej) on 2021-03-31
Changed in evergreen:
status: Confirmed → Fix Committed
Changed in evergreen:
status: Fix Committed → Fix Released
status: Fix Released → Fix Committed
Galen Charlton (gmc) on 2021-04-01
information type: Private Security → Public Security
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers