From 7e7c4cf0a7daff517f37432fc9de4df34981ce16 Mon Sep 17 00:00:00 2001
From: Jared Hancock <jared@osticket.com>
Date: Wed, 8 Jan 2014 12:54:25 -0600
Subject: [PATCH] Fix possible serious errors in dynamic form fields
Fix dropping of materialized view when variable name is changed
Ensure view exists before merging updates
Prevent possible sql injection error in field name used in the materialized
view.
Prevent possible xss error in the display of the field label and variable
name in the admin panel.
---
include/ajax.tickets.php | 7 ++++---
include/class.dynamic_forms.php | 29 ++++++++++++++++++-----------
include/staff/dynamic-form.inc.php | 5 +++--
scp/forms.php | 2 ++
4 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/include/ajax.tickets.php b/include/ajax.tickets.php
index ceb824e2b..376e2a41d 100644
--- a/include/ajax.tickets.php
+++ b/include/ajax.tickets.php
@@ -211,10 +211,11 @@ class TicketsAjaxAPI extends AjaxController {
foreach (TicketForm::getInstance()->getFields() as $f) {
if (isset($req[$f->getFormName()])
&& ($val = $req[$f->getFormName()])) {
- $name = $f->get('name') ? $f->get('name') : 'field_'.$f->get('id');
- $cwhere = "cdata.`$name` LIKE '%".db_real_escape($val)."%'";
+ $name = $f->get('name') ? db_real_escape($f->get('name'))
+ : 'field_'.$f->get('id');
+ $cwhere = "cdata.\"$name\" LIKE '%".db_real_escape($val)."%'";
if ($f->getImpl()->hasIdValue() && is_numeric($val))
- $cwhere .= " OR cdata.`{$name}_id` = ".db_input($val);
+ $cwhere .= " OR cdata.\"{$name}_id\" = ".db_input($val);
$where .= ' AND ('.$cwhere.')';
$cdata_search = true;
}
diff --git a/include/class.dynamic_forms.php b/include/class.dynamic_forms.php
index ac1be4f3d..69e5a2e40 100644
--- a/include/class.dynamic_forms.php
+++ b/include/class.dynamic_forms.php
@@ -210,15 +210,15 @@ class TicketForm extends DynamicForm {
if (!$impl->hasData() || $impl->isPresentationOnly())
continue;
- $name = ($f->get('name')) ? $f->get('name')
+ $name = ($f->get('name')) ? db_real_escape($f->get('name'))
: 'field_'.$f->get('id');
$fields[] = sprintf(
- 'MAX(IF(field.name=\'%1$s\',ans.value,NULL)) as `%1$s`',
+ 'MAX(IF(field.name=\'%1$s\',ans.value,NULL)) as "%1$s"',
$name);
if ($impl->hasIdValue()) {
$fields[] = sprintf(
- 'MAX(IF(field.name=\'%1$s\',ans.value_id,NULL)) as `%1$s_id`',
+ 'MAX(IF(field.name=\'%1$s\',ans.value_id,NULL)) as "%1$s_id"',
$name);
}
}
@@ -261,21 +261,21 @@ class TicketForm extends DynamicForm {
if (!($e = $answer->getEntry()) || $e->get('object_type') != 'T')
return;
- // If the `name` column is in the dirty list, we would be renaming a
- // column. Delete the view instead.
- if (isset($data['dirty']) && isset($data['dirty']['name']))
- return self::dropDynamicDataView();
-
// $record = array();
// $record[$f] = $answer->value'
// TicketFormData::objects()->filter(array('ticket_id'=>$a))
// ->merge($record);
+ $sql = 'SHOW TABLES LIKE \''.TABLE_PREFIX.'ticket__cdata\'';
+ if (!db_num_rows(db_query($sql)))
+ return;
+
$f = $answer->getField();
- $name = $f->get('name') ? $f->get('name') : 'field_'.$f->get('id');
+ $name = $f->get('name') ? db_real_escape($f->get('name'))
+ : 'field_'.$f->get('id');
$ids = $f->hasIdValue();
- $fields = sprintf('`%s`=', $name) . db_input($answer->get('value'));
+ $fields = sprintf('"%s"=', $name) . db_input($answer->get('value'));
if ($f->hasIdValue())
- $fields .= sprintf(',`%s_id`=', $name) . db_input($answer->getIdValue());
+ $fields .= sprintf(',"%s_id"=', $name) . db_input($answer->getIdValue());
$sql = 'INSERT INTO `'.TABLE_PREFIX.'ticket__cdata` SET '.$fields
.', `ticket_id`='.db_input($answer->getEntry()->get('object_id'))
.' ON DUPLICATE KEY UPDATE '.$fields;
@@ -309,6 +309,13 @@ Signal::connect('model.deleted',
array('TicketForm', 'dropDynamicDataView'),
'DynamicFormField',
function($o) { return $o->getForm()->get('type') == 'T'; });
+// If the `name` column is in the dirty list, we would be renaming a
+// column. Delete the view instead.
+Signal::connect('model.updated',
+ array('TicketForm', 'dropDynamicDataView'),
+ 'DynamicFormField',
+ // TODO: Lookup the dynamic form to verify {type == 'T'}
+ function($o, $d) { return isset($d['dirty']) && isset($d['dirty']['name']); });
require_once(INCLUDE_DIR . "class.json.php");
diff --git a/include/staff/dynamic-form.inc.php b/include/staff/dynamic-form.inc.php
index a5019a722..a565e1be5 100644
--- a/include/staff/dynamic-form.inc.php
+++ b/include/staff/dynamic-form.inc.php
@@ -123,7 +123,7 @@ $info=Format::htmlchars(($errors && $_POST)?$_POST:$info);
<tr>
<td><i class="icon-sort"></i></td>
<td><input type="text" size="32" name="label-<?php echo $id; ?>"
- value="<?php echo $f->get('label'); ?>"/>
+ value="<?php echo Format::htmlchars($f->get('label')); ?>"/>
<font class="error"><?php
if ($ferrors['label']) echo '<br/>'; echo $ferrors['label']; ?>
</td>
@@ -161,7 +161,8 @@ $info=Format::htmlchars(($errors && $_POST)?$_POST:$info);
</td>
<td>
<input type="text" size="20" name="name-<?php echo $id; ?>"
- value="<?php echo $f->get('name'); ?>" <?php echo $force_name ?>/>
+ value="<?php echo Format::htmlchars($f->get('name'));
+ ?>" <?php echo $force_name ?>/>
<font class="error"><?php
if ($ferrors['name']) echo '<br/>'; echo $ferrors['name'];
?></font>
diff --git a/scp/forms.php b/scp/forms.php
index 6f14be6c1..077b3e0ff 100644
--- a/scp/forms.php
+++ b/scp/forms.php
@@ -43,6 +43,8 @@ if($_POST) {
}
if (in_array($field->get('name'), $names))
$field->addError('Field variable name is not unique', 'name');
+ if (preg_match('/[.{}\'"`; ]/u', $field->get('name')))
+ $field->addError('Invalid character in variable name. Please use letters and numbers only.', 'name');
if ($field->get('name'))
$names[] = $field->get('name');
if ($field->isValid())
--
GitLab