XOOPS
							*****
Informations :
°°°°°°°°°°°°°°
Langage : PHP
Website : http://www.xoops.org
Version : 1.3.X,2.0.X  ->  2.0.5
Problèmes : 
- Changement des urls des bannières
- Injection SQL autour des bannières
- Redéfinitions de variables locales

Developpement :
°°°°°°°°°°°°°°°
"XOOPS est un portal open source dynamique orienté objet écrit en PHP." Il contient de nombreux
modules tel qu'un forum, un espace membre, des sondages, des messages privés, un gestionnaire de news,...

Les premières failles touchent les bannières, donc via le fichier banners.php. Il contient des problèmes de sécurité.
Voici la partie du code buggée :
-----------------------------------------------------------------------------------------------------------------------------
[...]

function EmailStats($login, $cid, $bid, $pass)
{
	global $xoopsDB, $xoopsConfig;
	$result2 = $xoopsDB->query("select name, email from ".$xoopsDB->prefix("bannerclient")." where cid=$cid");
    	list($name, $email) = $xoopsDB->fetchRow($result2);
 	if ( $email == "" ) {
		redirect_header("banners.php",3,"There isn't an email associated with client ".$name.".<br />Please contact the Administrator");

		exit();
	} else {
		$result = $xoopsDB->query("select bid, imptotal, impmade, clicks, imageurl, clickurl, date from ".$xoopsDB->prefix("banner")." where bid=$bid and cid=$cid");
		list($bid, $imptotal, $impmade, $clicks, $imageurl, $clickurl, $date) = $xoopsDB->fetchRow($result);
	[...]

		$fecha = date("F jS Y, h:iA.");
		$subject = "Your Banner Statistics at ".$xoopsConfig[sitename]."";
		$message = "Following are the complete stats for your advertising investment at ". $xoopsConfig['sitename']." :\n\n\nClient Name: $name\nBanner ID: $bid\nBanner Image: $imageurl\nBanner URL: $clickurl\n\nImpressions Purchased: $imptotal\nImpressions Made: $impmade\nImpressions Left: $left\nClicks Received: $clicks\nClicks Percent: $percent%\n\n\nReport Generated on: $fecha";
		$xoopsMailer =& getMailer();
		$xoopsMailer->useMail();
		$xoopsMailer->setToEmails($email);
		$xoopsMailer->setFromEmail($xoopsConfig['adminmail']);
		$xoopsMailer->setFromName($xoopsConfig['sitename']);
		$xoopsMailer->setSubject($subject);
		$xoopsMailer->setBody($message);
		$xoopsMailer->send();
		redirect_header("banners.php?op=Ok&amp;login=$login&amp;pass=$pass",3,"Statistics for your banner has been sent to your email address.");
		//include "footer.php";
		exit();
	}
}

function change_banner_url_by_client($login, $pass, $cid, $bid, $url)
{
	global $xoopsDB;
    $result = $xoopsDB->query("select passwd from ".$xoopsDB->prefix("bannerclient")." where cid=".$cid."");
    list($passwd) = $xoopsDB->fetchRow($result);
    if ( $pass == $passwd ) {
		$xoopsDB->queryF("update ".$xoopsDB->prefix("banner")." set clickurl='".$url."' where bid=".$bid."");
    }
	redirect_header("banners.php?op=Ok&amp;login=$login&amp;pass=$pass",3,"URL has been changed.");
	//include "footer.php";
	exit();
}

[...]

switch ( $op ) {
case "Change":
	change_banner_url_by_client($login, $pass, $cid, $bid, $url);
	break;
case "EmailStats":
	EmailStats($login, $cid, $bid, $pass);
	break;
[...]
}

