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

"on" replaced by "data-cke-671a164ccbf7b-on" in CKEditor #5887

Open
ywarnier opened this issue Oct 24, 2024 · 5 comments
Open

"on" replaced by "data-cke-671a164ccbf7b-on" in CKEditor #5887

ywarnier opened this issue Oct 24, 2024 · 5 comments

Comments

@ywarnier
Copy link
Member

1.11.28 (just released) came with many security fixes, one of them being to filter "on(event)" words in any HTML edited through CKEditor.
However, this filter was too wide and actually replaced all "on" words preceded by a space by something like "data-cke-671a164ccbf7b-on", which is a big issue.

This stems from this commit: df47eac which added the "attr_on_filter()" method, which is not strict enough.

We are working on a fix.

@ywarnier ywarnier added the Bug label Oct 24, 2024
@ywarnier ywarnier added this to the 1.11.30 milestone Oct 24, 2024
ywarnier referenced this issue Oct 24, 2024
Fix GHSA-8qqw-rjh4-5gp2
@ywarnier
Copy link
Member Author

Given it's a filter on attributes, the regular expression should at least ensure it is withing an HTML tag or (because this might not cover enough) ensure that it checks for a given number of known events from the JavaScript language (this is limited because this might change over time) or to just try and recognize a syntax where the event can be declared as any text contained between "on" and "=", but this might also be too wide and trap any text contained between an "on" word and an equal sign.

ywarnier added a commit that referenced this issue Oct 24, 2024
… and replaced normal text containing " on" - refs #5887
@ywarnier
Copy link
Member Author

The patch above might still cause issue (mostly in the English language) if a text contains some formula that has " on[something] =" in it, but we assume cases like this will be limited.

@ywarnier
Copy link
Member Author

The fix is just one line to change in Chamilo's code, but we're preparing a corrective release to make sure this is included.

@ywarnier
Copy link
Member Author

In the meantime, try to avoid the "on" word with a space/separation marker in front in anything edited through the WYSIWYG editor.

AngelFQC added a commit that referenced this issue Oct 24, 2024
…al text containing " on" - refs #5887

Fix df47eac
Advisory GHSA-8qqw-rjh4-5gp2
@AngelFQC
Copy link
Member

AngelFQC commented Nov 4, 2024

After 3e2582f

  • Remove On* attributes single quotes
 on onion onboard <img src="image.jpg" onclick='alert("click!")'>
 on onion onboard <img src="image.jpg">
  • Remove On* attributes double quotes
 on onion onboard <img src="image.jpg" onmouseover="alert('hover!')">
 on onion onboard <img src="image.jpg">
  • Remove On* attributes no quotes
 on onion onboard <img src="image.jpg" onerror=alert(1)>
 on onion onboard <img src="image.jpg">
  • No On* attributes
 on onion onboard <img src="image.jpg" alt="An image">
 on onion onboard <img src="image.jpg" alt="An image">
  • Remove multiple On* attributes
 on onion onboard <img src="image.jpg" onclick="alert('click!')" onmouseover="alert('hover!')" alt="Image">
 on onion onboard <img src="image.jpg" alt="Image">
  • Remove mixed On* attributes
<div class="container" onload="init()" onfocus="focusHandler()">on onion onboard <img src="image.jpg" onclick="alert('click!')"></div>
<div class="container">on onion onboard <img src="image.jpg"></div>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants