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

Make sure an invalid ctx parameter (array instead of string) doesn't throw errors #13627

Merged
merged 1 commit into from
Oct 18, 2017

Conversation

Mark-H
Copy link
Collaborator

@Mark-H Mark-H commented Oct 5, 2017

What does it do?

Ensures the ctx parameter when sent to the connector is ignored if it isn't a string.

Why is it needed?

If a different type, say array, is passed along into modX->initialize(), the connector will log and/or show notices and warnings in the response depending on the server configuration. When the server is set to show errors, this may expose core paths to attackers.

This came up in Slack (05:45:29) as a result of an ancient attack (fixed in 2.2.13/2.3.0: #11180) where the ctx parameter was vulnerable for sql injection. After investigating it a bit I found that the exploit can no longer be exploited (thanks to both the fix in #11180 and improvements to xPDO's sql injection detection in 2.5.x), but that it could still throw errors.

In my test environment, these errors were shown when providing an array ctx like in the exploit:

  • ( ! ) Notice: Array to string conversion in core/model/modx/modx.class.php on line 2325 (_initContext method)
  • ( ! ) Warning: preg_match() expects parameter 2 to be string, array given in core/xpdo/validation/xpdovalidator.class.php on line 82
  • ( ! ) Warning: Illegal offset type in isset or empty in core/model/modx/modx.class.php on line 2319

In addition, the following got written to the error log:

[2017-10-05 12:11:34] (ERROR @ core/model/modx/modx.class.php : 2102) Culture not initialized; cannot use lexicon.
[2017-10-05 12:11:34] (ERROR @ core/model/modx/modx.class.php : 2325) No valid context specified: Array
[2017-10-05 12:11:34] (ERROR @ connectors/index.php : 51) PHP warning: Cannot modify header information - headers already sent by (output started at core/model/modx/modx.class.php:2319)
[2017-10-05 12:11:34] (ERROR @ connectors/index.php : 52) PHP warning: Cannot modify header information - headers already sent by (output started at core/model/modx/modx.class.php:2319)

With this fix applied, both the output and the error log remain quiet.

As context keys can only be strings, I see no potential harm in ignoring the provided ctx value. Users that have no access to the mgr context will get an unauthorized error like normal.

Related issue(s)/PR(s)

Somewhat related to #11180, but just to clarify, this is not a regression or exploitable vulnerability.

@Jako Jako added area-core bug The issue in the code or project, which should be addressed. priority-2-high labels Oct 16, 2017
@Jako Jako added this to the v2.6.0 milestone Oct 16, 2017
@opengeek opengeek merged commit 8890627 into modxcms:2.x Oct 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core 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.

3 participants