?>
-----------------------------------------------------------------------------------------------------------------------------
On voit d'abord dans la fonction EmailStats() que la requête sql :
select name, email from ".$xoopsDB->prefix("bannerclient")." where cid=$cid
est exécutée.
On peut trouver la structure de la table "bannerclient" dans le fichier install/sql/mysql.structure.sql :
----------------------------------------------------
CREATE TABLE bannerclient (
  cid smallint(5) unsigned NOT NULL auto_increment,
  name varchar(60) NOT NULL default '',
  contact varchar(60) NOT NULL default '',
  email varchar(60) NOT NULL default '',
  login varchar(10) NOT NULL default '',
  passwd varchar(10) NOT NULL default '',
  extrainfo text NOT NULL,
  PRIMARY KEY  (cid),
  KEY login (login)
) TYPE=MyISAM;
----------------------------------------------------
Le problème est que si on donne par exemple à $cid la valeur : 1 AND passwd LIKE 'a%'/*
alors la requête SQL exécutée deviendra :
select name,email from bannerclient where cid=1 AND passwd LIKE 'a%'/*
Dans ce cas, si le mot de passe de l'utilisateur dont l'id est 1 commence par "a", on verra s'afficher le message :
"Statistics for your banner has been sent to your email address."
sinon le message :
"There isn't an email associated with client .<br />Please contact the Administrator"
On peut de cette manière retrouver tout le mot de passe, login, etc...
Exemple d'url :
http://[target]/banners.php?op=EmailStats&cid=1%20AND%20passwd%20LIKE%20'a%'/*
Il est aussi possible d'utiliser INTO OUTFILE/DUMPFILE pour enregistrer tout les résultats de la requête dans un fichier.
Et UNION.

La deuxième fonction, change_banner_url_by_client() peut permettre à n'importe qui de changer l'url de n'importe quel
bannière. On y voit le code :
--------------------------------------------------------------------------------------------------------------
$result = $xoopsDB->query("select passwd from ".$xoopsDB->prefix("bannerclient")." where cid=".$cid."");
list($passwd) = $xoopsDB->fetchRow($result);
if ( $pass == $passwd ) {
	$xoopsDB->queryF("update ".$xoopsDB->prefix("banner")." set clickurl='".$url."' where bid=".$bid."");
}
redirect_header("banners.php?op=Ok&amp;login=$login&amp;pass=$pass",3,"URL has been changed.");
exit();
--------------------------------------------------------------------------------------------------------------
Ici tout d'abord on pourrait encore une fois enregistrer tout les mots de passe dans un fichier ou retrouver
des informations via un LIKE avec la variable $cid dans la requête :
select passwd from ".$xoopsDB->prefix("bannerclient")." where cid=".$cid
Mais il y a un problème de logique plus interessant.
En effet imaginons que $cid valle -1, un ID non existant. Dans ce cas, la variable $passwd, qui est le résultat
de la requête recherchant le cid -1, aura une valeur NULLE.
Ensuite vient la vérification du mot de passe :
-----------------------
if ( $pass == $passwd )
-----------------------
Si on ne donne aucune valeur à $pass, $passwd étant lui aussi NULL, l'égalité renverra vrai, car $pass et $passwd sont
nuls tout les deux.
Enfin vient la requête :
------------------------------------------------------------------------------
update ".$xoopsDB->prefix("banner")." set clickurl='".$url."' where bid=".$bid
------------------------------------------------------------------------------
Permettant de changer l'url de redirection d'une bannière en fonction du $bid, ce qui ne pose pas de problème vu 
qu'on ne l'a pas encore définit.
Ainsi pour changer l'url de la bannière dont le bid est 100, il suffira de taper l'url :
http://[target]/banners.php?op=Change&cid=-1&bid=100&url=HTTP://WWW.NEWURL.COM



L'autre faille se trouve dans edituser.php et imagemanager.php. Le fichier edituser.php commence comme ceci :
------------------------------------------------------------
<?php
[...]
$xoopsOption['pagetype'] = 'user';
include 'mainfile.php';
include_once XOOPS_ROOT_PATH.'/class/xoopsformloader.php';

// If not a user, redirect
if ( !$xoopsUser ) {
	redirect_header('index.php',3,_US_NOEDITRIGHT);
	exit();
}

// initialize $op variable
$op = 'editprofile';

if (isset($HTTP_POST_VARS)) {
	foreach ($HTTP_POST_VARS as $k => $v) {
		${$k} =  $v;
	}
}
[...]
------------------------------------------------------------
Le code qui provoque la faille sont les dernières lignes :
------------------------------------------------
if (isset($HTTP_POST_VARS)) {
	foreach ($HTTP_POST_VARS as $k => $v) {
		${$k} =  $v;
	}
}
------------------------------------------------
Il transforme toutes les variables POST en variables locales.
Il permet donc de REDEFINIR des variables déjà définies, comme des variables contenant des classes, ou des variables 
de configuration (il semble qu'il y ait un filtre pour les xoopsConfig[] mais les xoopsOption[] passent sans problèmes).
Un exemple à priori sans conséquences (je dis bien à priori) :
Dans edituser.php se trouvent les lignes :
---------------------------------------------------
[...]
if ($op == 'editprofile') {
	include_once XOOPS_ROOT_PATH.'/header.php';
[...]
---------------------------------------------------
et dans header.php on voit les lignes :
-----------------------------------------------------------------------------------------------------------------------------
[...]
if (!isset($xoopsOption['template_main'])) {
	$xoopsCachedTemplate = 'db:system_dummy.html';
} else {
	$xoopsCachedTemplate = 'db:'.$xoopsOption['template_main'];
}
// generate safe cache Id
$xoopsCachedTemplateId = 'mod_'.$xoopsModule->getVar('dirname').'|'.md5(str_replace(XOOPS_URL, '', $GLOBALS['xoopsRequestUri']));
if ($xoopsTpl->is_cached($xoopsCachedTemplate, $xoopsCachedTemplateId)) {
	$xoopsLogger->addExtra($xoopsCachedTemplate, $xoopsConfig['module_cache'][$xoopsModule->getVar('mid')]);
	$xoopsTpl->assign('xoops_contents', $xoopsTpl->fetch($xoopsCachedTemplate, $xoopsCachedTemplateId));
	$xoopsTpl->xoops_setCaching(0);
[...]
-----------------------------------------------------------------------------------------------------------------------------
Donc si
- On s'inscrit sur un site XOOPS
- On affiche la page permettant de changer son profile
- On ajoute à se formulaire la ligne :
<input type="text" name="xoopsOption[template_main]" value="system_userinfo.html">
- On envois le formulaire normalement
alors la variable locale xoopsOption['template_main'] déjà définie se verra attribuer une nouvelle valeur :
system_userinfo.html, et c'est le fichier /modules/system/templates/system_userinfo.html qui sera
affiché après l'exécution du script. Si on avait choisi comme valeur system_block_search.html ça aurait été le fichier
/modules/system/templates/block/system_block_search.html qui serait affiché, etc...

Le même problème se trouve dans imagemanager.php, dont le début du code est :
----------------------------------------------------------------------------
<?php
[...]
include './mainfile.php';
if (!isset($HTTP_GET_VARS['target']) && !isset($HTTP_POST_VARS['target'])) {
	exit();
}
$op = 'list';
if (isset($HTTP_GET_VARS['op']) && $HTTP_GET_VARS['op'] == 'upload') {
	$op = 'upload';
}
if (isset($HTTP_POST_VARS)) {
	foreach ($HTTP_POST_VARS as $k => $v) {
		${$k} = $v;
	}
}
[...]
----------------------------------------------------------------------------



Solution :
°°°°°°°°°°
Un patch est disponible sur http://www.phpsecure.info.
Le créateur (Onokazu) a été avertit et a publié une version 2.0.5.1 sécurisée.

Dans banners.php, remplacer la fonction change_banner_url_by_client() par :
-----------------------------------------------------------------------------------------------------------------------------
function change_banner_url_by_client($login, $pass, $cid, $bid, $url)
{
	global $xoopsDB;
	if ( !empty($cid) AND !empty($bid) AND !empty($pass) ){
		$result = $xoopsDB->query("select passwd from ".$xoopsDB->prefix("bannerclient")." where cid='".$cid."'");
		list($passwd) = $xoopsDB->fetchRow($result);
		if ( $pass == $passwd ) {
			$xoopsDB->queryF("update ".$xoopsDB->prefix("banner")." set clickurl='".$url."' where bid='".$bid."'");
		}
		redirect_header("banners.php?op=Ok&amp;login=$login&amp;pass=$pass",3,"URL has been changed.");
		//include "footer.php";
	}
	exit();
}
-----------------------------------------------------------------------------------------------------------------------------

et ajouter, juste avant la ligne :
----------------
switch ( $op ) {
----------------
les lignes :
--------------------
$cid = intval($cid);
$bid = intval($bid);
--------------------

Enfin dans edituser.php et imagemanager.php, remplacer le code :
-----------------------------------------------
if (isset($HTTP_POST_VARS)) {
	foreach ($HTTP_POST_VARS as $k => $v) {
		${$k} = $v;
	}
}
-----------------------------------------------
par :
-----------------------------------------------------------------------------------------------------------------------------
$forbidden = array('forbidden','sess_handler','member_handler','config_handler','xoopsUserIsAdmin','xoopsUser','xoopsDB','xoopsLogger','xoopsConfig','XoopsOption');
if (isset($HTTP_POST_VARS)) {
	foreach ($HTTP_POST_VARS as $k => $v) {
		if ( !in_array($k,$forbidden) ){
			${$k} =  $v;
		}
	}
}
-----------------------------------------------------------------------------------------------------------------------------
$forbidden contient des noms de variables sensibles, qui ne doivent pas pouvoir être redéfinies.
Ceci est une solution qui est loin d'être sûre à 100 % mais il faudrait changer tout le code et ce n'est pas mon rôle.
On peut alors ajouter facilement d'autres variables à interdire.

Credits :
°°°°°°°°°
Auteur : frog-m@n
E-mail : leseulfrog@hotmail.com
Website : http://www.phpsecure.info
Date : 22/11/03

Merci à Magistrat (http://www.blocus-zone.com).