Various file upload issues (#2225)

* Fixed issue: [security] Possible XSS in file upload question type * Fixed issue: In file upload question type any file with non-alphanumeric multi-byte characters may not properly show * Fixed issue: File upload question type preview not working * Fixed issue: File upload question type popup not showing correct title * Fixed issue: [security] Possible XSS in file upload question type * Fixed issue: Title & comment not shown for file upload question when navigating back and forth * Fixed issue #17718: In file upload question type any file with non-alphanumeric multi-byte characters may not properly show * Fixed issue #17718: In file upload question type any file with non-alphanumeric multi-byte characters may not properly show * Dev Removed basename because it is not needed anymore * Dev Better error message if file is not found or inaccessible for download * Dev Tiny CSS fix * Dev Fixed top line when listing uploaded question in responses * DEv Minified file * Dev Removed obsolete script load * Dev Minor display fixes
LimeSurvey · Feb 2, 2022 · aa00a0a · Shnoulle · May 9, 2022 · aa00a0a
1 parent 1b3e1a1
commit aa00a0a
Show file tree

Hide file tree

Showing 13 changed files with 50 additions and 43 deletions.
diff --git a/application/controllers/ResponsesController.php b/application/controllers/ResponsesController.php
@@ -629,11 +629,13 @@ public function actionDownloadfile(int $surveyId, int $responseId, int $qid, int
             if (isset($aQuestionFiles[$index])) {
                 $aFile = $aQuestionFiles[$index];
                 // Real path check from here: https://stackoverflow.com/questions/4205141/preventing-directory-traversal-in-php-but-allowing-paths
-                $sDir = Yii::app()->getConfig('uploaddir') . DIRECTORY_SEPARATOR . "surveys" . DIRECTORY_SEPARATOR . $surveyId . DIRECTORY_SEPARATOR."files" . DIRECTORY_SEPARATOR;
+                $sDir = Yii::app()->getConfig('uploaddir') . DIRECTORY_SEPARATOR . "surveys" . DIRECTORY_SEPARATOR . $surveyId . DIRECTORY_SEPARATOR . "files" . DIRECTORY_SEPARATOR;
                 $sFileRealName = $sDir . $aFile['filename'];
                 $sRealUserPath = realpath($sFileRealName);
-                if ($sRealUserPath === false || strpos($sRealUserPath, $sDir) !== 0) {
-                    throw new CHttpException(403, "Disable for security reasons.");
+                if ($sRealUserPath === false) {
+                    throw new CHttpException(404, "File not found.");
+                } elseif (strpos($sRealUserPath, $sDir) !== 0) {
+                        throw new CHttpException(403, "File cannot be accessed.");
                 } else {
                     $mimeType = CFileHelper::getMimeType($sFileRealName, null, false);
                     if (is_null($mimeType)) {

diff --git a/application/controllers/UploaderController.php b/application/controllers/UploaderController.php
@@ -140,9 +140,7 @@ public function run($actionID)
                 // Try to create
                 mkdir($sTempUploadDir);
             }
-            $filename = $_FILES['uploadfile']['name'];
-            // Do we filter file name ? It's used on displaying only , but not save like that.
-            //$filename = sanitize_filename($_FILES['uploadfile']['name']);// This remove all non alpha numeric characters and replaced by _ . Leave only one dot .
+            $filename = sanitize_filename($_FILES['uploadfile']['name'], false, false, true);
             $size = $_FILES['uploadfile']['size'] / 1024;
             $preview = Yii::app()->session['preview'];
             $aFieldMap = createFieldMap($oSurvey, 'short', false, false, $sLanguage);
@@ -272,12 +270,12 @@ public function run($actionID)
                 }
                 if (move_uploaded_file($_FILES['uploadfile']['tmp_name'], $randfileloc)) {
                     $return = array(
-                        "success" => true,
-                        "size"    => $size,
-                        "name"    => rawurlencode(basename($filename)),
-                        "ext"     => $cleanExt,
-                        "filename"      => $randfilename,
-                        "msg"     => gT("The file has been successfully uploaded.")
+                        "success"  => true,
+                        "size"     => $size,
+                        "name"     => $filename,
+                        "ext"      => $cleanExt,
+                        "filename" => $randfilename,
+                        "msg"      => gT("The file has been successfully uploaded.")
                     );
                     //header('Content-Type: application/json');
                     echo ls_json_encode($return);
@@ -341,8 +339,6 @@ public function run($actionID)
         $oTemplate = Template::model()->getInstance('', $surveyid);
         App()->getClientScript()->registerScript('sNeededScriptVar', $sNeededScriptVar, LSYii_ClientScript::POS_BEGIN);
         App()->getClientScript()->registerScript('sLangScriptVar', $sLangScriptVar, LSYii_ClientScript::POS_BEGIN);
-        App()->getClientScript()->registerScriptFile(Yii::app()->getConfig('packages') . 'questions/upload/build/uploadquestion.js');
-        App()->getClientScript()->registerScriptFile(Yii::app()->getConfig('packages') . 'questions/upload/src/ajaxupload.js');
 
         $header = getHeader($meta);
 

diff --git a/application/helpers/common_helper.php b/application/helpers/common_helper.php
@@ -4521,7 +4521,7 @@ function fixSubquestions()
 */
 function ls_json_encode($content)
 {
-    $ans = json_encode($content);
+    $ans = json_encode($content, JSON_UNESCAPED_UNICODE);
     $ans = str_replace(array('{', '}'), array('{ ', ' }'), $ans);
     return $ans;
 }

diff --git a/application/helpers/expressions/em_manager_helper.php b/application/helpers/expressions/em_manager_helper.php
@@ -8452,6 +8452,7 @@ public static function ProcessCurrentResponses()
                                     // Move the (unmoved, temp) files from temp to files directory.
                                     // Check all possible file uploads
                                     for ($i = 0; $i < $iSize; $i++) {
+                                        $aFiles[$i]->name = sanitize_filename($aFiles[$i]->name, false, false, true);
                                         $aFiles[$i]->filename = get_absolute_path($aFiles[$i]->filename);
                                         if (file_exists($tmp . $aFiles[$i]->filename)) {
                                             $sDestinationFileName = 'fu_' . randomChars(15);

diff --git a/application/helpers/qanda_helper.php b/application/helpers/qanda_helper.php
@@ -1782,10 +1782,12 @@ function do_file_upload($ia)
         'value' => $_SESSION['survey_' . Yii::app()->getConfig('surveyID')][$ia[1]],
         'filecountvalue' => $filecountvalue,
         'coreClass' => $coreClass,
+        'maxFiles' =>  $aQuestionAttributes['max_num_of_files'],
         'basename' => $ia[1],
         'uploadurl' => $uploadurl,
-        'show_title' => $aQuestionAttributes['show_title'],
-        'show_comment' => $aQuestionAttributes['show_comment'],
+        'scriptloc' => Yii::app()->getController()->createUrl('/uploader/index/mode/upload/'),
+        'showTitle' => $aQuestionAttributes['show_title'],
+        'showComment' => $aQuestionAttributes['show_comment'],
         'uploadButtonLabel' => ngT("Upload file|Upload files", $aQuestionAttributes['max_num_of_files'])
     );
     $answer = doRender('/survey/questions/answer/file_upload/answer', $fileuploadData, true);

diff --git a/application/helpers/sanitize_helper.php b/application/helpers/sanitize_helper.php
@@ -119,14 +119,12 @@ function nice_addslashes($string)
 function sanitize_filename($filename, $force_lowercase = true, $alphanumeric = false, $beautify = true)
 {
     // sanitize filename
-    $filename = preg_replace(
-        '~
-        [<>:"/\\|?*]|
+    $filename = mb_ereg_replace(
+        '[<>:"\\|?*]|
         [\x00-\x1F]|
         [\x7F\xA0\xAD]|
         [#\[\]@!$&\'()+,;=]|
-        [{}^\~`]
-        ~x',
+        [{}^\~`]',
         '-',
         $filename
     );
@@ -141,7 +139,7 @@ function sanitize_filename($filename, $force_lowercase = true, $alphanumeric = f
     // maximise filename length to 255 bytes http://serverfault.com/a/9548/44086
     $ext = pathinfo($filename, PATHINFO_EXTENSION);
     $filename = mb_strcut(pathinfo($filename, PATHINFO_FILENAME), 0, 255 - ($ext ? strlen($ext) + 1 : 0), mb_detect_encoding($filename)) . ($ext ? '.' . $ext : '');
-    $filename = ($alphanumeric) ? preg_replace("/[^a-zA-Z0-9]/", "", $filename) : $filename;
+    $filename = ($alphanumeric) ? mb_ereg_replace("[^a-zA-Z0-9]", "", $filename) : $filename;
 
     if ($force_lowercase) {
         $filename = mb_strtolower($filename, 'UTF-8');
@@ -171,8 +169,6 @@ function beautify_filename($filename)
         // "file...name..zip" becomes "file.name.zip"
         '/\.{2,}/'
     ), '.', $filename);
-    // lowercase for windows/unix interoperability http://support.microsoft.com/kb/100625
-    $filename = mb_strtolower($filename, mb_detect_encoding($filename));
     // ".file-name.-" becomes "file-name"
     $filename = trim($filename, '.-');
     return $filename;

diff --git a/application/models/SurveyDynamic.php b/application/models/SurveyDynamic.php
@@ -385,7 +385,7 @@ public function getExtendedData($colName, $sLanguage, $base64jsonFieldMap)
 
         // Upload question
         if ($oFieldMap->type == Question::QT_VERTICAL_FILE_UPLOAD && strpos($oFieldMap->fieldname, 'filecount') === false) {
-            $sSurveyEntry = "<table class='table table-condensed upload-question'><tr>";
+            $sSurveyEntry = "<table class='table table-condensed upload-question'>";
             $aQuestionAttributes = QuestionAttribute::model()->getQuestionAttributes($oFieldMap->qid);
             $aFilesInfo = json_decode_ls($this->$colName);
             for ($iFileIndex = 0; $iFileIndex < $aQuestionAttributes['max_num_of_files']; $iFileIndex++) {

diff --git a/application/views/survey/questions/answer/file_upload/answer.twig b/application/views/survey/questions/answer/file_upload/answer.twig
@@ -18,8 +18,8 @@
             href='{{uploadurl}}'
             data-toggle="modal"
             data-target="file-upload-modal-{{fileid}}"
-            data-showtitle="{{show_title}}"
-            data-showcomment="{{show_comment}}"
+            data-showtitle="{{showTitle}}"
+            data-showcomment="{{showComment}}"
             data-fieldname="{{basename}}"
         >
             <span class='fa fa-upload'></span>&nbsp;{{ uploadButtonLabel }}
@@ -88,9 +88,9 @@
     $(document).on("ready pjax:scriptcomplete", function(){
         var fieldname = "{{fileid}}",
             filecount = $("#"+fieldname+"_filecount").val(),
-            show_title = "{{showTitle}}",
-            show_comment = "{{showComment}}";
-        window.uploadQuestionController_{{ fileid }}.displayUploadedFiles( filecount, fieldname, show_title, show_comment);
+            showTitle = "{{showTitle}}",
+            showComment = "{{showComment}}";
+        window.uploadQuestionController_{{ fileid }}.displayUploadedFiles( filecount, fieldname, showTitle, showComment);
     });
 
     $(".basic_{{fileid}}").change(function() {

diff --git a/assets/packages/questions/upload/build/uploadquestion.js b/assets/packages/questions/upload/build/uploadquestion.js
@@ -777,15 +777,15 @@
 
             if (this.isValueInArray(image_extensions, item.ext)) {
               imageOrPlaceholder = "image";
-              imageOrPlaceholderHtml = `<img src="${uploadurl}/filegetcontents/${decodeURIComponent(item.filename)}" class="uploaded" />`;
+              imageOrPlaceholderHtml = '<img src="'+ uploadurl +'/filegetcontents/'+ decodeURIComponent(item.filename) +'" class="uploaded" />';
             } else {
               imageOrPlaceholder = "placeholder";
               imageOrPlaceholderHtml = `<div class="upload-placeholder"></div>`;
             }
 
-            title = show_title != 0 ? item.title : '';
-            comment = show_comment != 0 ? item.comment : '';
-            name = item.name;
+            title = show_title != 0 ? escapeHtml(item.title) : '';
+            comment = show_comment != 0 ? escapeHtml(item.comment) : '';
+            name = escapeHtml(item.name);
             filepointer = iterator;
             const rowHtml = this.replaceWithObject(templateHtml, {
               imageOrPlaceholder,
@@ -808,7 +808,7 @@
         let outString = templateString;
 
         for (let key in objectWithReplacements) {
-          outString = outString.replace(new RegExp(`\{${key}\}`), objectWithReplacements[key]);
+          outString = outString.replace(new RegExp("\{"+key+"\}"), objectWithReplacements[key]);
         }
 
         return outString;
@@ -835,7 +835,7 @@
         var i = iterator;
         var image_extensions = new Array('gif', 'jpeg', 'jpg', 'png', 'swf', 'psd', 'bmp', 'tiff', 'jp2', 'iff', 'bmp', 'xbm', 'ico', 'heic');
         var previewblock = $('<li id="' + fieldname + '_li_' + i + '" class="previewblock file-element"></li>');
-        var previewContainer = $('<div class="file-preview"></div>');
+        var previewContainer = $('<div class="file-preview form-group"></div>');
 
         if (isValueInArray(image_extensions, item.ext.toLowerCase())) {
           previewContainer.append('<img src="' + options.uploadurl + '/filegetcontents/' + item.filename + '" class="uploaded" />');
@@ -865,7 +865,6 @@
         var previewDeleteBlock = $('<div class="form-group"></div>').append($('<a class="btn btn-danger"></a>').html('<span class="fa fa-trash"></span>&nbsp;' + options.uploadLang.deleteFile).on('click', function () {
           deletefile(fieldname, i);
         }).wrap('<div class="input-container text-center"></div>'));
-        $('<fieldset></fieldset>').append(previewTitleContainer).append(previewCommentContainer).append(previewDeleteBlock).wrap('<div class="file-info"></div>').appendTo(previewContainer);
         $('<input type="hidden" />').attr('id', fieldname + '_size_' + i).attr('value', item.size).appendTo(previewblock);
         $('<input type="hidden" />').attr('id', fieldname + '_name_' + i).attr('value', item.name).appendTo(previewblock);
         $('<input type="hidden" />').attr('id', fieldname + '_file_index_' + i).attr('value', i).appendTo(previewblock);
@@ -876,6 +875,7 @@
           previewblock.append(previewContainer);
           $('#field' + fieldname + '_listfiles').append(previewblock);
         }
+        $('<fieldset></fieldset>').append(previewTitleContainer).append(previewCommentContainer).append(previewDeleteBlock).wrap('<div class="file-info"></div>').appendTo(previewblock);
       };
 
       var doFileUpload = function () {

diff --git a/assets/packages/questions/upload/build/uploadquestion.min.js b/assets/packages/questions/upload/build/uploadquestion.min.js
diff --git a/assets/packages/questions/upload/src/modaldialog.js b/assets/packages/questions/upload/src/modaldialog.js
@@ -114,10 +114,10 @@ class UploadQuestionController {
                 let imageOrPlaceholder, imageOrPlaceholderHtml, title, comment, name, filepointer;
                 if (this.isValueInArray(image_extensions, item.ext)) {
                     imageOrPlaceholder = "image";
-                    imageOrPlaceholderHtml = `<img src="${uploadurl}/filegetcontents/${decodeURIComponent(item.filename)}" class="uploaded" />`;
+                    imageOrPlaceholderHtml  = '<img src="'+uploadurl+'/filegetcontents/'+decodeURIComponent(item.filename)+'" class="uploaded" />';
                 } else {
                     imageOrPlaceholder = "placeholder";
-                    imageOrPlaceholderHtml = `<div class="upload-placeholder"></div>`;
+                    imageOrPlaceholderHtml = '<div class="upload-placeholder"></div>';
                 }
 
                 title = (show_title != 0) ? item.title : '';

diff --git a/assets/packages/questions/upload/styles/uploader.css b/assets/packages/questions/upload/styles/uploader.css
@@ -39,7 +39,13 @@ margin:0;
     }
 }
 
-
+.file-preview {
+    display: flex;
+    align-items: center;
+}
+.file-preview span {
+    margin-left: 1em;
+}
 
 .file-info label{
     float:left;
@@ -68,6 +74,7 @@ margin:0;
 }
 .upload-div{
     text-align: center;
+    margin-bottom: 0.5em;    
 }
 .uploadmsg {
     text-align: center;

diff --git a/assets/styles-public/browse.css b/assets/styles-public/browse.css
@@ -31,6 +31,10 @@
     margin-left: 3px;
 }
 
+#responses-grid table.upload-question > tbody > tr:first-child > td {
+    border: none;
+}
+
 /*!
  * Style for browsing response : in jqgrid
  * TODO: Is not in use anymore