Skip to content

Commit

Permalink
xss: Draft Uploaded Attachment
Browse files Browse the repository at this point in the history
This mitigates an XSS vulnerability reported by [The NAVEX
Project](https://sisl.lab.uic.edu/projects/chess/) where if an attachment is
uploaded via Draft AJAX, the filename stored/returned in the response is not
sanitized. This updates `DraftAjaxAPI::_uploadInlineImage()` to sanitize the
filename before it's returned in the JSON encoded response. This also
updates `AttachmentFile::format()` to sanitize the filename before saving to
the backend. In addition this forces the `application/json` Content-Type on
all AJAX responses that return strictly JSON; this adds another layer of
protection.
  • Loading branch information
JediKev committed Aug 19, 2020
1 parent d98c2d0 commit 518de22
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 1 deletion.
6 changes: 5 additions & 1 deletion include/ajax.draft.php
Expand Up @@ -16,6 +16,7 @@ function _createDraft($vars) {
if (!($draft = Draft::create($vars)) || !$draft->save())
Http::response(500, 'Unable to create draft');

header('Content-Type: application/json; charset=UTF-8');
echo JsonDataEncoder::encode(array(
'draft_id' => $draft->getId(),
));
Expand All @@ -27,6 +28,7 @@ function _getDraft($draft) {

$body = Format::viewableImages($draft->getBody());

header('Content-Type: application/json; charset=UTF-8');
echo JsonDataEncoder::encode(array(
'body' => $body,
'draft_id' => $draft->getId(),
Expand Down Expand Up @@ -126,8 +128,9 @@ function _uploadInlineImage($draft) {
if (!($f = AttachmentFile::lookup($id)))
return Http::response(500, 'Unable to attach image');

header('Content-Type: application/json; charset=UTF-8');
echo JsonDataEncoder::encode(array(
$f->getName() => array(
Format::sanitize($f->getName()) => array(
'content_id' => 'cid:'.$f->getKey(),
'id' => $f->getKey(),
// Return draft_id to connect the auto draft creation
Expand Down Expand Up @@ -369,6 +372,7 @@ function getFileList() {
'title' => $f->getName(),
);
}
header('Content-Type: application/json; charset=UTF-8');
echo JsonDataEncoder::encode($files);
}

Expand Down
2 changes: 2 additions & 0 deletions include/ajax.forms.php
Expand Up @@ -382,6 +382,7 @@ function upload($id) {
if (!$impl instanceof FileUploadField)
Http::response(400, 'Upload to a non file-field');

header('Content-Type: application/json; charset=UTF-8');
return JsonDataEncoder::encode(
array('id'=>$impl->ajaxUpload())
);
Expand All @@ -404,6 +405,7 @@ function attach($object=null) {
->first()->getConfiguration();
$field = new FileUploadField();
$field->_config = $config;
header('Content-Type: application/json; charset=UTF-8');
return JsonDataEncoder::encode(
array('id'=>$field->ajaxUpload($thisstaff ? true : false))
);
Expand Down
3 changes: 3 additions & 0 deletions include/ajax.i18n.php
Expand Up @@ -49,6 +49,7 @@ function getTranslations($tag) {
$json = JsonDataEncoder::encode($phrases) ?: '{}';
//Http::cacheable(md5($json), $lm);

header('Content-Type: application/json; charset=UTF-8');
return $json;
}

Expand Down Expand Up @@ -124,6 +125,7 @@ function getConfiguredLanguages() {
$json = JsonDataEncoder::encode($langs);
Http::cacheable(md5($json), $cfg->lastModified());

header('Content-Type: application/json; charset=UTF-8');
return $json;
}

Expand All @@ -142,6 +144,7 @@ function getSecondaryLanguages() {
$json = JsonDataEncoder::encode($langs);
Http::cacheable(md5($json), $cfg->lastModified());

header('Content-Type: application/json; charset=UTF-8');
return $json;
}
}
Expand Down
2 changes: 2 additions & 0 deletions include/class.file.php
Expand Up @@ -633,6 +633,8 @@ function format($files) {

//Basic validation.
foreach($attachments as $i => &$file) {
$file['name'] = Format::sanitize($file['name']);

//skip no file upload "error" - why PHP calls it an error is beyond me.
if($file['error'] && $file['error']==UPLOAD_ERR_NO_FILE) {
unset($attachments[$i]);
Expand Down

0 comments on commit 518de22

Please sign in to comment.