Custom CSS considered harmful

Bug #1869971 reported by Jeff Davis on 2020-03-31
This bug affects 1 person
Affects Status Importance Assigned to Milestone

Bug Description

master / 3.5 beta

Bug 1849152 added a new "opac.patron.custom_css" org setting which allows a user to add custom CSS to the OPAC. It is trivial to use this setting to insert malicious code into every OPAC page. As a proof of concept, try setting it to this:

"</style><script>window.alert(\"uh oh\");</script><style>"

Bug 1849683 mitigated the issue by adding an update permission to the setting (this fix is in master already). But if we don't restrict allowable values or at least filter the value somehow before using it, we are asking for trouble.

I propose that we revert the new feature in 3.5/master until we have a more secure implementation.

Marking as Private Security -- the 3.5 beta release is affected and doesn't have the perm mitigation from bug 1849683, so some test environments are surely vulnerable.

Jeff Davis (jdavis-sitka) wrote :

One alternative would be to disable the feature by default via config.tt2. Admins who want the feature would have to turn it on in TT2 before staff can make use of it, while those of us who don't want it don't have to worry about someone getting the wrong permission.

Jeff Godin pointed out that bug 1849113 would allow staff to add custom jQuery to the OPAC in a very similar way, with the same drawbacks. That one hasn't been committed yet.

Bill Erickson (berick) wrote :

I agree it should have a hard off switch by default at minimum.

Another tool we could use is running the content through HTML::Defang, similar to print templates, which scrubs executable content. It would still offer quite a bit of power, though, to modify the UI.

I'm also not opposed to reverting the feature with consensus.

Jason Boyer (jboyer) wrote :

So, being responsible for initially committing this and not thinking it through enough at the time I don't believe it can realistically be made robust in the 3.5 timeframe and am for reverting these master / 3.5 commits:

and these master only commits:

(master only because I forgot yesterday that rel_3_5 had already been cut and rather than now also push them to rel_3_5 the feature can just be reverted everywhere applicable.) I do think upgrade script numbers 1196 (obviously) and 1201 should still be considered burned to prevent any possible future confusion. (Or altered to just insert the number and do nothing? Another point for discussion.)

I haven't done this yet because I wanted to see more people chime in on this first. (Though if someone is sufficiently motivated or impatient before that I won't complain.)

That said, I don't believe that bug 1849152 and bug 1849113 should be written off as things never to be considered; they can be a powerful way to allow less technical users (granted the appropriate permissions and warnings, etc.) to customize their installation in minor ways without having to involve a system admin just because you want to change the color of some widget from green to blue or similar. This is the primary way that customizations are performed in Koha, though I have yet to look at what they may be doing to prevent rogue admins from doing undesirable. (I don't really believe you can do a whole lot to "verify" the JavaScript stuff other than only granting the currently missing permission to people that you would also trust with psql access, or give the feature a 2-step change, then verify/apply mechanism.)

Galen Charlton (gmc) wrote :

As Jason mentioned, equivalent functionality has existed in Koha for years; I am not aware of particular security issues in practice, although this bug report is quite correct that the potential exists for a naive or malicious local admin to put in undesirable script tags.

It is my opinion that a reversion is overkill and that Evergreen admins can manage the risk via proper training of their local admins and taking care about to whom they assign UPDATE_ORG_UNIT_SETTING.opac.patron.custom_css.

I have no objection to adding a config.tt2 flag and adjusting the wording of the library setting to warn against dangerous values . This can definitely be accomplished before general release (and I am willing to commit to doing the work); I am also willing to see if HTML::Defang or similar approaches can be readily applied to sanitize the values. For that matter, tossing Template Toolkit replace() filters in base.tt2 to zap < and > should help ameliorate the issue.

However, allowing local admins more direct and responsive control for tweaking their catalogs is a valuable feature that I would hate to see get lost.

Galen Charlton (gmc) wrote :

Thinking a bit more, rather than the replace() TT filter, a custom one that wraps HTML::Defang would be generally useful.

Galen Charlton (gmc) wrote :

As a side note: since this bug is discussing a feature that is not available in a general release, I propose that we convert it to a public security bug.

Jason Boyer (jboyer) wrote :

I agree that this can be made public for broader discussion, especially as the 3.5 release has been shifted to give us more time to choose a path forward.

information type: Private Security → Public Security
Jeff Davis (jdavis-sitka) wrote :

Moved to public security since there were no objections to doing so.

Working branch user/jeffdavis/lp1869971-disable-custom-css-by-default adds a setting to config.tt2 to enable the custom CSS org setting, leaving it disabled by default:;a=shortlog;h=refs/heads/user/jeffdavis/lp1869971-disable-custom-css-by-default

I think this is the minimum necessary change if we aren't reverting the feature. I haven't attempted to sanitize the setting with HTML::Defang or anything else, but that would be nice to see too.

Thanks, Jeff. I'll put out an HTML::Defang filter branch by the end of the

Jason Boyer (jboyer) wrote :

An update since the release of 3.5 is closing in; I've applied the 3 patches for bug 1849683 to the rel_3_5 branch since this bug hasn't is still in progress and those 3 patches were only in master, but the feature itself was already in 3.5.

Changed in evergreen:
milestone: none → 3.5.0
tags: added: pullrequest
Changed in evergreen:
milestone: 3.5.0 → 3.5.1
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