Security fixes: XSS and CSRF

craigk5n · Oct 14, 2021 · d9729b4 · d9729b4
1 parent a9720b9
commit d9729b4
Show file tree

Hide file tree

Showing 31 changed files with 241 additions and 61 deletions.
diff --git a/access.php b/access.php
@@ -175,8 +175,9 @@
     <h2>' . translate( 'User Access Control' )
    . ( empty( $user_fullname ) ? '' : ': ' . $user_fullname ) . '</h2>
     ' . display_admin_link( false ) . '
-    <form action="access.php" method="post" name="SelectUser">
-      <select name="guser" onchange="document.SelectUser.submit()">'
+    <form action="access.php" method="post" name="SelectUser">';
+  print_form_key();
+  echo '<select name="guser" onchange="document.SelectUser.submit()">'
   // Add a DEFAULT CONFIGURATION to be used as a mask.
   . '
         <option value="__default__"'
@@ -213,8 +214,9 @@
     // defined in access.php.
     assert( count( $order ) == ACCESS_NUMBER_FUNCTIONS );
 
+    echo '<form action="access.php" method="post" id="accessform" name="accessform">';
+    print_form_key();
     echo '
-    <form action="access.php" method="post" id="accessform" name="accessform">
       <input type="hidden" name="auser" value="' . $guser . '" />
       <input type="hidden" name="guser" value="' . $guser . '" />
       <table>
@@ -282,7 +284,9 @@
     $userlist = get_list_of_users( $guser );
     echo '
     <h2 style="margin-bottom: 2px;">' . $pagetitle . '</h2>
-    <form action="access.php" method="post" name="SelectOther">
+    <form action="access.php" method="post" name="SelectOther">';
+    print_form_key();
+    echo '
       <input type="hidden" name="guser" value="' . $guser . '" />
       <select name="otheruser" onchange="document.SelectOther.submit()">'
     // Add a DEFAULT CONFIGURATION to be used as a mask.
