Skip to content

Commit

Permalink
Merge pull request #1608 from fquffio/fix/v3/sanitize-relation-params
Browse files Browse the repository at this point in the history 
Avoid potential SQL injection when updating relations with params
  • Loading branch information
@batopa
batopa committed Jun 6, 2019
2 parents d7739dc + 85b0da4 commit 0ddcd46
Show file tree
Hide file tree
Showing 3 changed files with 247 additions and 105 deletions.
191 changes: 92 additions & 99 deletions bedita-app/models/object_relation.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,30 @@ public function afterFind($results) {
return $results;
}

/**
* Escape all arguments that have been passed to this function.
*
* @param any[] ...$values Values to be prepared.
* @return string[]
*/
private static function prepareAll() {
$values = func_get_args();
foreach ($values as &$val) {
if ($val === null) {
$val = 'NULL';
continue;
}

if (!is_scalar($val)) {
$val = json_encode($val);
}
$val = sprintf('\'%s\'', Sanitize::escape($val));
}
unset($val);

return $values;
}

/**
* Create relation between objects
*
Expand All @@ -61,22 +85,19 @@ public function afterFind($results) {
*/
public function createRelation($id, $objectId, $switch, $priority, $bidirectional = true, $params = array()) {
// #CUSTOM QUERY - TODO: use cake, how??
$jParams = $params === null ? 'NULL' : sprintf("'%s'", json_encode($params));
$q = "INSERT INTO object_relations (id, object_id, switch, priority, params) VALUES ({$id}, {$objectId}, '{$switch}', {$priority}, {$jParams})";
list($qId, $qObjectId, $qSwitch, $qPriority, $qParams) = static::prepareAll($id, $objectId, $switch, $priority, $params);
$q = "INSERT INTO object_relations (id, object_id, switch, priority, params) VALUES ({$qId}, {$qObjectId}, {$qSwitch}, {$qPriority}, {$qParams})";
$res = $this->query($q);
if ($res === false) {
return $res;
}
if (!$bidirectional) {
ClassRegistry::init('BEObject')->clearCacheByIds(array($id, $objectId));
return $res;
}

$q = "INSERT INTO object_relations (id, object_id, switch, priority, params) VALUES ({$objectId}, {$id}, '{$switch}', {$priority}, {$jParams})";
$res = $this->query($q);

if ($res === false) {
return $res;
if ($bidirectional) {
$q = "INSERT INTO object_relations (id, object_id, switch, priority, params) VALUES ({$qObjectId}, {$qId}, {$qSwitch}, {$qPriority}, {$qParams})";
$res = $this->query($q);

if ($res === false) {
return $res;
}
}

ClassRegistry::init('BEObject')->clearCacheByIds(array($id, $objectId));
Expand All @@ -96,35 +117,35 @@ public function createRelation($id, $objectId, $switch, $priority, $bidirectiona
* @return array|false
*/
public function createRelationAndInverse($id, $objectId, $switch, $inverseSwitch = null, $priority = null, $params = array()) {
if ($inverseSwitch == null) {
$inverseSwitch = $switch;
}

list($qId, $qObjectId, $qSwitch, $qInverseSwitch, $qPriority, $qParams) = static::prepareAll($id, $objectId, $switch, $inverseSwitch, $priority, $params);
if ($priority == null) {
$rel = $this->query("SELECT MAX(priority)+1 AS priority FROM object_relations WHERE id={$id} AND switch='{$switch}'");
$priority = (empty($rel[0][0]["priority"]))? 1 : $rel[0][0]["priority"];
$rel = $this->query("SELECT MAX(priority)+1 AS priority FROM object_relations WHERE id={$qId} AND switch={$qSwitch}");
$qPriority = $priority = (empty($rel[0][0]["priority"]))? 1 : $rel[0][0]["priority"];
}

// #CUSTOM QUERY
$jParams = $params === null ? 'NULL' : sprintf("'%s'", json_encode($params));
$q = "INSERT INTO object_relations (id, object_id, switch, priority, params) VALUES ({$id}, {$objectId}, '{$switch}', {$priority}, {$jParams})";
$q = "INSERT INTO object_relations (id, object_id, switch, priority, params) VALUES ({$qId}, {$qObjectId}, {$qSwitch}, {$qPriority}, {$qParams})";
$res = $this->query($q);
if ($res === false) {
return $res;
}

if ($inverseSwitch == null) {
$inverseSwitch = $switch;
}

$inverseRel = $this->query("SELECT priority FROM object_relations WHERE id={$objectId}
AND object_id={$id} AND switch='{$inverseSwitch}'");

$inverseRel = $this->query("SELECT priority FROM object_relations WHERE id={$qObjectId}
AND object_id={$qId} AND switch={$qInverseSwitch}");
if (empty($inverseRel[0]["object_relations"]["priority"])) {
// #CUSTOM QUERY
$inverseRel = $this->query("SELECT MAX(priority)+1 AS priority FROM object_relations WHERE id={$objectId} AND switch='{$inverseSwitch}'");
$inversePriority = (empty($inverseRel[0][0]["priority"]))? 1 : $inverseRel[0][0]["priority"];
$inverseRel = $this->query("SELECT MAX(priority)+1 AS priority FROM object_relations WHERE id={$qObjectId} AND switch={$qInverseSwitch}");
$qInversePriority = (empty($inverseRel[0][0]["priority"])) ? 1 : $inverseRel[0][0]["priority"];
} else {
$inversePriority = $inverseRel[0]["object_relations"]["priority"];
$qInversePriority = $inverseRel[0]["object_relations"]["priority"];
}

// #CUSTOM QUERY
$q= "INSERT INTO object_relations (id, object_id, switch, priority, params) VALUES ({$objectId}, {$id}, '{$inverseSwitch}', {$inversePriority}, {$jParams})" ;
$q= "INSERT INTO object_relations (id, object_id, switch, priority, params) VALUES ({$qObjectId}, {$qId}, {$qInverseSwitch}, {$qInversePriority}, {$qParams})" ;
$res = $this->query($q);

if ($res === false) {
Expand All @@ -147,17 +168,18 @@ public function createRelationAndInverse($id, $objectId, $switch, $inverseSwitch
*/
public function deleteRelation($id, $objectId=null, $switch=null, $bidirectional = true) {
// #CUSTOM QUERY - TODO: use cake, how?? changing table structure (id primary key, object_id, related_object_id, switch, priority)
list($qId, $qObjectId, $qSwitch) = static::prepareAll($id, $objectId, $switch);
$clearObjects = array($id);
$q = "DELETE FROM object_relations WHERE id={$id}";
$qReverse = "DELETE FROM object_relations WHERE object_id={$id}";
$q = "DELETE FROM object_relations WHERE id={$qId}";
$qReverse = "DELETE FROM object_relations WHERE object_id={$qId}";
if ($objectId !== null) {
$q .= " AND object_id={$objectId}";
$qReverse .= " AND id={$objectId}";
$q .= " AND object_id={$qObjectId}";
$qReverse .= " AND id={$qObjectId}";
$clearObjects[] = $objectId;
}
if ($switch !== null) {
$q .= " AND switch='{$switch}'";
$qReverse .= " AND switch='{$switch}'";
$q .= " AND switch={$qSwitch}";
$qReverse .= " AND switch={$qSwitch}";
}
$res = $this->query($q);
if ($res === false) {
Expand Down Expand Up @@ -190,24 +212,23 @@ public function deleteRelation($id, $objectId=null, $switch=null, $bidirectional
*/
public function deleteRelationAndInverse($id, $objectId = null, $switch = null) {
// #CUSTOM QUERY - TODO: use cake, how?? changing table structure (id primary key, object_id, related_object_id, switch, priority)
$id = Sanitize::escape($id);
list($qId, $qObjectId, $qSwitch) = static::prepareAll($id, $objectId, $switch);
$clearObjects = array($id);
$q = "DELETE FROM object_relations WHERE id={$id}";
$qReverse = "DELETE FROM object_relations WHERE object_id={$id}";
$q = "DELETE FROM object_relations WHERE id={$qId}";
$qReverse = "DELETE FROM object_relations WHERE object_id={$qId}";
if ($objectId !== null) {
$objectId = Sanitize::escape($objectId);
$q .= " AND object_id={$objectId}";
$qReverse .= " AND id={$objectId}";
$q .= " AND object_id={$qObjectId}";
$qReverse .= " AND id={$qObjectId}";
$clearObjects[] = $objectId;
}
if ($switch !== null) {
$switch = Sanitize::escape($switch);
$inverseSwitch = $this->inverseOf($switch);
if (empty($inverseSwitch)) {
return false;
}
$q .= " AND switch='{$switch}'";
$qReverse .= " AND switch='{$inverseSwitch}'";
list($qInverseSwitch) = static::prepareAll($inverseSwitch);
$q .= " AND switch={$qSwitch}";
$qReverse .= " AND switch={$qInverseSwitch}";
}
$res = $this->query($q);
if ($res === false) {
Expand All @@ -233,17 +254,18 @@ public function deleteRelationAndInverse($id, $objectId = null, $switch = null)
* @return bool
*/
public function deleteObjectRelation($id, $switch, $inverseSwitch = null) {
if (empty($inverseSwitch)) {
$inverseSwitch = $switch;
}
// #CUSTOM QUERY - TODO: use cake, how??
$q = "DELETE FROM object_relations WHERE id={$id} AND switch='{$switch}'";
list($qId, $qSwitch, $qInverseSwitch) = static::prepareAll($id, $switch, $inverseSwitch);
$q = "DELETE FROM object_relations WHERE id={$qId} AND switch={$qSwitch}";
$res = $this->query($q);
if ($res === false) {
$this->log('Error executing query: ' . $q, 'error');
return $res;
}
if (empty($inverseSwitch)) {
$inverseSwitch = $switch;
}
$qReverse = "DELETE FROM object_relations WHERE object_id={$id} AND switch='{$inverseSwitch}'";
$qReverse = "DELETE FROM object_relations WHERE object_id={$qId} AND switch={$qInverseSwitch}";
$res = $this->query($qReverse);
if ($res === false) {
$this->log('Error executing query: ' . $qReverse, 'error');
Expand All @@ -264,18 +286,8 @@ public function deleteObjectRelation($id, $switch, $inverseSwitch = null) {
* @param int $priority
* @return false on failure
*/
public function updateRelationPriority($id, $objectId, $switch, $priority){
$q = " UPDATE object_relations
SET priority={$priority}
WHERE id={$id} AND object_id={$objectId} AND switch='{$switch}'";
$res = $this->query($q);
if ($res === false) {
return $res;
}

ClassRegistry::init('BEObject')->clearCacheByIds(array($id, $objectId));

return $res;
public function updateRelationPriority($id, $objectId, $switch, $priority) {
return $this->updateRelation($id, $objectId, $switch, compact('priority'));
}

/**
Expand All @@ -288,18 +300,7 @@ public function updateRelationPriority($id, $objectId, $switch, $priority){
* @return false on failure
*/
public function updateRelationParams($id, $objectId, $switch, $params=array()) {
$jParams = $params === null ? 'NULL' : sprintf("'%s'", json_encode($params));
$q = " UPDATE object_relations
SET params={$jParams}
WHERE ((id={$id} AND object_id={$objectId}) OR (id={$objectId} AND object_id={$id})) AND switch='{$switch}'";
$res = $this->query($q);
if ($res === false) {
return $res;
}

ClassRegistry::init('BEObject')->clearCacheByIds(array($id, $objectId));

return $res;
return $this->updateRelation($id, $objectId, $switch, compact('params'));
}

/**
Expand All @@ -319,42 +320,31 @@ public function updateRelation($id, $objectId, $switch, array $set) {
}
$updateData = array();
if (array_key_exists('params', $set)) {
if ($set['params'] === null) {
$updateData[] = "params=NULL";
} else {
if (!is_array($set['params'])) {
return false;
}
$set['params'] = json_encode($set['params']);
$updateData[] = "params='{$set['params']}'";
}
list($qParams) = static::prepareAll($set['params']);
$updateData[] = sprintf('params=%s', $qParams);
}
if (array_key_exists('priority', $set)) {
if ($set['priority'] === null) {
$updateData[] = "priority=NULL";
} else {
$set['priority'] = Sanitize::escape($set['priority']);
$updateData[] = "priority='{$set['priority']}'";
}
list($qPriority) = static::prepareAll($set['priority']);
$updateData[] = sprintf('priority=%s', $qPriority);
}

if (empty($updateData)) {
return false;
}

$q = 'UPDATE object_relations SET ';
foreach ($updateData as $key => $value) {
$q .= ($key == 0) ? $value : ', ' . $value;
}
$q .= " WHERE id={$id} AND object_id={$objectId} AND switch='{$switch}'";
list($qId, $qObjectId, $qSwitch) = static::prepareAll($id, $objectId, $switch);
$updateData = implode(', ', $updateData);

$q = "UPDATE object_relations SET {$updateData} WHERE id={$qId} AND object_id={$qObjectId} AND switch={$qSwitch}";
$result = $this->query($q);

// update params in inverse relation
if ($result !== false && array_key_exists('params', $set)) {
$switchInverse = $this->inverseOf($switch);
list($qSwitchInverse, $qParams) = static::prepareAll($switchInverse, $set['params']);
$q = "UPDATE object_relations
SET params='{$set['params']}'
WHERE id={$objectId} AND object_id={$id} AND switch='{$switchInverse}'";
SET params={$qParams}
WHERE id={$qObjectId} AND object_id={$qId} AND switch={$qSwitchInverse}";
$result = $this->query($q);
}

Expand All @@ -375,8 +365,9 @@ public function updateRelation($id, $objectId, $switch, array $set) {
* @return true if relation exists, false otherwise
*/
public function relationExists($id, $objectId, $switch) {
$actualId = $this->query("SELECT id FROM object_relations WHERE id={$id}
AND object_id={$objectId} AND switch='{$switch}'");
list($qId, $qObjectId, $qSwitch) = static::prepareAll($id, $objectId, $switch);
$actualId = $this->query("SELECT id FROM object_relations WHERE id={$qId}
AND object_id={$qObjectId} AND switch={$qSwitch}", false);
if (empty($actualId[0]['object_relations']['id'])) {
return false;
}
Expand All @@ -391,8 +382,9 @@ public function relationExists($id, $objectId, $switch) {
* @return priority value or false if field is NULL or relation missing
*/
public function relationPriority($id, $objectId, $switch) {
$pri = $this->query("SELECT priority FROM object_relations WHERE id={$id}
AND object_id={$objectId} AND switch='{$switch}'");
list($qId, $qObjectId, $qSwitch) = static::prepareAll($id, $objectId, $switch);
$pri = $this->query("SELECT * FROM object_relations WHERE id={$qId}
AND object_id={$qObjectId} AND switch={$qSwitch}", false);
if(empty($pri[0]["object_relations"]["priority"])) {
return false;
}
Expand All @@ -408,8 +400,9 @@ public function relationPriority($id, $objectId, $switch) {
* @return parameters as an associative array or object, or false if field is NULL or relation missing
*/
public function relationParams($id, $objectId, $switch, $assoc=true) {
$pri = $this->query("SELECT params FROM object_relations WHERE id={$id}
AND object_id={$objectId} AND switch='{$switch}'");
list($qId, $qObjectId, $qSwitch) = static::prepareAll($id, $objectId, $switch);
$pri = $this->query("SELECT params FROM object_relations WHERE id={$qId}
AND object_id={$qObjectId} AND switch={$qSwitch}", false);
if(empty($pri[0]["object_relations"]["params"])) {
return false;
}
Expand Down
14 changes: 8 additions & 6 deletions bedita-app/models/objects/b_e_object.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -392,20 +392,22 @@ function afterSave() {
foreach ($this->data['BEObject']['RelatedObject'] as $switch => $values) {

foreach ($values as $key => $val) {
$obj_id = isset($val['id'])? $val['id'] : false;
$priority = isset($val['priority'])? "'{$val['priority']}'" : 'NULL';
$params = isset($val['params'])? "'" . json_encode($val['params']) . "'" : 'NULL';
// Delete old associations
// #CUSTOM QUERY
$queriesDelete[] = "DELETE FROM {$table} WHERE {$assoc['foreignKey']} = '{$this->id}' AND switch = '{$switch}' ";
$obj_id = isset($val['id'])? Sanitize::escape($val['id']) : false;
$priority = isset($val['priority'])? Sanitize::escape($val['priority']) : 'NULL';
$params = isset($val['params'])? "'" . Sanitize::escape(json_encode($val['params'])) . "'" : 'NULL';

$inverseSwitch = $switch;
if (!empty($allRelations[$switch]) && !empty($allRelations[$switch]["inverse"])) {
$inverseSwitch = $allRelations[$switch]["inverse"];
} elseif (!empty($inverseRelations[$switch])) {
$inverseSwitch = $inverseRelations[$switch];
}
$switch = Sanitize::escape($switch);
$inverseSwitch = Sanitize::escape($inverseSwitch);

// Delete old associations
// #CUSTOM QUERY
$queriesDelete[] = "DELETE FROM {$table} WHERE {$assoc['foreignKey']} = '{$this->id}' AND switch = '{$switch}' ";
$queriesDelete[] = "DELETE FROM {$table} WHERE {$assoc['associationForeignKey']} = '{$this->id}'
AND switch = '{$inverseSwitch}' ";

Expand Down

0 comments on commit 0ddcd46

Please sign in to comment.