Filter custom CSS library setting using HTML::Defang

[Jeff Davis]

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)]

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)]

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)]

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:
485624d4a7bc8d44843857060b72a813e47573ab
11868e97844a47d57489062197ac6809d9473366

and these master only commits:
3b9b5e7b22d2b39f11dc518d1023abac9d067901
f11b2356b39e5e7a1a3b728c072cd531dc93125d
da42d5649ea4bc12a2d2ab28d4bddca15ad0278d

(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)]

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)]

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

[Galen Charlton (gmc)]

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)]

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.

[Jeff Davis (jdavis-sitka)]

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:

https://git.evergreen-ils.org/?p=working/Evergreen.git;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.

[Galen Charlton (gmc)]

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

[Jason Boyer (jboyer)]

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.

[Rogan Hamby (rogan-hamby)]

I've put a branch to use HTML::Defang on the incoming setting. I'm going to go ahead and slap a pull request on it to improve chances of eyeballs. It has worked in my limited testing, taking script calls in css and commenting them out.

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=242ea9f55465c462b66caaf6fc260c4d738d07a3

[Rogan Hamby (rogan-hamby)]

Had a momentary failure of brain finger coordination so thinko fix pushed to 574dda8579c6a0a1b55d8a8cf7327e499e10f4b6

[Jane Sandberg (sandbergja)]

Thanks, Rogan! This does remove potentially harmful <script> tags and js before they can bug a user. A note for other testers, because it took me a little while to figure out: the contents of this setting are filtered when the setting is saved, so existing values in the opac.patron.custom_css setting won't be affected (until they are saved again).

I think this is a really good improvement! Signoff at user/sandbergja/lp1869971_defang_css-signoff / https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/sandbergja/lp1869971_defang_css-signoff

I exercised the patch as follows:
1. Set the value of the opac.patron.custom_css library setting to the string here: https://gist.githubusercontent.com/sandbergja/b27ba0b7afeaefd5c989fc7f93274b6c/raw/296c2d313cacefba1684195cf83e32e911566bb3/bad.css (launchpad won't let me put it in the ticket verbatim -- it's: body { background-color: purple; }, then a closing style tag, then a script tag with a javascript alert in it)
2. Confirmed that, when I went to the OPAC, the purple displayed (good) but a javascript alert also displayed (bad).
3. Removed the setting
4. Applied Rogan's commit
5. Repeated step 1
6. Checked the OPAC again. It's purple still (good), but the arbitrary javascript doesn't affect the user, it gets commented out.

[Galen Charlton (gmc)]

Pushed to main, rel_3_12, and rel_3_11, along with a follow-up that adds a belt to Rogan's suspenders by applying HTML::Defang to the value of the setting in the OPAC templates. (Running a string through HTML::Defang twice works). That way, any questionable tags included in the CSS will get removed without requiring that the setting be saved again.

Thanks, Rogan and Jane!