Prevent custom scripts by default

[sebastienros]

By default every content part should prevent an input from rendering <script> tags. This should be opt-in on a per type part level. This means that even if a part editor permits it, the rendering would filter these out. We can provide a reusable service as many parts will need it.

As for templates, there is a permission to edit templates when the feature is enabled, such that not every user can edit templates and inject custom scripts in a view. Right now Editors can edit front-end templates, which is right. Contributors can't edit templates.

[sebastienros]

Also maybe provide a filter and a global method such that workflows that render content can reuse the logic. In case a form requires users to enter some markdown or html (even though you would not allow a user to provide html directly, but markdown).

[sebastienros]

Use this instead of creating something with issues https://github.com/mganss/HtmlSanitizer

[thiennv-2147]

Dear sebastienros, Did your team already fix this issue?

[agriffard]

https://www.meziantou.net/sanitize-html-snippet-with-anglesharp.htm

[thiennv-2147]

Dear agriffard, Did you use it in RC1 for new patch?

[sebastienros]

WE should have the liquid support driven by a setting for markdown and html parts and fields, such that is safe by default. Also allow bbcode style flags that have to be safe.

[sebastienros]

@thiennv-2147 we won't patch older versions as they are not major releases. We will release this feature as part of the next release.

[thiennv-2147]

Dear sebastienros, So you will fix this issue in next release rc2, right?

[sebastienros]

@thiennv-2147 yes

[thiennv-2147]

Thank you, please notify me when you release a new version.

[ns8482e]

Only Administrator should be able to enable/disable at site level that should sanitize all fields/parts/templates that content editor can change.

[thiennv-2147]

@ns8482e yes, you can do as you describe.

[sebastienros]

Some other reflections:

Validating at Edit time, even preventing Liquid, is only sufficient as long as there are no other way to edit a content item without using the editor or without adequate permissions. For instance updating a content via the REST APIs or using a Workflow should require the same level of permission as being able to disable Sanitization.

@ns8482e just a reminder that we have a "Security Sensitive" category for permissions that could potentially be used for user priviledge elevation such as executing recipes (you could use a recipe to grant you some more permission, or enable custom features). "Disable Sanitization" could be such a permission. We usually use these to warn users that this is a security risk, but it's better to grant this permission rather than just "Admin" as it won't show parts of the systems you are not supposed to see, and could potentially do bad things easily if these sections/buttons appeared as you'd be Admin. Same as writing HTML templates. If you trust someone to write a template, then you should give as much trust as being able to steal auth cookies by creating XSS vulnerabilities when editing the content.

If sanitization should only be done at render time as it's the safest solution, we should still be able to render dynamic tags as these can contain script and be safe. So there should still be a set of dynamic tags (like liquid, bbode) that would be evaluated after sanitization because they are considered safe (containing aproved scripts, not scripts entered by the editor). So if Sanitization is enabled on a piece of content, Liquid would be disabled (as it can be used to enter custom scripts, even in the {{ }} form), but safe codes (bbcode) would still be processed.

There are many parts that will never have sanitization, like Liquid Part. And same with the Templates feature. So this feature is really to protect the most basic Editor that should just be able to write content in an editor, for very constrained content types (no liquid part available in the Flow widgets, no access to APIs or workflows or templates, no access to content type definition)

[ns8482e]

@sebastienros Yes, I meant the same, it should be driven by higher level permission more related to role of Admin and not related Content Editor and should not be configurable on individual Part setting instead it should be global level setting that each part should implement.

[sebastienros]

@ns8482e I want to be able to define "secured" Content Types that will be able to use script, while some other "secured" types might not be able to. For instance some editors could only edit Blog Post and we'd not allow scripts in these. But some other content types might allow that, and they would be editable only by another role. So I don't think at global single setting would allow that. Also that might show a false sign of security as some elements will definitely not be able to sanitize edited content: templates, liquid parts.

[sebastienros]

TODO:

• Change GetApiContent permission to AccessContentApi and protect all /api/content methods with it. If you don't want a user to see all the fields, just don't grant access to the apis at all, hence the GetApiContent permission is not required as a separate one. This will also protect POSt that currently returns the full content.
• Remove GetApiContent from Authenticated role as being authenticated should not grant access to all field.

I a user has AccessContentApi they can create non-sanitized content. Enabling sanitization and still allowing content api is "security sensitive" but at least it will prevent easy mistakes.

We can do sanitization at edit time for html, and still support safe blocks, not liquid. We can do sanitization of the markdown by evaluating the markdown without evaluating safe blocks, and checking that the sanitized results is the same as without sanitization. It's assuming that the markdown rendering will produce valid html, such that the sanitizer would not alter the output if no defect is detected.

Sample bbcode: [media]/photo.jpg?width=120[media] white list for path until ? then parse query string and re-render it for sanitization

Unit test: [media]/photo.jpg' onerror="alert('XSS')[media]

Keep liquid if we are not sanitizing, liquid first filters after.

[thiennv-2147]

Dear @sebastienros, Did you release a fixed version for this issue? I think it's better to admin can choose what role can use full script content or not!

[Piedone]

This is about preventing malicious dashboard-accessing but non-site owner users from elevating their access level by doing a scripting attack against the site's own admins, right? Because if yes then just not allowing <script> tags won't be sufficient, you can even write scripts in the src attribute of img tags.

[sebastienros]

@Piedone Yes it's about that, and we have thought about it too. HtmlSanitizer will prevent that.

@thiennv-2147 This feature will be released in rc2.

[thiennv-2147]

@sebastienros thank you! Please notify me when having rc2 version.

[sebastienros]

@thiennv-2147 I just discovered that you submitted this feature as a vulnerability? However I was very clear in my wording that we considered this as a new feature to prevent users from being able to write custom script in their HTML editors. So when you mention that a "patch" will be released, it is not correct: we are implementing an option to let administrators define what contents and what roles can use this. Only content editors can already write scripts in their content.

Also the way the issue is decribed, by using custom POST request instead of just showing that this is something that is done from the standard UI editors, WYSIWYG editors, tends to manipulate the message into thinking it's a more clever find that it actually is.

Furthermore I think that the steps you share to reproduce the issue you are describing don't include one important fact, that you need to be authenticated with an account that has the permissions to edit and publish content. That is a very important precision in my opinion. or please correct me if I didn't understand this correctly.

Update: I am totally fine with transparency and you can write and share all the things you want about this issue publicly. But I would kindly ask you to update the reports on how this issues can be reproduced, by at least showing that you need to have Publish permission on the site you want to inject scripts on.

[thiennv-2147]

Dear @sebastienros , I completely agree with you to release a new function to admin can turn on or off HTML attribute of exactly role permission. I can give you an example ò Wordpress:

• Administrator and editor can add full HTML to post
• Author can create a post and publish it but no HTML content with malicious javascript.

[sebastienros]

@thiennv-2147

What other CMS do is irrelevant to my concerns with what you published. Can you at least fix and complete your report as a first step?

[thiennv-2147]

Dear @sebastienros , Yes, of course, I only give you an example. Let do as you mention in the last comment