XSS problem with QM+ plugin on modx 1.0

164 Posts

soushi Reply #1, 16 years, 11 months ago

Dear all,

I post to this private forum.
Because this problem is security problem.

There is XSS problem in QM+ plugin.

Example code:

http://DOMAIN/assets/plugins/qm/close.php?id='"><script>alert("XSS!")</script><'

Fix code:
/assets/plugins/qm/close.php (about 11line)

if (isset($_GET['id'])) $id = $_GET['id'];
if (isset($_GET['baseurl'])) $baseurl = $_GET['baseurl'];
if (isset($_GET['action'])) $action = $_GET['action'];

↓

$id = isset($_GET['id'])?htmlspecialchars($_GET['id'],ENT_QUOTES):'1';
$baseurl = isset($_GET['baseurl'])?htmlspecialchars($_GET['baseurl'],ENT_QUOTES):'';
$action = isset($_GET['action'])?$_GET['action']:'';

* "$action" no necessary htmlspecialchars function.

Please fix this problem.
I will notify in the forum in Japan if this problem is fixed.

Sincerely yours,
soushi

164 Posts

soushi Reply #2, 16 years, 11 months ago

Dear all,

I found another problem.
It is possible to induce user to another site by using counterfeited URL.

Example code:

http://DOMAIN/assets/plugins/qm/close.php?baseurl=http://example.com'">

Fix code:

$baseurl = isset($_GET['baseurl'])?htmlspecialchars($_GET['baseurl'],ENT_QUOTES):'';

↓

$baseurl = isset($_GET['baseurl']) && !preg_match('/^[ \t]*https?:\/\//',$_GET['baseurl']) ?htmlspecialchars($_GET['baseurl'],ENT_QUOTES):'';

Sincerely yours,
soushi

634 Posts

Lammikko Reply #3, 16 years, 11 months ago

I’ll fix these and report back ASAP (in 24 hours).

Mikko Lammi, Owner at Maagit

164 Posts

soushi Reply #4, 16 years, 11 months ago

Dear Lammikko,

Thank you for correspondence.

There is a supplementation.
As for the fix code that I contributed last time, it is not enough yet.

/assets/plugins/qm/close.php (about 10line)

// Get parameters
if (isset($_GET['id'])) $id = $_GET['id'];
if (isset($_GET['baseurl'])) $baseurl = $_GET['baseurl'];
if (isset($_GET['action'])) $action = $_GET['action'];

↓

// Get parameters
$id = isset($_GET['id']) && preg_match('/^[0-9]+$/',$_GET['id']) ? $_GET['id']:'1';
$baseurl = isset($_GET['baseurl']) && !preg_match('/^[ \t]*https?:\/\//i',$_GET['baseurl']) ? htmlspecialchars($_GET['baseurl'],ENT_QUOTES):'/';
$action = isset($_GET['action']) ? $_GET['action']:'';

Could you confirm whether there is problem?
By the way,Japanese modx demo site is running by this fix code.

http://demo.modxcms-jp.com/

http://demo.modxcms-jp.com/manager/
user=modx
pass=modx

Sincerely yours,
soushi

634 Posts

Lammikko Reply #5, 16 years, 11 months ago

QuickManager+ 1.3.3 with security fix and some other changes too. Zip file and screenshots attached.

Tested with IE8, IE7, Firefox 3, Opera 10, Safari 4.

Changelog:
* Added: A fallback to English if the languages are missing in the Manager/user settings
* Added: Little tweaks with QuickManager action buttons: small hide/show animations and possibilty to close the menu by clicking the MODx logo
* Added: New option for custom buttons: modal = any link opened in a modal box (ColorBox)
* Changed: Custom buttons separator is now || instead of # (reserved for URL anchors)
* Changed: Rounded modal box is back, finally resolved bugs with ColorBox CSS in IE and friendly urls
* Changed: qm-colorbox.css renamed back to colorbox.css
* Fixed: Satinitized possible XSS vulnerabilities
* Removed: OnManagerPageInit plugin event removed
* Removed: IE6 support dropped, QuickManager+ is not tested with IE6 as MODx manager doesn’t offially support IE6

Vulnerable close.php file is changed to this:

<?php
// Get parameters
if (isset($_GET['id'])) $id = intval($_GET['id']);

print <<<HTML
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title></title>
<script type="text/javascript">
function getCookie(cookieName)
{
    var results = document.cookie.match ( "(^|;) ?" + cookieName + "=([^;]*)(;|$)" );

    if (results) return (unescape(results[2]));
    else return null;
}

function getUrl()
{
    var protocol = window.location.protocol;
    var host = window.location.host;
    var baseUrl = getCookie("baseUrlQM");
    
    return protocol + "//" + host + baseUrl + "index.php?id={$id}"; 
}

</script>

</head>
<body onload="javascript: parent.location.href = getUrl();">
</body>
</html>
HTML;
?>

Only $_GET parameter left is "id" which must be an int type: $id = intval($_GET[’id’]);. Can you find any possible security problems from this implementation?

Mikko Lammi, Owner at Maagit

634 Posts

Lammikko Reply #6, 16 years, 11 months ago

Quote from: soushi at Oct 06, 2009, 10:19 AM

Could you confirm whether there is problem?
By the way,Japanese modx demo site is running by this fix code.

I tried your hotfix on my own server and it worked without any problems.

Thank you very very much for addressing your time and energy to this problem!

Mikko Lammi, Owner at Maagit

164 Posts

soushi Reply #7, 16 years, 11 months ago

Dear Lammikko,

Quote from: Lammikko at Oct 07, 2009, 03:17 AM

Only $_GET parameter left is "id" which must be an int type: $id = intval($_GET[’id’]);. Can you find any possible security problems from this implementation?

I think this code have not problem.
Thank you for quick correspondence.

But,this fix cannot be used for default installing QM+ of modx1.0.
(Because, this QM+ version not use cookie.)
I will announce to Japan modx community that please apply my fix code to QM+ on modx1.0.
Or upgrade QM+.

Sincerely yours,
soushi

MODX Staff
12,272 Posts

Ryan Thrash Reply #8, 16 years, 11 months ago

Quote from: soushi at Oct 07, 2009, 09:11 AM

But,this fix cannot be used for default installing QM+ of modx1.0.
(Because, this QM+ version not use cookie.)
I will announce to Japan modx community that please apply my fix code to QM+ on modx1.0.
Or upgrade QM+.

Can you clarify why this fix will not work with the default MODx 1.0? Or you mean it is not included (yet) with the default installation?

Ryan Thrash, MODX Co-Founder
Follow me on Twitter at @rthrash or catch my occasional unofficial thoughts at thrash.me

164 Posts

soushi Reply #9, 16 years, 11 months ago

Dear rthrash

I am sorry for not transmitting the intention well because I am not good at English.

QM+ 1.3.3 work with the modx1.0.
I wanted to write, "Do not work when new close.php was replaced with QM+(included in default modx1.0)".
Therefore, I think that the patch to QM+ included in modx1.0 is necessary.
And, notify this information to user.

Sincerely yours,
soushi

MODX Staff
12,272 Posts

Ryan Thrash Reply #10, 16 years, 11 months ago

Thank you for the update.

Ryan Thrash, MODX Co-Founder
Follow me on Twitter at @rthrash or catch my occasional unofficial thoughts at thrash.me