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

Fix some minor XSS issues reported in security ticket 69 #13887

Merged
merged 2 commits into from May 24, 2018

Conversation

Mark-H
Copy link
Collaborator

@Mark-H Mark-H commented May 1, 2018

What does it do?

Applies htmlspecialchars to some processors which were exposing 3 stored XSS issues.

Why is it needed?

Tighten security a bit.

None of these XSS issues should have any far-reaching consequences. They all require manager access for starters. Also

  • The one in the recently edited dashboard widget would only be useful to XSS yourself, as the resource list only shows resources you personally edited. Perhaps a problem when sharing user accounts.
  • Access to edit the top menu (actions) is needed for number 2. That's an advanced permission that only full admins should have.
  • Adding/editing setting permission is needed for number 3. Also, that should already be limited to full admin users.

Related issue(s)/PR(s)

Security inbox ticket number 69. Reported by Aman Sahey on April 25th.

@Mark-H Mark-H requested a review from opengeek as a code owner May 1, 2018 22:01
@Jako
Copy link
Collaborator

Jako commented May 2, 2018

Maybe you should use $modx->getOption('modx_charset', $options, 'UTF-8')

Copy link
Member

@opengeek opengeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur with @Jako in that you should be using $modx->getOption('modx_charset', $options, 'UTF-8') rather than hardcoding UTF-8.

@Omeryl
Copy link
Member

Omeryl commented May 19, 2018

Just a minor note, looks like we're going to need to go back and fix this up in a number of other places:

'text' => htmlentities($group->get('name'), ENT_QUOTES, 'UTF-8') . ' ('.$group->get('id') . ')',

@opengeek opengeek merged commit 56f131f into modxcms:2.6.x May 24, 2018
@opengeek opengeek added this to the v2.6.4 milestone May 24, 2018
@opengeek opengeek added bug The issue in the code or project, which should be addressed. area-security priority-2-high labels May 24, 2018
@Mark-H Mark-H deleted the security-xss-69 branch June 7, 2018 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-security bug The issue in the code or project, which should be addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants