Cross-site Scripting (XSS) - Stored in osticket/osticket
Reported on
Oct 18th 2021
Description
As it is written on github profile, osTicket is a widely-used open source support ticket system. During source code research I discovered bad uploaded file type check, which is controlled by user. Unauthenticated user can upload malicious html/js file.
FROM OWASP:: Cross-Site Scripting (XSS) attacks are a type of injection, in which malicious scripts are injected into otherwise benign and trusted websites. XSS attacks occur when an attacker uses a web application to send malicious code, generally in the form of a browser side script, to a different end user.
Proof of Concept
POST /ajax.php/draft/ticket.client.jfi710uc6f1h/attach HTTP/1.1
Host: 172.17.0.1:8888
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:94.0) Gecko/20100101 Firefox/94.0
Accept: */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
X-Requested-With: XMLHttpRequest
Content-Type: multipart/form-data; boundary=---------------------------124988469840978117853377254190
Content-Length: 6456
Origin: http://172.17.0.1:8888
DNT: 1
Connection: close
Referer: http://172.17.0.1:8888/open.php
Cookie: OSTSESSID=2h2moaijbnk5nojfi710uc6f1h
-----------------------------124988469840978117853377254190
Content-Disposition: form-data; name="file[]"; filename="osticket-phish.html"
Content-Type: image/png,text/html
<html>
<img src=x onerror=alert("XSS")>
</html>
-----------------------------124988469840978117853377254190--
PoC Video
https://www.youtube.com/watch?v=iasn6dEDnCw
Impact
FROM OWASP:: An attacker can use XSS to send a malicious script to an unsuspecting user. The end user’s browser has no way to know that the script should not be trusted, and will execute the script. Because it thinks the script came from a trusted source, the malicious script can access any cookies, session tokens, or other sensitive information retained by the browser and used with that site.
In this case local link generated link is valid for few hours, and is visible by issue author and administrators.
@theworstcomrade
Thank you for the report. We will start investigating this to verify it's legitimacy. Once verified, I will mark this as Valid and start the process of patching.
Cheers.
@theworstcomrade
Can you please test the following patch to ensure it addresses the issue on your side?
diff --git a/bootstrap.php b/bootstrap.php
index 8d65604..13ad074 100644
--- a/bootstrap.php
+++ b/bootstrap.php
@@ -45,6 +45,15 @@ class Bootstrap {
}
date_default_timezone_set('UTC');
+ if (!function_exists('exif_imagetype')) {
+ function exif_imagetype ($filename) {
+ if ((list($width,$height,$type,) = getimagesize($filename)) !== false)
+ return $type;
+
+ return false;
+ }
+ }
+
if (!isset($_SERVER['REMOTE_ADDR']))
$_SERVER['REMOTE_ADDR'] = '';
}
diff --git a/include/ajax.draft.php b/include/ajax.draft.php
index 3268b65..408a575 100644
--- a/include/ajax.draft.php
+++ b/include/ajax.draft.php
@@ -53,78 +53,71 @@ class DraftAjaxAPI extends AjaxController {
function _uploadInlineImage($draft) {
global $cfg;
- if (!isset($_POST['data']) && !isset($_FILES['file']))
+ if (!isset($_FILES['file']))
Http::response(422, "File not included properly");
# Fixup for expected multiple attachments
- if (isset($_FILES['file'])) {
- $file = AttachmentFile::format($_FILES['file']);
-
- # Allow for data-uri uploaded files
- $fp = fopen($file[0]['tmp_name'], 'rb');
- if (fread($fp, 5) == 'data:') {
- $data = 'data:';
- while ($block = fread($fp, 8192))
- $data .= $block;
- $file[0] = Format::parseRfc2397($data);
- list(,$ext) = explode('/', $file[0]['type'], 2);
- $file[0] += array(
- 'name' => Misc::randCode(8).'.'.$ext,
- 'size' => strlen($file[0]['data']),
- );
- }
- fclose($fp);
+ $file = AttachmentFile::format($_FILES['file']);
+
+ # Allow for data-uri uploaded files
+ $fp = fopen($file[0]['tmp_name'], 'rb');
+ if (fread($fp, 5) == 'data:') {
+ $data = 'data:';
+ while ($block = fread($fp, 8192))
+ $data .= $block;
+ $file[0] = Format::parseRfc2397($data);
+ list(,$ext) = explode('/', $file[0]['type'], 2);
+ $file[0] += array(
+ 'name' => Misc::randCode(8).'.'.$ext,
+ 'size' => strlen($file[0]['data']),
+ );
+ }
+ fclose($fp);
+
+ // Check file type to ensure image
+ $type = $file[0]['type'];
+ if (strpos($file[0]['type'], 'image/') !== 0)
+ return Http::response(403,
+ JsonDataEncoder::encode(array(
+ 'error' => 'File type is not allowed',
+ ))
+ );
- # TODO: Detect unacceptable attachment extension
- # TODO: Verify content-type and check file-content to ensure image
- $type = $file[0]['type'];
- if (strpos($file[0]['type'], 'image/') !== 0)
- return Http::response(403,
- JsonDataEncoder::encode(array(
- 'error' => 'File type is not allowed',
- ))
- );
+ // Check if file is truly an image
+ if (!FileUploadField::isValidFile($file[0]))
+ return Http::response(403,
+ JsonDataEncoder::encode(array(
+ 'error' => 'File is not valid',
+ ))
+ );
- # TODO: Verify file size is acceptable
- if ($file[0]['size'] > $cfg->getMaxFileSize())
+ // Verify file size is acceptable
+ if ($file[0]['size'] > $cfg->getMaxFileSize())
+ return Http::response(403,
+ JsonDataEncoder::encode(array(
+ 'error' => 'File is too large',
+ ))
+ );
+
+ // Paste uploads in Chrome will have a name of 'blob'
+ if ($file[0]['name'] == 'blob')
+ $file[0]['name'] = 'screenshot-'.Misc::randCode(4);
+
+ $ids = $draft->attachments->upload($file);
+
+ if (!$ids) {
+ if ($file[0]['error']) {
return Http::response(403,
JsonDataEncoder::encode(array(
- 'error' => 'File is too large',
+ 'error' => $file[0]['error'],
))
);
-
- // Paste uploads in Chrome will have a name of 'blob'
- if ($file[0]['name'] == 'blob')
- $file[0]['name'] = 'screenshot-'.Misc::randCode(4);
-
- $ids = $draft->attachments->upload($file);
-
- if (!$ids) {
- if ($file[0]['error']) {
- return Http::response(403,
- JsonDataEncoder::encode(array(
- 'error' => $file[0]['error'],
- ))
- );
- }
- else
- return Http::response(500, 'Unable to attach image');
}
-
- $id = (is_array($ids)) ? $ids[0] : $ids;
- }
- else {
- $type = explode('/', $_POST['contentType']);
- $info = array(
- 'data' => base64_decode($_POST['data']),
- 'name' => Misc::randCode(10).'.'.$type[1],
- // TODO: Ensure _POST['contentType']
- 'type' => $_POST['contentType'],
- );
- // TODO: Detect unacceptable filetype
- // TODO: Verify content-type and check file-content to ensure image
- $id = $draft->attachments->save($info);
+ else
+ return Http::response(500, 'Unable to attach image');
}
+
+ $id = (is_array($ids)) ? $ids[0] : $ids;
if (!($f = AttachmentFile::lookup($id)))
return Http::response(500, 'Unable to attach image');
diff --git a/include/class.forms.php b/include/class.forms.php
index 9f632cb..9d71c6c 100644
--- a/include/class.forms.php
+++ b/include/class.forms.php
@@ -3906,7 +3906,6 @@ class FileUploadField extends FormField {
// Check invalid image hacks
if ($file['tmp_name']
&& stripos($file['type'], 'image/') === 0
- && function_exists('exif_imagetype')
&& !exif_imagetype($file['tmp_name']))
return false;
Cheers.
@JediKev Hi, In my opinion this patch is not good enough. You are still comparing Content-type value entered by the user. Which means You can still upload for example real SWF file setting this content-type image/png,application/vnd.adobe.flash.movie. There is possibility to enter javascript into the swf file, but at this moment I'm not able to force browser to run it instead of download.
Think about using mime_content_type function and building some kind mime type whitelist. Also I don't understand why are You twice comparing $file[0]['type'], but this an extra note beyond this report :)
Overall, this patch prevents from sending a file not handled by the getimagesize function.
@theworstcomrade
Thank you for the feedback. Could you please provide a POC of uploading an SWF file after applying the above patch?
About the mime_content_type(), you are correct in that we shouldn't trust the user provided content-type. We are working on improving the validation for mime-types.
The first $file[0]['type'] check is to make sure the uploaded file is in fact an image and if not gets rejected immediately. The subsequent check in FileUploadField::isValidFile() is generic and only checks the content signature if the uploaded file is an image. So, with this in mind the first check is necessary in this context.
By the way, osTicket has the ability to restrict file uploads based on mime-types, configurable globally or by individual File Upload field. You are correct in that if it's not handled by getimagesize() then it gets rejected, only for images. Do you have an example of an image that's not being handled by getimagesize() function?
Please note that getimagesize() is only used if the other method (exif_imagetype()) does not exist (eg. some Windows PHP installs).
Cheers.
@JediKev
Thank You for response. As I wrote earlier, I am able just to upload swf file and download it without execution inside browser
Sample files You can find https://github.com/pykexe/SWF-xss-based or here https://github.com/evilcos/xss.swf
POST /ajax.php/draft/103/attach HTTP/1.1
Host: 172.17.0.1:8888
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:94.0) Gecko/20100101 Firefox/94.0
Accept: */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
X-Requested-With: XMLHttpRequest
Content-Type: multipart/form-data; boundary=---------------------------131731669352538571332988628
Content-Length: 564
Origin: http://172.17.0.1:8888
DNT: 1
Connection: close
Referer: http://172.17.0.1:8888/open.php
Cookie: OSTSESSID=4r9voaqlv2bpfdcrdau2end9r7
-----------------------------131731669352538571332988628
Content-Disposition: form-data; name="file[]"; filename="xss-2.swf"
Content-Type: image/png,application/vnd.adobe.flash.movie
<SWF_FILE_HERE>
-----------------------------131731669352538571332988628--
I also recorder screen to show it
https://www.youtube.com/watch?v=3uM9oS6rAbE
@theworstcomrade
Thank you for the feedback and references. We were able to replicate the vulnerability with the example .swf files. With the updated patch below we are now using mime_content_type() as a backup to finfo_file() to check the mime-type of the uploaded file itself. I can confirm this patch rejects the .swf files as well but I would like for you to please review the updated patch on your side to confirm this fully resolves the issue.
diff --git a/bootstrap.php b/bootstrap.php
index 8d65604..13ad074 100644
--- a/bootstrap.php
+++ b/bootstrap.php
@@ -45,6 +45,15 @@ class Bootstrap {
}
date_default_timezone_set('UTC');
+ if (!function_exists('exif_imagetype')) {
+ function exif_imagetype ($filename) {
+ if ((list($width,$height,$type,) = getimagesize($filename)) !== false)
+ return $type;
+
+ return false;
+ }
+ }
+
if (!isset($_SERVER['REMOTE_ADDR']))
$_SERVER['REMOTE_ADDR'] = '';
}
diff --git a/include/ajax.draft.php b/include/ajax.draft.php
index 3268b65..408a575 100644
--- a/include/ajax.draft.php
+++ b/include/ajax.draft.php
@@ -53,78 +53,71 @@ class DraftAjaxAPI extends AjaxController {
function _uploadInlineImage($draft) {
global $cfg;
- if (!isset($_POST['data']) && !isset($_FILES['file']))
+ if (!isset($_FILES['file']))
Http::response(422, "File not included properly");
# Fixup for expected multiple attachments
- if (isset($_FILES['file'])) {
- $file = AttachmentFile::format($_FILES['file']);
-
- # Allow for data-uri uploaded files
- $fp = fopen($file[0]['tmp_name'], 'rb');
- if (fread($fp, 5) == 'data:') {
- $data = 'data:';
- while ($block = fread($fp, 8192))
- $data .= $block;
- $file[0] = Format::parseRfc2397($data);
- list(,$ext) = explode('/', $file[0]['type'], 2);
- $file[0] += array(
- 'name' => Misc::randCode(8).'.'.$ext,
- 'size' => strlen($file[0]['data']),
- );
- }
- fclose($fp);
+ $file = AttachmentFile::format($_FILES['file']);
+
+ # Allow for data-uri uploaded files
+ $fp = fopen($file[0]['tmp_name'], 'rb');
+ if (fread($fp, 5) == 'data:') {
+ $data = 'data:';
+ while ($block = fread($fp, 8192))
+ $data .= $block;
+ $file[0] = Format::parseRfc2397($data);
+ list(,$ext) = explode('/', $file[0]['type'], 2);
+ $file[0] += array(
+ 'name' => Misc::randCode(8).'.'.$ext,
+ 'size' => strlen($file[0]['data']),
+ );
+ }
+ fclose($fp);
+
+ // Check file type to ensure image
+ $type = $file[0]['type'];
+ if (strpos($file[0]['type'], 'image/') !== 0)
+ return Http::response(403,
+ JsonDataEncoder::encode(array(
+ 'error' => 'File type is not allowed',
+ ))
+ );
- # TODO: Detect unacceptable attachment extension
- # TODO: Verify content-type and check file-content to ensure image
- $type = $file[0]['type'];
- if (strpos($file[0]['type'], 'image/') !== 0)
- return Http::response(403,
- JsonDataEncoder::encode(array(
- 'error' => 'File type is not allowed',
- ))
- );
+ // Check if file is truly an image
+ if (!FileUploadField::isValidFile($file[0]))
+ return Http::response(403,
+ JsonDataEncoder::encode(array(
+ 'error' => 'File is not valid',
+ ))
+ );
- # TODO: Verify file size is acceptable
- if ($file[0]['size'] > $cfg->getMaxFileSize())
+ // Verify file size is acceptable
+ if ($file[0]['size'] > $cfg->getMaxFileSize())
+ return Http::response(403,
+ JsonDataEncoder::encode(array(
+ 'error' => 'File is too large',
+ ))
+ );
+
+ // Paste uploads in Chrome will have a name of 'blob'
+ if ($file[0]['name'] == 'blob')
+ $file[0]['name'] = 'screenshot-'.Misc::randCode(4);
+
+ $ids = $draft->attachments->upload($file);
+
+ if (!$ids) {
+ if ($file[0]['error']) {
return Http::response(403,
JsonDataEncoder::encode(array(
- 'error' => 'File is too large',
+ 'error' => $file[0]['error'],
))
);
-
- // Paste uploads in Chrome will have a name of 'blob'
- if ($file[0]['name'] == 'blob')
- $file[0]['name'] = 'screenshot-'.Misc::randCode(4);
-
- $ids = $draft->attachments->upload($file);
-
- if (!$ids) {
- if ($file[0]['error']) {
- return Http::response(403,
- JsonDataEncoder::encode(array(
- 'error' => $file[0]['error'],
- ))
- );
- }
- else
- return Http::response(500, 'Unable to attach image');
}
-
- $id = (is_array($ids)) ? $ids[0] : $ids;
- }
- else {
- $type = explode('/', $_POST['contentType']);
- $info = array(
- 'data' => base64_decode($_POST['data']),
- 'name' => Misc::randCode(10).'.'.$type[1],
- // TODO: Ensure _POST['contentType']
- 'type' => $_POST['contentType'],
- );
- // TODO: Detect unacceptable filetype
- // TODO: Verify content-type and check file-content to ensure image
- $id = $draft->attachments->save($info);
+ else
+ return Http::response(500, 'Unable to attach image');
}
+
+ $id = (is_array($ids)) ? $ids[0] : $ids;
if (!($f = AttachmentFile::lookup($id)))
return Http::response(500, 'Unable to attach image');
diff --git a/include/class.file.php b/include/class.file.php
index 9247939..cec531b 100644
--- a/include/class.file.php
+++ b/include/class.file.php
@@ -1047,12 +1047,8 @@ class FileObject extends SplFileObject {
}
function getMimeType() {
- if (!isset($this->_mimetype)) {
- // Try to to auto-detect mime type
- $finfo = new finfo(FILEINFO_MIME);
- $this->_mimetype = $finfo->buffer($this->getContents(),
- FILEINFO_MIME_TYPE);
- }
+ if (!isset($this->_mimetype))
+ $this->_mimetype = self::mime_type($this->getRealPath());
return $this->_mimetype;
}
@@ -1068,6 +1064,21 @@ class FileObject extends SplFileObject {
function getData() {
return $this->getContents();
}
+
+ /*
+ * Given a filepath - auto detect the mime type
+ *
+ */
+ static function mime_type($filepath) {
+ // Try to to auto-detect mime type
+ $type = null;
+ if (function_exists('finfo_open')) {
+ $finfo = finfo_open(FILEINFO_MIME_TYPE);
+ $type = finfo_file($finfo, $filepath);
+ finfo_close($finfo);
+ }
+ return $type ?: mime_content_type($filepath);
+ }
}
?>
diff --git a/include/class.forms.php b/include/class.forms.php
index 9f632cb..657facc 100644
--- a/include/class.forms.php
+++ b/include/class.forms.php
@@ -3903,10 +3903,14 @@ class FileUploadField extends FormField {
function isValidFile($file) {
+ // Make sure mime type is valid
+ if (strcasecmp(FileObject::mime_type($file['tmp_name']),
+ $file['type']) !== 0)
+ return false;
+
// Check invalid image hacks
if ($file['tmp_name']
&& stripos($file['type'], 'image/') === 0
- && function_exists('exif_imagetype')
&& !exif_imagetype($file['tmp_name']))
return false;
Cheers.
@JediKev
This patch is good. I don't see other possibilities to upload file different than image.