@@ -306,8 +310,9 @@
 if( ! empty( $otheruser ) ) {
   if( $allow_view_other ) {
     $typeStr = translate( 'Type' );
+    echo '<form action="access.php" method="post" name="EditOther">';
+    print_form_key();
     echo '
-    <form action="access.php" method="post" name="EditOther">
       <input type="hidden" name="guser" value="' . $guser . '" />
       <input type="hidden" name="otheruser" value="' . $otheruser . '" /><br />
       <table cellpadding="5">

diff --git a/admin.php b/admin.php
@@ -23,9 +23,12 @@ function save_pref ( $prefs, $src ) {
 
       // Validate key name. Should start with "admin_" and not include
       // any unusual characters that might be an SQL injection attack.
-      if ( ! preg_match ( '/admin_[A-Za-z0-9_]+$/', $key ) )
+      if ( $key == 'csrf_form_key' ) {
+        // Ignore this for validation...
+      } else if ( ! preg_match ( '/admin_[A-Za-z0-9_]+$/', $key ) ) {
         die_miserable_death ( str_replace ( 'XXX', $key,
             translate ( 'Invalid setting name XXX.' ) ) );
+     }
     } else {
       $prefix = 'admin_';
       $setting = $key;
@@ -238,7 +241,7 @@ function save_pref ( $prefs, $src ) {
    . '\'dependent,menubar,scrollbars,height=400,width=400,innerHeight=420,'
    . 'outerWidth=420\' );" /></h2>
     <form action="admin.php" method="post" onsubmit="return valid_form( this );"'
-   . ' name="prefform">'
+   . ' name="prefform">' . csrf_form_key()
    . display_admin_link() . '
       <input class="btn btn-primary" type="submit" value="' . $saveStr
    . '" name="" /><br /><br />

diff --git a/approve_entry.php b/approve_entry.php
@@ -19,8 +19,9 @@
   print_header();
   echo '
     <form action="approve_entry.php' . $q_string
-   . '" method="post" name="add_comments">
-      <table cellspacing="5">
+   . '" method="post" name="add_comments">';
+   print_form_key();
+   echo '<table cellspacing="5">
         <tr>
           <td class="aligncenter alignbottom"><h3>'
    . translate ( 'Additional Comments (optional)' ) . '</h3></td>

diff --git a/assistant_edit.php b/assistant_edit.php
@@ -14,7 +14,7 @@
   '<script type="text/javascript" src="includes/js/assistant_edit.js"></script>' );
 echo '
     <form action="assistant_edit_handler.php" method="post" '
- . 'name="assistanteditform">' . ( $user ? '
+ . 'name="assistanteditform">' . csrf_form_key() . ( $user ? '
       <input type="hidden" name="user" value="' . $user . '" />' : '' ) . '
       <h2>';
 

diff --git a/availability.php b/availability.php
@@ -74,7 +74,7 @@
       </div>
     </div><br />
     <form action="availability.php" method="post">
-      ' . daily_matrix ( $date, $users ) . '
+      ' . csrf_form_key() . daily_matrix ( $date, $users ) . '
     </form>
     ' . print_trailer ( false, true, true );
 

diff --git a/category.php b/category.php
@@ -66,7 +66,7 @@
 if ( ( ( $add == '1' ) || ( ! empty ( $id ) ) ) && empty ( $error ) ) {
   echo '
     <form action="category_handler.php" method="post" name="catform" '
-    . 'enctype="multipart/form-data">' . $idStr . '
+    . 'enctype="multipart/form-data">' . csrf_form_key() . $idStr . '
     <div class="form-inline">
     <label class="col-sm-3 col-form-label" for="catname">' . translate ('Category Name') . '</label>
     <input class="form-control" type="text" name="catname" size="20" value="'

diff --git a/catsel.php b/catsel.php
@@ -33,7 +33,7 @@
         <th colspan="3">' . translate ( 'Categories' ) . '</th>
       </tr>
       <form action="" method="post" name="editCategories" '
- . 'onSubmit="sendCats( this )">
+ . 'onSubmit="sendCats( this )">' . csrf_form_key() . '
       <tr>
         <td class="aligntop">';
 

diff --git a/docadd.php b/docadd.php
@@ -202,6 +202,7 @@
   // Comment
 ?>
 <form action="docadd.php" method="post" name="docform">
+<?php print_form_key(); ?>
 <input type="hidden" name="id" value="<?php echo $id?>" />
 <input type="hidden" name="type" value="C" />
 
@@ -223,6 +224,7 @@
   // Attachment
 ?>
 <form action="docadd.php" method="post" name="docform" enctype="multipart/form-data">
+<?php print_form_key(); ?>
 <input type="hidden" name="id" value="<?php echo $id?>" />
 <input type="hidden" name="type" value="A" />
 <table>

diff --git a/edit_entry.php b/edit_entry.php
@@ -101,7 +101,7 @@ function time_selection($prefix, $time = '', $trigger = false)
 $checked = ' checked="checked"';
 $selected = ' selected="selected"';
 
-$eType = getGetValue('eType');
+$eType = getGetValue('eType','event',true);
 $id    = getGetValue('id');
 
 $copy  = getValue('copy', '[01]');
@@ -567,6 +567,7 @@ function time_selection($prefix, $time = '', $trigger = false)
 
 ?>
 <form action="edit_entry_handler.php" method="post" name="editentryform" id="editentryform">
+  <?php print_form_key(); ?>
   <input type="hidden" name="eType" value="<?php echo $eType; ?>" />
   <?php if (!empty($id) && (empty($copy) || $copy != '1')) { ?>
     <input type="hidden" name="cal_id" value="<?php echo $id; ?>" />
@@ -1593,7 +1594,7 @@ function time_selection($prefix, $time = '', $trigger = false)
         <div class="col-auto">
           <input type="button" class="form-check btn btn-primary" value="<?php echo $saveStr; ?>" onclick="validate_and_submit()">
           <?php if ($id > 0 && ($login == $create_by || $single_user == 'Y' || $is_admin)) { ?>
-            <a class="btn btn-danger" href="del_entry.php?id=' <?php echo $id; ?>" onclick="return confirm('<?php etranslate('Are you sure you want to delete this entry?'); ?>');">
+            <a class="btn btn-danger" href="del_entry.php?id=<?php echo $id; ?>&csrf_form_key=<?php echo getFormKey();?>" onclick="return confirm('<?php etranslate('Are you sure you want to delete this entry?'); ?>');">
               <?php etranslate('Delete entry'); ?></a><br />
           <?php } ?>
         </div>

diff --git a/edit_entry_handler.php b/edit_entry_handler.php
@@ -1272,6 +1272,7 @@ function sort_byday( $a, $b ) {
     </ul>
     ' // User can confirm conflicts.
   . '<form name="confirm" method="post">';
+  print_form_key();
   foreach ($_POST as $xkey => $xval) {
     if (is_array($xval)) {
       $xkey .= "[]";

diff --git a/edit_remotes.php b/edit_remotes.php
@@ -79,10 +79,12 @@
   $urlValue = ( empty ( $remotestemp_url )
     ? '' : htmlspecialchars ( $remotestemp_url ) );
 
+  $formKey = csrf_form_key();
   echo <<<EOT
     <h2>{$lableStr}</h2>
     <form action="edit_remotes_handler.php" method="post" name="prefform"
       onsubmit="return valid_form( this );">
+      ${formKey}
       <table cellpadding="2">
         <tr>
           <td><label for="calid">{$calIdStr}:</label></td>

diff --git a/edit_report.php b/edit_report.php
@@ -207,6 +207,7 @@ function print_options ( $textarea, $option ) {
  . ( $adding_report ? translate ( 'Add Report' ) : translate ( 'Edit Report' ) )
  . '</h2>
     <form action="edit_report_handler.php" method="post" name="reportform">'
+ . csrf_form_key()
  . ( $updating_public ? '
       <input type="hidden" name="public" value="1" />' : '' )
  . ( ! $adding_report ? '

diff --git a/edit_template.php b/edit_template.php
@@ -124,7 +124,8 @@
 }
 
 echo '</h2>' . ( ! empty ( $error ) ? print_error ( $error ) : '
-    <form action="edit_template.php" method="post" name="reportform">
+    <form action="edit_template.php" method="post" name="reportform">'
+   . csrf_form_key() . '
       <input type="hidden" name="type" value="' . $type . '" />'
    . ( ! empty ( $ALLOW_USER_HEADER ) && $ALLOW_USER_HEADER == 'Y' && !
     empty ( $user ) && $user != '__system__' ? '

diff --git a/export.php b/export.php
@@ -24,6 +24,7 @@
 print_header ( array ( 'js/export_import.php', 'js/visible.php' ) );
 echo '<h2>' . translate ( 'Export' ) . '</h2>
     <form action="export_handler.php" method="post" name="exportform" id="exportform">
+      ' . print_form_key() . '
       <table class="table">
         <tr>
           <td><label for="exformat">' . translate ( 'Export format' )

diff --git a/groups.php b/groups.php
@@ -172,7 +172,8 @@ function load_groups() {
         groups = [];
         $('#group-tbody').html('<tr><td colspan="5"><?php echo $LOADING; ?></td></tr>');
         $.post('users_ajax.php', {
-                action: 'group-list'
+                action: 'group-list',
+                csrf_form_key: '<?php echo getFormKey(); ?>'
             },
             function(data, status) {
                 var stringified = JSON.stringify(data);
@@ -295,7 +296,8 @@ function save_handler() {
                     action: "save-group",
                     id: id,
                     name: name,
-                    users: users
+                    users: users,
+                    csrf_form_key: '<?php echo getFormKey(); ?>'
                 },
                 function(data, status) {
                     console.log('Data: ' + data);
@@ -357,7 +359,8 @@ function delete_handler() {
 
         $.post('users_ajax.php', {
                     action: "delete-group",
-                    id: id
+                    id: id,
+                    csrf_form_key: '<?php echo getFormKey(); ?>'
                 },
                 function(data, status) {
                     console.log('Data: ' + data);
@@ -395,4 +398,4 @@ function(data, status) {
     }
 </script>
 
-<?php echo print_trailer(); ?>
+<?php echo print_trailer(); ?>
diff --git a/import.php b/import.php
@@ -123,7 +123,8 @@ function print_categories() {
   $yesStr = translate ( 'Yes' );
   echo '
     <form action="import_handler.php" method="post" name="importform" '
-   . 'enctype="multipart/form-data" onsubmit="return checkExtension()">
+   . 'enctype="multipart/form-data" onsubmit="return checkExtension()">'
+   . print_form_key() . '
       <table class="table">
         <tr>
           <td><label for="importtype">' . translate ( 'Import format' ) . ':</label></td>

diff --git a/includes/formvars.php b/includes/formvars.php
@@ -22,12 +22,41 @@ function preventHacking_helper($matches) {
   return chr(hexdec($matches[1]));
 }
 function preventHacking ( $name, $instr ) {
+  global $PHP_SELF;
+  $script = basename($PHP_SELF);
+
   $bannedTags = [
     'APPLET', 'BODY', 'EMBED', 'FORM', 'HEAD',
     'HTML', 'IFRAME', 'LINK', 'META', 'NOEMBED',
     'NOFRAMES', 'NOSCRIPT', 'OBJECT', 'SCRIPT'];
   $failed = false;
 
+  // If this is a POST, require a form key to prevent CSRF
+  // Assume all database db changes make use of POST or else
+  // they end in "_handler.php" or are one of a handful of known URLs.
+  if ($script == "login.php" || $script=="register.php" ||
+    $script == "search_handler.php") {
+    // No form token needed
+  } else if ($_SERVER['REQUEST_METHOD'] === 'POST' ||
+    ($_SERVER['REQUEST_METHOD'] === 'GET' &&
+    ($script == 'del_entry.php' ||
+    $script == 'add_entry.php' || $script == 'docdel.php' ||
+    endsWith($script, "_handler.php")))) {
+//echo "KEY CHECK <br>\n";
+    $formKey = $_REQUEST['csrf_form_key'];
+    if ($formKey == $_SESSION['csrf_form_key'] && !empty($_SESSION['csrf_form_key'])) {
+      // Okay to proceed
+//echo "FORM KEY: $formKey \n"; exit;
+    } else {
+      die_miserable_death ( translate ( 'Fatal Error' ) . ': '
+         . translate ( 'Invalid form request' ) );
+    }
+  }
+  //echo "METHOD " . $_SERVER['REQUEST_METHOD'] . "<br>";
+  //echo "PHP_SELF " . $script . "<br>";
+  //print_r ( $_SERVER );
+  //echo "NO ERROR <br>\n"; exit;
+
   if ( is_array ( $instr ) ) {
     for ( $j = 0; $j < count ( $instr ); $j++ ) {
       // First, replace any escape characters like '\x3c'
@@ -41,7 +70,7 @@ function preventHacking ( $name, $instr ) {
     }
     if ( $failed ) {
       die_miserable_death ( translate ( 'Fatal Error' ) . ': '
-         . translate ( 'Invalid data format for' ) . ' ' . $name .
+         . translate ( 'Invalid data format for' ) . '&nbsp;' . $name .
          '<br>Value: ' . htmlspecialchars($instr));
     }
   } else {
@@ -62,6 +91,38 @@ function preventHacking ( $name, $instr ) {
   }
 }
 
+// Function to check the string is ends 
+// with given substring or not
+function endsWith($string, $endString)
+{
+  $len = strlen($endString);
+  if ($len == 0) {
+    return true;
+  }
+  return (substr($string, -$len) === $endString);
+}
+
+/**
+  * Generate a key to include in HTML form to prevent CSRF.
+  */
+function getFormKey() {
+  if (!isset($_SESSION['csrf_form_key'])) {
+    $formKey = bin2hex(openssl_random_pseudo_bytes(32));
+    $_SESSION['csrf_form_key'] = $formKey;
+  } else {
+    $formKey = $_SESSION['csrf_form_key'];
+  }
+  return $formKey;
+}
+
+function csrf_form_key() {
+  return '<input type="hidden" name="csrf_form_key" value="' .
+    getFormKey() . '" />' .  "\n";
+}
+function print_form_key() {
+  echo csrf_form_key ();
+}
+
 
 /**
  * Gets the value resulting from an HTTP POST method.
@@ -103,15 +164,16 @@ function getPostValue($name, $defVal = NULL, $chkXSS = false)
  *
  * @see getPostValue
  */
-function getGetValue($name)
+function getGetValue($name, $devVal=NULL, $chkCSS=false)
 {
   $getName = null;
   if (isset($_GET) && is_array($_GET) && isset($_GET[$name])) {
     $getName = is_array($_GET[$name]) ? array_map('addslashes', $_GET[$name]) :
       addslashes($_GET[$name]);
   }
+  $cleanXSS = $chkXSS ? chkXSS($getName) : true;
   preventHacking($name, $getName);
-  return $getName;
+  return $cleanXSS ? $postName : NULL;
 }
 
 /**
@@ -190,9 +252,7 @@ function chkXSS($name) {
   global $login;
   $cleanXSS = true;
     //add more array elements as needed
-    foreach (array(
-'Ajax.Request',
- 'onerror') as $i) {
+    foreach (array( 'Ajax.Request', 'onerror') as $i) {
       if (preg_match("/$i/i", $name)) {
         activity_log(0, $login, $login, SECURITY_VIOLATION,
                 'Hijack attempt:' . $i);