Ajax search progrem(SQL injection)

164 Posts

soushi Reply #1, 18 years, 7 months ago

Dear all,

When I investigated the vulnerabilities about MODx announced the other day, another problem was found.

http://seclists.org/bugtraq/2008/Feb/0068.html

"Item 3.1 XSS(AjaxSearch)" in above-mentioned URL.
Not only XSS but also the problem of "SQL Injection" contains the vulnerabilities.

For instance, the following sample codes execute AjaxSearch including the article that cannot be retrieved.

<form method="post" name="test" action="MODx-SITE/index-ajax.php">
  <input type="hidden" name="q" value="assets/snippets/AjaxSearch/AjaxSearch.php" />
  <input type="hidden" name="search" value="modx" />
  <input type="hidden" name="maxResults" value="6" />
  <input type="hidden" name="stripHtml" value="1" />
  <input type="hidden" name="stripSnip" value="1" />
  <input type="hidden" name="stripSnippets" value="1" />
  <input type="hidden" name="useAllWords" value="0" />
  <input type="hidden" name="searchStyle" value="partial" />
  <input type="hidden" name="minChars" value="4" />
  <input type="hidden" name="showMoreResults" value="1" />
  <input type="hidden" name="moreResultsPage" value="8" />
  <input type="hidden" name="as_language" value="japanese-utf8" />
  <input type="hidden" name="extract" value="0" />
  <input type="hidden" name="highlightResult" value="1" />
  <!-- progrem code -->
  <input type="hidden" name="docgrp" value="'')) -- " />

  <input type="submit" value="Login" name="cmdweblogin" class="button" />
</form>

<input type="hidden" name="docgrp" value="’’)) -- " />

SQL sentence in the back is invalidated by burying comment out strings.

I don’t know ,how much potential problem is hidden in this vulnerabilities.
I think that the patch should be offered.

Sincerely yours,

1,717 Posts

coroico Reply #2, 18 years, 7 months ago

Hi Soushi,

the $docgrp variable incriminated in http://seclists.org/bugtraq/2008/Feb/0068.html is used as follow in the dosearch function of the includes/ajaxSearch.inc.php:

  if ($docgrp) {
		$tbl_sql = " LEFT JOIN $tbl_stc stc ON sc.id = stc.contentid LEFT JOIN $tbl_dg dg ON sc.id = dg.document";
    $qry_sql .= " (ISNULL(dg.document_group) OR dg.document_group IN ({$docgrp})) AND ";
  } else {
    $tbl_sql = " LEFT JOIN $tbl_stc stc ON sc.id = stc.contentid ";
    $qry_sql .= " sc.privateweb = 0 AND ";
  }

So, as $docgrp is used in a IN statement and with $docgrp enclosed by parentheses, I don’t know how this "where" clause statement could be hacked.
Nevertheless, for security, i suggest to strip unwanted characters from the $docgrp variable by a specific strip_id function called before the use in the select statement:

function strip_id($text){

    $mReg = '~([^0-9,]*)~';   // 

    $text = preg_replace($mReg,'',$text);
    return $text;
}
?>

I will deliver this improvement in a new version of ajaxSearch : 1.7.0.2 version

1,717 Posts

coroico Reply #3, 18 years, 7 months ago

AjaxSearch version 1.7.0.2 has been delivered in the repository. This version includes the following security patches :

includes/ajaxSearch.inc.php - line 134 added to avoid XSS and sql injection:

$docgrp = mysql_real_escape_string($docgrp); // to avoid sql injection and XSS

The plugin searchHighlight.tpl has been updated too to avoid too XSS as follow (strip_tags function added):

  $searched = strip_tags(urldecode($_GET['searched']));
  $highlight = strip_tags(urldecode($_GET['highlight']));

892 Posts

MEGU Reply #4, 18 years, 7 months ago

Dear coroico,

I read about plugin searchHighlight.tp here.
http://modxcms.com/forums/index.php/topic,22621.msg140214.html#msg140214

Such the following:

$searched = strip_tags(urldecode($_REQUEST['searched'])); 
$highlight = strip_tags(urldecode($_REQUEST['highlight']));

Which should I select? "$_REQUEST" or "$_GET"?

Sincerely yours,
MEGU

164 Posts

soushi Reply #5, 18 years, 7 months ago

Dear all,

