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

Stored XSS #14094

Closed
AnusyaAngamuthu opened this issue Sep 26, 2018 · 14 comments
Closed

Stored XSS #14094

AnusyaAngamuthu opened this issue Sep 26, 2018 · 14 comments

Comments

@AnusyaAngamuthu
Copy link

Stored XSS: The application is vulnerable to stored XSS.

Step to reproduce

  1. Under Media->Media sources choose Create New Media source and enter the Xss Payload <img src=xss onerror=alert("giraffee")> in the Name field and click on save.
  2. The application renders the entered script and displays a pop-up whenever the page is being visited by the user.

Observed behavior
The application processes the html tags or scripts and it is getting stored in the database.

Expected behavior
It should not accept any scripts or html tags.

Environment
MODX version:MODX Revolution 2.6.5-pl

@sottwell
Copy link
Contributor

Disagree. In the first place, this can only be done by a trusted Manager login. Access to Media Source management is controlled by ACLs, so lesser-trusted managers are easily excluded.

In the second place, in a CMS such as MODX, it is a feature that such functions as assigning names or attributes of elements are highly flexible, allowing all sorts of programmatic assignments.

@Mark-H
Copy link
Collaborator

Mark-H commented Sep 26, 2018

The manager being a powerful tool should not be an excuse to "disagree" with vulnerabilities. It's a textbook example of XSS vulnerabilities, there's no discussion possible about that.

I think we all agree it's not the most severe of issues, though. Otherwise @AnusyaAngamuthu would've reported this to the private mailing list.

I looked at a fix for this, but unfortunately it seems that it's not specific to the sources grid. There is nothing specific in MODx.grid.Sources that is making this vulnerability possible (e.g. a custom tpl that forgot a htmlent filter like we've seen elsewhere), meaning it goes up the tree to MODx.grid.Grid and even Ext.grid.EditorGridPanel from what I can tell. Perhaps even beyond that. That suggests practically every grid in the manager may be vulnerable for similar issues.

There are some suggested solutions on the web to make grids encode by default, but they would likely cause unexpected side-effects in both core and third party components that do expect markup to be accepted (columns that include a link or buttons, for example, would likely also be encoded). Would perhaps be better to address that in 3.0, but that does mean we're leaving the door open to all sorts of XSS issues until then.

(As a side note, the XSS also affects the Files tree. That should be patched separately.)

@Omeryl
Copy link
Member

Omeryl commented Sep 26, 2018

@Mark-H

Agreed. The problem with stored XSS in the manager is that if a user were able to inject something from the frontend (say, a contact form with a CMP in the manager) they could run javascript code in the context of a sudo user.

@mrhaw
Copy link
Contributor

mrhaw commented Sep 27, 2018

This is an awesome read https://www.markhamstra.com/extjs/

@JoshuaLuckers
Copy link
Collaborator

Thanks for taking time to report this issue and to make MODX better and safer.

It is strongly recommended to not share any known vulnerabilities with the general public and instead inform the MODX Security Team.

@abergmann
Copy link

CVE-2018-17556 was assigned to this issue.

@AgelxNash
Copy link
Contributor

AgelxNash commented Oct 1, 2018

I can upload the SVG file with JavaScript. This will also be vulnerability, while CSRF token not yet implemented....

I can upload the ZIP file with PHP file. And then unpack it. Pofit! I uploaded the PHP file!!! It's all different vulnerability, but they are executed from the administration panel...

I think this position will be very useful: WonderCMS/wondercms#57

Even with minimal rights in the administration panel, you can easily elevate the rights to the administrator. Perform arbitrary reading of files, running snippets, reading information from the database, etc. Therefore, XSS through the administration panel is the least evil.

@mrhaw
Copy link
Contributor

mrhaw commented Oct 1, 2018

My 2 cents:

  1. Put the effort to make FRED https://fred.modx.com/ secure in this regard. Then it be sweet if we could block access to the regular manager.
  2. Utilize front-end and web users before trusting people with Manager logins.

// I setup manager users with very restricted access and lots of Form Customization. I limit access to extras by category etc. It is possible to control most stuff, but still I feel the need to trust the user before letting him in.

@AgelxNash
Copy link
Contributor

And again: #14105 #14104 #14103 #14102
Do you want to continue to receive such reports about XSS in the admin panel?

@attritionorg
Copy link

If the admin role is expected to be able to insert JavaScript, then a) document it on a page by page or feature by feature basis and b) consider putting a one-line warning in the interface on each page, that the admin has that ability and such script can be dangerous. Any subsequent reports of XSS in the admin panel mean the researcher ignored both documentation and warning clearly indicating it is intended functionality.

@JoshuaLuckers
Copy link
Collaborator

@AgelxNash Thanks for taking time to report this issue and to make MODX better and safer.

It is strongly recommended to not share any known vulnerabilities with the general public and instead inform the MODX Security Team.

@Mark-H
Copy link
Collaborator

Mark-H commented Oct 15, 2018

I do think we need to have a candid discussion about how secure the manager needs to be.

My personal guideline for when to worry about vulnerabilities in the manager has always been about if the limited user account on the modmore demo site is enough to do harm/elevate rights/run arbitrary server side code. While that is not the case for the media source name reported in this issue, #14105 might actually pose a risk because a fairly low-level permission is used there.

The only way to truly stop XSS issues from coming up time and time again is to implement security-first rendering. Escape every input, unless it has been specifically marked to be trusted. There appear to be options for that in ExtJS, but they'll cause things to break in both the core and extras. Perhaps we need to consider doing that in 3.0 so we can put this to bed, and spend the time we save on dealing with individual XSS issues in the future on fixing the things that the change will break.

@AgelxNash
Copy link
Contributor

@Mark-H I think, you can using Content Security Policy headers

@Mark-H
Copy link
Collaborator

Mark-H commented Feb 7, 2019

I've created PR #14344 to apply htmlEncode to all grid columns by default, targetting 3.0.

@AgelxNash AgelxNash mentioned this issue Feb 15, 2019
opengeek added a commit that referenced this issue Feb 22, 2019
* origin/2.x: (104 commits)
  Change the RSS feed URLs to HTTPS
  MODX Revolution 2.7.1-pl
  Update lexicons from crowdin
  Change after review
  Update phpThumb 1.7.15-201902101903
  Restore html in resource tree (#14358) while preserving XSS protections in trees by default
  Handle deprecated $type and $responseCode parameters in $modx->sendRedirect and fix message
  Update lexicon entry
  Include not deleted children of deleted parents in the list
  Using cltr/cmd and click will open the url in a new tab/window again for ExtJS elements that use `loadPage()` to open URLs
  Forbid generating child resources for deleted resources
  Fix #14094
  XSS in the tree
  Fix #14105
  Fix #14104
  Fix #14103
  Fix #14102
  Enable remote avatars
  Fix regression in resourcelist that prevents parents from working correctly
  Improve wording in variables
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants