modx::stripTags filtering may not work correctly #9080

modxbot · 2012-11-08T08:05:37Z

Anonymous created Redmine issue ID 9080

input: [[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]tag]]
output: [[tag]]

input: [[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]!tag]]
output: [[!tag]]

input: [[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]_tag]]
output: [[_tag]]

$text=$modx->stripTags($text);
$title=$modx->stripTags($title);
$data=array("content"=>$text,"pagetitle"=>$title);
//save data
//..
//..
//..
//
//display data

Is this normal?

For example: http://www.youtube.com/watch?v=tNlOg3MlUjk

modxbot · 2012-11-08T08:10:08Z

Anonymous submitted:

[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]tag]] -> [[tag]]
[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]!tag]] -> [[!tag]]
[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]*tag]] -> [[*tag]]

I'm sorry. Parser ate tags.

Combine this bug and http://tracker.modx.com/issues/9069.
Will now Fix?

Or will continue to ignore the bug to write articles in the style http://modx.com/blog/2012/05/29/why-we-dont-do-powered-by-by-default/

modxbot · 2012-11-08T08:51:32Z

Anonymous submitted:

We can use placeholders

[[++dsn]] [[++username]] and [[+password]]

[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]++password]]
[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]++dsn]]
[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]++username]]

And what a super-resistant hash and PDO do not save. It remains to find phpmyadmin

modxbot · 2012-11-11T21:41:43Z

bezumkin2 submitted:

In fact, blame the regular expression in $sanitizePatterns at modx.class.php on line 232

@[[(.[^[[]_?)]]@si

Need to change it to a more simple and reliable, such as

@[[(._?)]]@si

I treated all my sites with this command in server console:

find -type f -name modx.class.php -exec sed -i 's#[^\[\[]##g' {} ;

Use it only on you own risk!

splittingred · 2012-11-12T13:33:31Z

splittingred submitted:

A bit of FUD here; you can't use this to get to System Settings (doesn't work). TVs and Resource Fields to seem to show; that is correct and needs fixing.

However, bezumkin, your solution doesn't strip the extra [] and leaves the code injectable; that's not correct either. It should strip them all. Thoughts?

modxbot · 2012-11-12T13:49:18Z

Anonymous submitted:

It all depends on how the work is conceived of this code. When compared with the Evo, there is remove the tags. But still offers bezumkin replace brackets in html entity "& #91;" "& #93;"

gadamiak · 2012-11-12T19:26:27Z

gadamiak submitted:

splittingred wrote:

However, bezumkin, your solution doesn't strip the extra []

Should any single bracket be removed or only in pairs? The simplest PCRE search pattern is "@([|])+?@" for the former and "@([[|]])+?@" for the latter.

opengeek · 2012-11-12T20:34:40Z

opengeek submitted:

Single brackets cannot be removed, it is only tags, i.e. double brackets, that are the issue. If we strip out single brackets, it could kill a greater amount of legitimate POST data IMO. Ultimately, anything you put in the front-end should handle more detailed sanitization itself, while MODX simply tries to prevent you from pointing the gun at yourself by default.

gadamiak · 2012-11-12T23:36:32Z

gadamiak submitted:

opengeek wrote:

Single brackets cannot be removed

That's what I thought.

opengeek · 2012-11-13T02:57:51Z

opengeek submitted:

I separated the tag pattern into two separate patterns to scrub opening and then closing tags. Seems to be much more thorough.

modxbot · 2012-11-13T08:14:09Z

bezumkin2 submitted:

splittingred wrote:

However, bezumkin, your solution doesn't strip the extra [] and leaves the code injectable; that's not correct either. It should strip them all. Thoughts?

This was a quick solution but it always remove all [[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[ and will left only ]]]]]]]]]]]]]]]]]]*id]].
If cracker will see this extra brackets instead of my data - i just hurt his sense of beauty!

modxbot · 2012-11-13T08:14:48Z

bezumkin2 submitted:

.

modxbot · 2012-11-13T13:31:13Z

Anonymous submitted:

64f04d2 This is not the correct patch
string <[[a]]script>a</script> is replaced by <script>a</script> and string "]]" is replaced by empty. This is not the correct!!!

modxbot · 2012-11-13T14:34:57Z

bezumkin2 submitted:

Agel_Nash wrote:

string <[[a]]script>a</script> is replaced by <script>a</script>

It seems, like we need to correct modX::sanitizePatterns:

    public $sanitizePatterns = array(
        'tags1'         => '@\[\[(.*?)\]\]@si',
        'tags2'         => '@(\[\[|\]\])@si',
        'scripts'       => '@]*?>.*?</script>@si',
    'entities'      => '@&#(\d+);@e',
);

All tags will be stripped first, then will be processed scripts and entities.

modxbot · 2012-11-13T14:40:35Z

danya_postfactum submitted:

What about <script>a</script > (extra space at the closing tag end)? This is valid script element.

opengeek · 2012-11-14T02:19:47Z

opengeek submitted:

I've modified sanitize to g 8000 o through all the patterns in a loop until no matches occur on any of the patterns. This should prevent compromise through nesting different matches to take advantage of the pattern order.

The single regex for filtering the modx tags was not sufficient for cleaning all nested combinations. Separating them into open and close tag pairs makes it much easier to completely filter the tags.

…ulnerability

This should have always been disabled by default. Users who want this enabled will have to do so manually after 2.2.6 upgrade.

Merge branch 'release-2.2' * release-2.2: (40 commits) Bump version for 2.2.6-pl [modxcms#9178] Use PHP time for valid check in modDbRegisterMessage::getValidMessages() [modxcms#9165] Fix modError::hasError false positives when loaded via getService [modxcms#9029] Remove modRequest->loadErrorHandler dependency in runProcessor [modxcms#9156] Fix reload data for rendering multi-value TV types properly Prevent re-mapping custom Policies and Policy Templates [modxcms#7916] Fix Area functionality in Element Properties and Property Sets [modxcms#9080] Disable allow_tags_in_post System Setting on upgrade Add configcheck warning for allow_tags_in_post settings enabled fix css misprints Remove recursion in onCellDblClick() modx.grid.local.property.js Updated Dutch lexicon Remove unnecessary connector call on login tpl Update phpdoc comment Image optimization applied across distribution (changelog entry) Image Optimization Fixed copyright 2011 to 2012 [modxcms#9006] Fix ImageMagick which convert issue (PHP 5.3.2+) CS translation of updated strings in Setting Lexicon [modxcms#9080] Avoid filter order compromises in modX::sanitize() ...

* release-2.2: (23 commits) fix css misprints Remove recursion in onCellDblClick() modx.grid.local.property.js Updated Dutch lexicon Remove unnecessary connector call on login tpl Update phpdoc comment Image optimization applied across distribution (changelog entry) Image Optimization Fixed copyright 2011 to 2012 [modxcms#9006] Fix ImageMagick which convert issue (PHP 5.3.2+) CS translation of updated strings in Setting Lexicon [modxcms#9080] Avoid filter order compromises in modX::sanitize() Updating French translation [modxcms#9069] Remove math output filter Set allow_tags_in_post false in default System Settings Make sanitizeRequest handle GET vars as arrays Adjust nesting value accidentally left at 99 [modxcms#9080] Fix modX::stripTags() bugs allowing script execution vulnerability [modxcms#9080] Separate open and close tags in modX::$sanitizePatterns Replaced MODX logo in setup [modxcms#9007] Add changelog entry ... Conflicts: core/docs/changelog.txt

This should have always been disabled by default. Users who want this enabled will have to do so manually after 2.2.6 upgrade.

* release-2.2: [modxcms#8101] Add support for httpOnly session cookies in PHP 5.2+ Adding System Setting for HttpOnly cookies phpdoc comment correction [modxcms#8420] Add changelog entries for flock-independent file locking issues Bump version for 2.2.7-dev [#8423] Add multi-node support to flock-independent file locking Set log_level and log_target from xPDO constructor options Bump version for 2.2.6-pl [modxcms#9178] Use PHP time for valid check in modDbRegisterMessage::getValidMessages() [modxcms#9165] Fix modError::hasError false positives when loaded via getService [modxcms#9029] Remove modRequest->loadErrorHandler dependency in runProcessor [modxcms#9156] Fix reload data for rendering multi-value TV types properly Prevent re-mapping custom Policies and Policy Templates [modxcms#7916] Fix Area functionality in Element Properties and Property Sets [modxcms#9080] Disable allow_tags_in_post System Setting on upgrade Add configcheck warning for allow_tags_in_post settings enabled Conflicts: _build/build.sample.properties _build/build.xml _build/data/transport.core.system_settings.php core/docs/changelog.txt core/docs/version.inc.php core/model/modx/modx.class.php

enigmatic-user pushed a commit to enigmatic-user/revolution that referenced this issue Feb 13, 2014

[modxcms#9080] Fix modX::stripTags() bugs allowing script execution v…

64f04d2

…ulnerability

enigmatic-user pushed a commit to enigmatic-user/revolution that referenced this issue Feb 13, 2014

[modxcms#9080] Avoid filter order compromises in modX::sanitize()

83826bd

This issue was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modx::stripTags filtering may not work correctly #9080

modx::stripTags filtering may not work correctly #9080

modx::stripTags filtering may not work correctly #9080

modx::stripTags filtering may not work correctly #9080

Comments