Thank you coroico.
"SQL injection" can be prevented by this fix.
However, SQL error still occurs because of the input of illegal data.
(SQL sentence is displayed on the screen.)
I think that you should fix as much as possible.

For instance:

if ( checkDocgrp($docgrp) ) {
		$tbl_sql = " LEFT JOIN $tbl_stc stc ON sc.id = stc.contentid LEFT JOIN $tbl_dg dg ON sc.id = dg.document";
    $qry_sql .= " (ISNULL(dg.document_group) OR dg.document_group IN ({$docgrp})) AND ";
  } else {
    $tbl_sql = " LEFT JOIN $tbl_stc stc ON sc.id = stc.contentid ";
    $qry_sql .= " sc.privateweb = 0 AND ";
  }

//-------------------------
function checkDocgrp($docgrp){
  if( preg_match('/^([0-9]+,)*[0-9]+$/',$docgrp) == 0 )
    return false;

  return true;
}

Sincerely yours,

1,717 Posts

coroico Reply #6, 18 years, 7 months ago

Hi Soushi,

What do you mean by:

However, SQL error still occurs because of the input of illegal data.
(SQL sentence is displayed on the screen.)

How do you input illegal value for docgrp ? thru a specific post form ?
In ajaxSearch $docgrp is get thru the value of $modx->getUserDocGroups()

1,717 Posts

coroico Reply #7, 18 years, 7 months ago

@Megu

Which should I select? "$_REQUEST" or "$_GET"?

$_GET is correct and runs with AjaxSearch. But as suggested by Sottwell and PixelChutes $_REQUEST (which is an array concatenation of $_GET, $_POST and $_COOKIE ) will deal with any incoming values, just in case.

So, with this change ($_REQUEST instead of $_GET), I have upgraded the version of the file plugin.highlighting.tpl (now 1.2.0.2) delivered with the ajaxSearch.zip file in the repository.

Now the detailed version of ajaxSearch and of the highligth plugin are displayed on the in the updates section of the snippet presentation
Thanks for your relevant feedback

164 Posts

soushi Reply #8, 18 years, 7 months ago

Hi coroico,

Quote from: coroico at Feb 16, 2008, 12:01 PM

What do you mean by:

However, SQL error still occurs because of the input of illegal data.
(SQL sentence is displayed on the screen.)
How do you input illegal value for docgrp ? thru a specific post form ?
In ajaxSearch $docgrp is get thru the value of $modx->getUserDocGroups()

I think that GET/POST data cannot be trusted.
Because the GET/POST data can change freely on the client browser.
GET/POST might be counterfeited to possibly attack site.

I do not think that SQL error should be displayed as much as possible for a
attacking user.
(It might become the hint of capture.)

I think , this is a thing that can be said to not only AjaxSearch but also the entire MODx.
I am wishing that MODx becomes more secure.

Sincerely yours,

1,717 Posts

coroico Reply #9, 18 years, 7 months ago

Souchi, you have convinced me.
AjaxSearch 1.7.0.2, has been modified as follow:

  if (validListIDs($listIDs)) $qry_sql = "sc.id IN ({$listIDs}) AND ";
  
  if (validListIDs($docgrp)) {
		$tbl_sql = " LEFT JOIN $tbl_stc stc ON sc.id = stc.contentid LEFT JOIN $tbl_dg dg ON sc.id = dg.document";
    $qry_sql .= " (ISNULL(dg.document_group) OR dg.document_group IN ({$docgrp})) AND ";
  } else {
    $tbl_sql = " LEFT JOIN $tbl_stc stc ON sc.id = stc.contentid ";
    $qry_sql .= " sc.privateweb = 0 AND ";
  }

/**
 *	validListIDs : check the validity of a value separated list of Ids
 */
function validListIDs($IDs){
  if (preg_match('/^([0-9]+,)*[0-9]+$/',$IDs) == 0) return false;
  return true;
}

$listIDs (subset of documents) and $docgrp are now checking before using in the sql statement.
Thanks for your contribution and your feedback.

892 Posts

MEGU Reply #10, 18 years, 7 months ago

Dear coroico,

I confirmed plugin.highlighting.tpl (1.2.0.2) included in AjaxSearch.zip.
http://modxcms.com/AjaxSearch-1.7-1801.html

Thank you for answering to me!

Sincerely yours,
MEGU