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

Protect MODx.grid.Grid against XSS vulnerabilities by default #14344

Merged
merged 3 commits into from Feb 9, 2019

Conversation

@Mark-H
Copy link
Collaborator

Mark-H commented Feb 7, 2019

What does it do?

When a MODx.grid.Grid/MODx.grid.LocalGrid implementation does not define a renderer for a column, automatically add the Ext.util.Format.htmlEncode renderer which protects the grid from XSS vulnerabilities.

When a renderer is set, no action is taken, and the implementation is assumed to handle its own protections. This allows implementations to still do things like add links or buttons into grids.

Why is it needed?

The manager is very prone to XSS vulnerabilities. This has been discussed in different places, most notably #14094. In the past week, a lot of XSS-related issues were raised and also fixed, but those are all putting bandaids on the core problem, which is that ExtJS was not configured in a way to treat any value as potentially hostile user input.

This PR changes that and automatically escapes all values in grids, unless otherwise instructed. The PR also escapes values from a combobox that are rendered through an editor configuration.

That's also why this targets 3.x - while security issues would typically go into a bugfix release, it is likely that compatibility issues in both core and third party extras will surface where those grids took advantage of the lack of encoding to insert links/buttons/other html into values.

This does not fix all XSS issues in the manager, but it does stop a lot of them (like the list here).

Related issue(s)/PR(s)

#14094

Mark-H added some commits Feb 7, 2019

Apply the htmlEncode renderer to MODx.grid.Grid columns, when not alr…
…eady set, to prevent XSS vulnerabilities

@Mark-H Mark-H requested a review from opengeek as a code owner Feb 7, 2019

This was referenced Feb 7, 2019

@Mark-H

This comment has been minimized.

Copy link
Collaborator Author

Mark-H commented Feb 7, 2019

Sorry @Alroniks - found another potential vector that could be fixed globally (automatically rendered combos from an editor configuration), so added another commit after you approved.

@Ibochkarev

This comment has been minimized.

Copy link
Contributor

Ibochkarev commented Feb 9, 2019

Related #14342

@JoshuaLuckers JoshuaLuckers self-assigned this Feb 9, 2019

@JoshuaLuckers JoshuaLuckers merged commit 9ce7c4d into modxcms:3.x Feb 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Ibochkarev

This comment has been minimized.

Copy link
Contributor

Ibochkarev commented Feb 10, 2019

@Mark-H Please see my comment #14335 (comment)

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