Hi netnoise, here is one of the thing that I think need to be added to avoid mysql injection using document id.
/**
* name: getDocumentObject - used by parser
* desc: returns a document object - $method: alias, id
*/
function getDocumentObject($method,$identifier){
$tblsc = $this->getFullTableName("site_content");
$tbldg = $this->getFullTableName("document_groups");
// Check document id validity (To avoid sql injection) - Added by Wendy Novianto
if($method == 'id' && (!is_numeric($identifier) || $identifier != ((int)$identifier))) {
$this->sendErrorPage();
exit; // stop here
}
// get document groups for current user
if($docgrp = $this->getUserDocGroups()) $docgrp = implode(",",$docgrp);
// get document
$access = ($this->isFrontend() ? "sc.privateweb=0":"1='".$_SESSION['mgrRole']."' OR sc.privatemgr=0").
(!$docgrp ? "":" OR dg.document_group IN ($docgrp)");
$sql = "SELECT sc.*
FROM $tblsc sc
LEFT JOIN $tbldg dg ON dg.document = sc.id
WHERE sc.".$method." = '".$identifier."'
AND ($access) LIMIT 1;";
$result = $this->db->query($sql);
$rowCount = $this->recordCount($result);
if($rowCount<1) {
if ($this->config['unauthorized_page']) {
// check if file is not public
$secrs = $this->dbQuery("SELECT id FROM $tbldg WHERE document = '".$identifier."' LIMIT 1;");
if($secrs) $seclimit = mysql_num_rows($secrs);
}
if ($seclimit>0) {
// match found but not publicly accessible, send the visitor to the unauthorized_page
$this->sendUnauthorizedPage();
exit; // stop here
}
else {
// no match found, send the visitor to the error_page
$this->sendErrorPage();
exit; // stop here
}
}
if($rowCount>1) {
// too many matches found, send the visitor to the error page
$this->messageQuit("More than one result returned when attempting to translate `alias` to `id` - there are multiple documents using the same alias");
}
# this is now the document :) #
$documentObject = $this->fetchRow($result);
// load TVs and merge with document - Orig by Apodigm - Docvars
$tbn = $this->dbConfig['dbase'].".".$this->dbConfig['table_prefix'];
$sql = "SELECT tv.*, IF(tvc.value!='',tvc.value,tv.default_text) as value ";
$sql.= "FROM ".$tbn."site_tmplvars tv ";
$sql.= "INNER JOIN ".$tbn."site_tmplvar_templates tvtpl ON tvtpl.tmplvarid = tv.id ";
$sql.= "LEFT JOIN ".$tbn."site_tmplvar_contentvalues tvc ON tvc.tmplvarid=tv.id AND tvc.contentid = '".$this->documentIdentifier."' ";
$sql.= "WHERE tvtpl.templateid = '".$documentObject['template']."'";
$rs = $this->dbQuery($sql);
$rowCount = $this->recordCount($rs);
if($rowCount>0) {
for($i=0;$i<$rowCount;$i++) {
$row = $this->fetchRow($rs);
$tmplvars[$row['name']] = array($row['name'],$row['value'],$row['display'],$row['display_params'],$row['type']);
}
$documentObject = array_merge($documentObject,$tmplvars);
}
return $documentObject;
}
I only added 3 lines of code to make sure if document method is id, the doc id being passed needs to be integer.
I found this problem when helping someone on the forum, hope you can apply it for next release, considering I have no access to the svn. Hope you fix the other security problem as well netnoise.
EDIT: I mistyped == to =