Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional optional sanitization of scripting in TinyMCE #10653

Merged

Conversation

nielslyngsoe
Copy link
Member

@nielslyngsoe nielslyngsoe commented Jul 12, 2021

As of GHSA-w7jx-j77m-wp65 and GHSA-5vm8-hhgr-jcjp we need to ensure to prevent inserting inline arbitrary JavaScript execution in HTML of the TinyMCE editor.

This fix eliminates the option to insert inline 'on' attributes.

This fix also eliminates using javascript in URIs of various element types. Including checking the validity of the URIs. If not a valid URI then it will be removed.

This also means that inline data in src tag is being limited to only be possible for data of type image or svg.

The additional sanitization is disabled for sites that are upgrading from an older version. Clean installs of Umbraco will have the option enabled by default. Enabling the option will not automatically remove any stored scripting, each TinyMCE instance on each content item will need to be saved manually.

Enabling the additional sanitization can be done in the web.config by adding the following appSetting:

<add key="Umbraco.Web.SanitizeTinyMce" value="true" />

This works is based on the fixes done for TinyMCE v5:

We determined which attributes can contain possibly problematic scripting.

Enabling this configuration will do the following:

  • Remove any attributes in HTML starting with on (onClick, onMouseOver, etc).
  • Remove any scripting (javascript, vbscript, mhtml) from the following attributes on any html tag
    • src
    • href
    • data
    • background
    • action
    • formaction
    • poster
    • xlink:href
  • Preserve embedded svgs in the data:image attribute on img and video tags, scripts in those tags never get executed by browsers

This fixes #10217


This item has been added to our backlog AB#13289

@nielslyngsoe nielslyngsoe added the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Aug 13, 2021
@nikolajlauridsen
Copy link
Contributor

nikolajlauridsen commented Sep 27, 2021

I've updated this so the patch is only applied if the following is present in the appsettings section of web.config:

<add key="Umbraco.Web.SanitizeTinyMce" value="true" />

If the key is missing or set the false the patch will not be applied, making it opt-in, I've also updated the web.template.config to have this key, so future projects will automatically have this key, making it opt-out for future projects.

Ideally, I would like to have placed this in either the security section of umbracoSettings.config or in the tinyMce.config however as far as I can tell these configs are not generated from a template, meaning that if I change it there it would break existing sites, which is no bueno, however, this would not be a problem in V9 since all settings are loaded from appsettings.json which is generated from a template.

@nul800sebastiaan nul800sebastiaan changed the title fixing cross-site scripting vulnerability in TinyMCE Additional optional sanitization of scripting in TinyMCE Nov 2, 2021
@nul800sebastiaan nul800sebastiaan added release/8.18.0 release/9.1.0 type/feature and removed state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks labels Nov 2, 2021
@nul800sebastiaan nul800sebastiaan merged commit f68dba7 into v8/contrib Nov 2, 2021
@nul800sebastiaan nul800sebastiaan deleted the v8/feature/TinyMCE-scripting-vulnerability-fix branch November 2, 2021 12:21
nul800sebastiaan pushed a commit that referenced this pull request Nov 3, 2021
@nul800sebastiaan nul800sebastiaan linked an issue Nov 3, 2021 that may be closed by this pull request
@nul800sebastiaan
Copy link
Member

Cherry picked for 8.17.1 in 23ce69d

bergmania added a commit that referenced this pull request Nov 22, 2021
* Changes to GetReducedEventList (#11444)

* Instead of only using first event, we combine events of same type into a single event with multiple arguments

* Added generic method to DRY up grouping logic.

* Renamed method to better reflect new functionality.

Co-authored-by: Andy Butland <[email protected]>

* Merge pull request #11360 from umbraco/v8/bugfix/11057-mandatory-image-not-validating-after-first-time-failure

Fixes 11057: Mandatory Image not validating after first time failure

(cherry picked from commit 5cc70d2)

* Additional optional sanitization of scripting in TinyMCE (#10653)

(cherry picked from commit f68dba7)

* Bump version to 8.17.1

* Hide localization key while loading

* ContentVersion cleanup backoffice UI (#11637)

* init rollback ui prototype

* add busy state to button, deselect version, add pagination status

* add localisation

* style current version

* disable rollback button when nothing is selected

* stop click event

* Endpoints for paginated content versions.
Light on tests, tight on time.

* Endpoints to "pin" content versions

* camel case json output.
Not sure why json formatter not set for controller, bit risky to add it now

* wire up paging

* wire up pin/unpin

* rename getPagedRollbackVersions to getPagedContentVersions

* prevent selection of current version and current draft

* add current draft and current version to UI

* remove pointer if the row is not selectable

* Improve warning for globally disabled cleanup feature.

* Fix current loses prevent cleanup state on publish.

* Added umbracoLog audit entries for "pin" / "unpin"

* Match v9 defaults for keepVersions settings

* Fix - losing preventCleanup on save current with content changes

* update pin/unpin button labels

* fix pagination bug

* add missing "

* always send culture when a doc type can vary

Co-authored-by: Mads Rasmussen <[email protected]>

* Bugfix - DocumentVersionRepository.Get should not join culture variation

* Bugfix - Missing write lock

* Bugfix - Policy returns items to delete not items to keep.
Switch to inverse behavior.

Co-authored-by: Andy Butland <[email protected]>
Co-authored-by: Nikolaj Geisle <[email protected]>
Co-authored-by: Niels Lyngsø <[email protected]>
Co-authored-by: Sebastiaan Janssen <[email protected]>
Co-authored-by: Ronald Barendse <[email protected]>
Co-authored-by: Paul Johnson <[email protected]>
Co-authored-by: Mads Rasmussen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security Scan reports vulnerable TinyMCE version lib.
4 participants