From a0999cb48adeb3ac8f139cfcccfe35dbf35515d8 Mon Sep 17 00:00:00 2001
From: Jared Hancock <jared@osticket.com>
Date: Wed, 11 Dec 2013 15:21:41 -0600
Subject: [PATCH] forms: Fixup possible XSS exploit
Displaying field values on various pages and dialogs could result in cross
site scripting exploits.
Fixes osTicket/osTicket-1.8#296
---
include/ajax.tickets.php | 10 ++++++----
include/ajax.users.php | 1 +
include/class.dynamic_forms.php | 4 ++++
include/class.forms.php | 15 +++++++++++++++
include/client/header.inc.php | 2 +-
include/client/view.inc.php | 4 ++--
include/staff/templates/user-lookup.tmpl.php | 4 ++--
include/staff/templates/user.tmpl.php | 6 +++---
include/staff/ticket-edit.inc.php | 2 +-
include/staff/ticket-view.inc.php | 9 +++++----
include/staff/tickets.inc.php | 2 +-
11 files changed, 41 insertions(+), 18 deletions(-)
diff --git a/include/ajax.tickets.php b/include/ajax.tickets.php
index b795aafc7..b8e98b8c8 100644
--- a/include/ajax.tickets.php
+++ b/include/ajax.tickets.php
@@ -463,8 +463,9 @@ class TicketsAjaxAPI extends AjaxController {
$info = array(
- 'title' => sprintf('Ticket #%s: %s', $ticket->getNumber(), $user->getName())
- );
+ 'title' => sprintf('Ticket #%s: %s', $ticket->getNumber(),
+ Format::htmlchars($user->getName()))
+ );
ob_start();
include(STAFFINC_DIR . 'templates/user.tmpl.php');
@@ -491,8 +492,9 @@ class TicketsAjaxAPI extends AjaxController {
$forms = $user->getForms();
$info = array(
- 'title' => sprintf('Ticket #%s: %s', $ticket->getNumber(), $user->getName())
- );
+ 'title' => sprintf('Ticket #%s: %s', $ticket->getNumber(),
+ Format::htmlchars($user->getName()))
+ );
ob_start();
include(STAFFINC_DIR . 'templates/user.tmpl.php');
diff --git a/include/ajax.users.php b/include/ajax.users.php
index f84fbd7d8..c98c1b94d 100644
--- a/include/ajax.users.php
+++ b/include/ajax.users.php
@@ -45,6 +45,7 @@ class UsersAjaxAPI extends AjaxController {
if(($res=db_query($sql)) && db_num_rows($res)){
while(list($id,$email,$name)=db_fetch_row($res)) {
+ $name = Format::htmlchars($name);
$users[] = array('email'=>$email, 'name'=>$name, 'info'=>"$email - $name",
"id" => $id, "/bin/true" => $_REQUEST['q']);
}
diff --git a/include/class.dynamic_forms.php b/include/class.dynamic_forms.php
index 21324cbbd..52e310935 100644
--- a/include/class.dynamic_forms.php
+++ b/include/class.dynamic_forms.php
@@ -621,6 +621,10 @@ class DynamicFormEntryAnswer extends VerySimpleModel {
return $this->getField()->toString($this->getValue());
}
+ function display() {
+ return $this->getField()->display($this->getValue());
+ }
+
function asVar() {
return $this->toString();
}
diff --git a/include/class.forms.php b/include/class.forms.php
index 0654a2648..8a876388a 100644
--- a/include/class.forms.php
+++ b/include/class.forms.php
@@ -287,6 +287,13 @@ class FormField {
return $value;
}
+ /**
+ * Returns an HTML friendly value for the data in the field.
+ */
+ function display($value) {
+ return Format::htmlchars($this->toString($value));
+ }
+
function getLabel() { return $this->get('label'); }
/**
@@ -516,6 +523,14 @@ class TextareaField extends FormField {
'configuration'=>array('desc'=>'Allow HTML input in this box'))),
);
}
+
+ function display($value) {
+ $config = $this->getConfiguration();
+ if ($config['html'])
+ return Format::safe_html($value);
+ else
+ return Format::htmlchars($value);
+ }
}
class PhoneField extends FormField {
diff --git a/include/client/header.inc.php b/include/client/header.inc.php
index e03f36cbb..7266060fb 100644
--- a/include/client/header.inc.php
+++ b/include/client/header.inc.php
@@ -43,7 +43,7 @@ header("Content-Type: text/html; charset=UTF-8\r\n");
<p>
<?php
if($thisclient && is_object($thisclient) && $thisclient->isValid()) {
- echo $thisclient->getName().' - ';
+ echo Format::htmlchars($thisclient->getName()).' - ';
?>
<?php
if($cfg->showRelatedTickets()) {?>
diff --git a/include/client/view.inc.php b/include/client/view.inc.php
index f7884302e..910d021de 100644
--- a/include/client/view.inc.php
+++ b/include/client/view.inc.php
@@ -39,7 +39,7 @@ if(!$dept || !$dept->isPublic())
<table class="infoTable" cellspacing="1" cellpadding="3" width="100%" border="0">
<tr>
<th width="100">Name:</th>
- <td><?php echo ucfirst($ticket->getName()); ?></td>
+ <td><?php echo ucfirst(Format::htmlchars($ticket->getName())); ?></td>
</tr>
<tr>
<th width="100">Email:</th>
@@ -70,7 +70,7 @@ foreach (DynamicFormEntry::forTicket($ticket->getId()) as $idx=>$form) {
<tr>
<th width="100"><?php echo $answer->getField()->get('label');
?>:</th>
- <td><?php echo $answer->toString(); ?></td>
+ <td><?php echo $answer->display(); ?></td>
</tr>
<?php } ?>
</table></td>
diff --git a/include/staff/templates/user-lookup.tmpl.php b/include/staff/templates/user-lookup.tmpl.php
index 74508a023..c2d8951a5 100644
--- a/include/staff/templates/user-lookup.tmpl.php
+++ b/include/staff/templates/user-lookup.tmpl.php
@@ -16,7 +16,7 @@ if ($info['error']) {
<i class="icon-user icon-4x pull-left icon-border"></i>
<a class="action-button pull-right" style="overflow:inherit"
id="unselect-user" href="#"><i class="icon-remove"></i> Add New User</a>
- <div><strong id="user-name"><?php echo $user ? $user->getName() : ''; ?></strong></div>
+ <div><strong id="user-name"><?php echo $user ? Format::htmlchars($user->getName()) : ''; ?></strong></div>
<div><<span id="user-email"><?php echo $user ? $user->getEmail() : ''; ?></span>></div>
<?php if ($user) { ?>
<table style="margin-top: 1em;">
@@ -26,7 +26,7 @@ if ($info['error']) {
<?php foreach ($entry->getAnswers() as $a) { ?>
<tr style="vertical-align:top"><td style="width:30%;border-bottom: 1px dotted #ccc"><?php echo Format::htmlchars($a->getField()->get('label'));
?>:</td>
- <td style="border-bottom: 1px dotted #ccc"><?php echo $a->toString(); ?></td>
+ <td style="border-bottom: 1px dotted #ccc"><?php echo $a->display(); ?></td>
</tr>
<?php }
}
diff --git a/include/staff/templates/user.tmpl.php b/include/staff/templates/user.tmpl.php
index aaf0cdedb..887f8e9ca 100644
--- a/include/staff/templates/user.tmpl.php
+++ b/include/staff/templates/user.tmpl.php
@@ -1,6 +1,6 @@
<?php
if (!$info['title'])
- $info['title'] = $user->getName();
+ $info['title'] = Format::htmlchars($user->getName());
?>
<h3><?php echo $info['title']; ?></h3>
<b><a class="close" href="#"><i class="icon-remove-circle"></i></a></b>
@@ -20,7 +20,7 @@ if ($info['error']) {
<?php
} ?>
<div><b><a href="#" id="edituser"><i class="icon-edit"></i> <?php
- echo $user->getName(); ?></a></b></div>
+ echo Format::htmlchars($user->getName()); ?></a></b></div>
<div><<?php echo $user->getEmail(); ?>></div>
<table style="margin-top: 1em;">
<?php foreach ($user->getDynamicData() as $entry) {
@@ -30,7 +30,7 @@ if ($info['error']) {
<?php foreach ($entry->getAnswers() as $a) { ?>
<tr style="vertical-align:top"><td style="width:30%;border-bottom: 1px dotted #ccc"><?php echo Format::htmlchars($a->getField()->get('label'));
?>:</td>
- <td style="border-bottom: 1px dotted #ccc"><?php echo $a->toString(); ?></td>
+ <td style="border-bottom: 1px dotted #ccc"><?php echo $a->display(); ?></td>
</tr>
<?php }
}
diff --git a/include/staff/ticket-edit.inc.php b/include/staff/ticket-edit.inc.php
index 0fe5c8ee7..cddb2ee3c 100644
--- a/include/staff/ticket-edit.inc.php
+++ b/include/staff/ticket-edit.inc.php
@@ -33,7 +33,7 @@ if ($_POST)
});
return false;
"><i class="icon-user"></i>
- <span id="client-name"><?php echo $user->getName(); ?></span>
+ <span id="client-name"><?php echo Format::htmlchars($user->getName()); ?></span>
<<span id="client-email"><?php echo $user->getEmail(); ?></span>>
</a>
<a class="action-button" style="float:none;overflow:inherit" href="#"
diff --git a/include/staff/ticket-view.inc.php b/include/staff/ticket-view.inc.php
index 1891bc07f..8e8953015 100644
--- a/include/staff/ticket-view.inc.php
+++ b/include/staff/ticket-view.inc.php
@@ -299,13 +299,13 @@ foreach (DynamicFormEntry::forTicket($ticket->getId()) as $form) {
<td colspan="2">
<table cellspacing="0" cellpadding="4" width="100%" border="0">
<?php foreach($answers as $a) {
- if (!$a->toString()) continue; ?>
+ if (!($v = $a->display())) continue; ?>
<tr>
<th width="100"><?php
echo $a->getField()->get('label');
?>:</th>
<td><?php
- echo $a->toString();
+ echo $v;
?></td>
</tr>
<?php } ?>
@@ -422,7 +422,8 @@ $tcount+= $ticket->getNumNotes();
<td>
<?php
echo sprintf('<span id="user-to-name">%s</span> <em><<span id="user-to-email">%s</span>></em>',
- $ticket->getName(), $ticket->getReplyToEmail());
+ Format::htmlChars($ticket->getName()),
+ $ticket->getReplyToEmail());
?>
<label><input type='checkbox' value='1' name="emailreply" id="remailreply"
@@ -894,7 +895,7 @@ $tcount+= $ticket->getNumNotes();
</p>
<p class="confirm-action" style="display:none;" id="changeuser-confirm">
<p id="msg_warning">
- <b><?php echo $ticket->getName(); ?></b> <<?php echo $ticket->getEmail(); ?>> will no longer have access to the ticket.
+ <b><?php echo Format::htmlchars($ticket->getName()); ?></b> <<?php echo $ticket->getEmail(); ?>> will no longer have access to the ticket.
</p>
Are you sure want to <b>change</b> ticket owner to <b><span id="newuser">this guy</span></b>?
</p>
diff --git a/include/staff/tickets.inc.php b/include/staff/tickets.inc.php
index 16fc4e1e2..e463e515d 100644
--- a/include/staff/tickets.inc.php
+++ b/include/staff/tickets.inc.php
@@ -367,7 +367,7 @@ $negorder=$order=='DESC'?'ASC':'DESC'; //Negate the sorting..
$lc=Format::truncate($row['dept_name'],40);
}
$tid=$row['ticketID'];
- $subject = Format::truncate($row['subject'],40);
+ $subject = Format::htmlchars(Format::truncate($row['subject'],40));
$threadcount=$row['thread_count'];
if(!strcasecmp($row['status'],'open') && !$row['isanswered'] && !$row['lock_id']) {
$tid=sprintf('<b>%s</b>',$tid);
--
GitLab