From 92824412a92dbac7cbc0b06e1d53acbd98c7ecbc Mon Sep 17 00:00:00 2001
From: Peter Rotich <peter@osticket.com>
Date: Thu, 17 Jul 2014 15:18:38 +0000
Subject: [PATCH] Fix XSS vulnerability on user's name

Names parsed from incoming emails are stored in the database as is. This
pull request addresses potential XSS vulnerability due to improper display
of unsanitized names. Going forward names will be scrubbed on create.
---
 include/class.format.php               | 4 ++--
 include/class.user.php                 | 8 ++++----
 include/staff/templates/users.tmpl.php | 6 ++++--
 include/staff/ticket-view.inc.php      | 4 +++-
 include/staff/tickets.inc.php          | 3 ++-
 include/staff/users.inc.php            | 3 ++-
 scp/tickets.php                        | 2 +-
 7 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/include/class.format.php b/include/class.format.php
index 4c73d5188..654a9c4e2 100644
--- a/include/class.format.php
+++ b/include/class.format.php
@@ -274,8 +274,8 @@ class Format {
             $flags |= ENT_HTML401;
 
         return is_array($var)
-            ? array_map(array('Format','htmlencode'), $var)
-            : htmlentities($var, $flags, 'UTF-8');
+            ? array_map(array('Format', 'htmlencode'), $var)
+            : htmlentities( (string) $var, $flags, 'UTF-8', false);
     }
 
     function htmldecode($var) {
diff --git a/include/class.user.php b/include/class.user.php
index b137d49ee..d2e2e9e57 100644
--- a/include/class.user.php
+++ b/include/class.user.php
@@ -157,9 +157,9 @@ class User extends UserModel {
                 list($name) = explode('@', $vars['email'], 2);
 
             $user = User::create(array(
-                'name'=>$name,
-                'created'=>new SqlFunction('NOW'),
-                'updated'=>new SqlFunction('NOW'),
+                'name' => Format::sanitize($name, false),
+                'created' => new SqlFunction('NOW'),
+                'updated' => new SqlFunction('NOW'),
                 //XXX: Do plain create once the cause
                 // of the detached emails is fixed.
                 'default_email' => UserEmail::ensure($vars['email'])
@@ -247,7 +247,7 @@ class User extends UserModel {
 
         $info = array(
                 'id'  => $this->getId(),
-                'name' => (string) $this->getName(),
+                'name' => Format::htmlchars($this->getName()),
                 'email' => (string) $this->getEmail(),
                 'phone' => (string) $this->getPhoneNumber());
 
diff --git a/include/staff/templates/users.tmpl.php b/include/staff/templates/users.tmpl.php
index 42ad7e6f1..6bff557b2 100644
--- a/include/staff/templates/users.tmpl.php
+++ b/include/staff/templates/users.tmpl.php
@@ -97,7 +97,9 @@ if ($num) { ?>
                     value="<?php echo $row['id']; ?>" <?php echo $sel?'checked="checked"':''; ?> >
                 </td>
                 <td>&nbsp;
-                    <a class="userPreview" href="users.php?id=<?php echo $row['id']; ?>"><?php echo $name; ?></a>
+                    <a class="userPreview"
+                        href="users.php?id=<?php echo $row['id']; ?>"><?php
+                        echo Format::htmlchars($name); ?></a>
                     &nbsp;
                     <?php
                     if ($row['tickets'])
@@ -105,7 +107,7 @@ if ($num) { ?>
                              <small>(%d)</small>', $row['tickets']);
                     ?>
                 </td>
-                <td><?php echo $row['email']; ?></td>
+                <td><?php echo Format::htmlchars($row['email']); ?></td>
                 <td><?php echo $status; ?></td>
                 <td><?php echo Format::db_date($row['created']); ?></td>
                </tr>
diff --git a/include/staff/ticket-view.inc.php b/include/staff/ticket-view.inc.php
index a63df79ea..bfef5605e 100644
--- a/include/staff/ticket-view.inc.php
+++ b/include/staff/ticket-view.inc.php
@@ -452,7 +452,9 @@ $tcount+= $ticket->getNumNotes();
                 <td>
                     <?php
                     # XXX: Add user-to-name and user-to-email HTML ID#s
-                    $to =sprintf('%s &lt;%s&gt;', $ticket->getName(), $ticket->getReplyToEmail());
+                    $to =sprintf('%s &lt;%s&gt;',
+                            Format::htmlchars($ticket->getName()),
+                            $ticket->getReplyToEmail());
                     $emailReply = (!isset($info['emailreply']) || $info['emailreply']);
                     ?>
                     <select id="emailreply" name="emailreply">
diff --git a/include/staff/tickets.inc.php b/include/staff/tickets.inc.php
index ce2728dd2..83bb13f05 100644
--- a/include/staff/tickets.inc.php
+++ b/include/staff/tickets.inc.php
@@ -424,7 +424,8 @@ if ($results) {
                             echo '<i class="icon-fixed-width icon-paperclip"></i>&nbsp;';
                     ?>
                 </td>
-                <td nowrap>&nbsp;<?php echo Format::truncate($row['name'],22,strpos($row['name'],'@')); ?>&nbsp;</td>
+                <td nowrap>&nbsp;<?php echo Format::htmlchars(
+                        Format::truncate($row['name'], 22, strpos($row['name'], '@'))); ?>&nbsp;</td>
                 <?php
                 if($search && !$status){
                     $displaystatus=ucfirst($row['status']);
diff --git a/include/staff/users.inc.php b/include/staff/users.inc.php
index 91362ae32..222cceb37 100644
--- a/include/staff/users.inc.php
+++ b/include/staff/users.inc.php
@@ -139,7 +139,8 @@ else
                 ?>
                <tr id="<?php echo $row['id']; ?>">
                 <td>&nbsp;
-                    <a class="userPreview" href="users.php?id=<?php echo $row['id']; ?>"><?php echo $name; ?></a>
+                    <a class="userPreview" href="users.php?id=<?php echo $row['id']; ?>"><?php
+                        echo Format::htmlchars($name); ?></a>
                     &nbsp;
                     <?php
                     if ($row['tickets'])
diff --git a/scp/tickets.php b/scp/tickets.php
index 16b9d4cde..1dda6f0fd 100644
--- a/scp/tickets.php
+++ b/scp/tickets.php
@@ -357,7 +357,7 @@ if($_POST && !$errors):
                     } elseif (!$_POST['user_id'] || !($user=User::lookup($_POST['user_id']))) {
                         $errors['err'] = 'Unknown user selected!';
                     } elseif ($ticket->changeOwner($user)) {
-                        $msg = 'Ticket ownership changed to '.$user->getName();
+                        $msg = 'Ticket ownership changed to ' . Format::htmlchars($user->getName());
                     } else {
                         $errors['err'] = 'Unable to change tiket ownership. Try again';
                     }
-- 
GitLab