Commit 8df9bdfa authored by Robert Lyon's avatar Robert Lyon Committed by Gerrit Code Review
Browse files

Stopping SWF files XSS exploitation (Bug #1190788)

By doing two things:

1) Getting the embedded SWF object to set the
 allowscriptaccess = "never" and allownetworking = "never"

2) By forcing a 'download file' link to actually download file
- this goes for all files now that don't have embedded=1
in their url.

I've done it this way, having the embedded item have extra url param
so that if a user tries to manipulate a url by removing params it
will default to force download.

I've merged the changes I'd done here https://reviews.mahara.org/#/c/3522/2


and I've also cleaned up places where the download=1 was used as that is
not needed now. Now if there are places where we need to embed rather
than download we add the embedded=1 to the url.

Change-Id: If5290a7c571d06d4178ef2ae5c4c09ed287403b4
Signed-off-by: Robert Lyon's avatarRobert Lyon <robertl@catalyst.net.nz>
parent 7cb72733
......@@ -264,12 +264,15 @@ class PluginBlocktypeInternalmedia extends PluginBlocktype {
$count++;
$id = 'blocktype_internalmedia_flash_' . time() . $count;
$url = self::get_download_link($artefact, $block);
$params = array('play' => 'true');
$params = array('play' => 'true',
'allowscriptaccess' => 'never',
'allownetworking' => 'never'
);
$html = '<a href="' . $url . '">' . hsc($artefact->get('title')) . '</a><br>
<span class="blocktype_internalmedia_mp3" id="' . $id . '">('
. get_string('flashanimation', 'blocktype.file/internalmedia') . ')</span>
<script type="application/javascript">
var so = new SWFObject("' . $url . '","player","' . $width . '","' . ($height + 20). '","7");
var so = new SWFObject("' . $url . '&embedded=1","player","' . $width . '","' . ($height + 20). '","7");
so.addParam("allowfullscreen","false");
so.addVariable("displayheight"," ' . $height . '");
so.addVariable("type", "swf");
......@@ -383,7 +386,7 @@ class PluginBlocktypeInternalmedia extends PluginBlocktype {
public static function wmp_player($artefact, $block, $width, $height) {
$url = hsc(self::get_download_link($artefact, $block, true));
$url = hsc(self::get_download_link($artefact, $block));
$size = 'width="' . $width . '" height="' . $height . '"';
$autosize = 'false';
......@@ -459,10 +462,9 @@ class PluginBlocktypeInternalmedia extends PluginBlocktype {
</object></span>';
}
private static function get_download_link(ArtefactTypeFile $artefact, BlockInstance $instance, $wmp=false) {
private static function get_download_link(ArtefactTypeFile $artefact, BlockInstance $instance) {
return get_config('wwwroot') . 'artefact/file/download.php?file='
. $artefact->get('id') . '&view=' . $instance->get('view')
. ($wmp ? '&download=1' : '');
. $artefact->get('id') . '&view=' . $instance->get('view');
}
private static function get_js_source($asarray = false) {
......
......@@ -20,19 +20,13 @@ $fileid = param_integer('file');
$groupid = param_integer('group', 0);
$viewid = param_integer('view', null);
$postid = param_integer('post', null);
$size = get_imagesize_parameters();
$forcedl = param_boolean('download');
$isembedded = param_integer('embedded', 0);
$size = get_imagesize_parameters();
$options = array();
if ($forcedl) {
if (empty($isembedded)) {
$options['forcedownload'] = true;
}
else {
$options['downloadurl'] = get_config('wwwroot')
. substr($_SERVER['REQUEST_URI'], strpos($_SERVER['REQUEST_URI'], 'artefact/file/download.php'))
. '&download=1';
}
if ($viewid && $fileid) {
$file = artefact_instance_from_id($fileid);
......
......@@ -179,17 +179,12 @@ function zip_write_contents(&$zip, $filepath, $allfiles) {
}
}
$forcedl = param_boolean('download');
$embedded = param_boolean('embedded', null);
$options = array();
if ($forcedl) {
if (empty($embedded)) {
$options['forcedownload'] = true;
}
else {
$options['downloadurl'] = get_config('wwwroot')
. substr($_SERVER['REQUEST_URI'], strpos($_SERVER['REQUEST_URI'], 'artefact/file/downloadfolder.php'))
. '&download=1';
}
// Clean up the temp directory before creating anymore zip files.
zip_clean_temp_dir();
......
......@@ -31,7 +31,7 @@
</head>
<body class="no-js loadingInProgress" tabindex="1">
<div id="nojsdownload" class="no-js"><a href="{$url|safe}&download=1">{$title}</a></div>
<div id="nojsdownload" class="no-js"><a href="{$url|safe}">{$title}</a></div>
<div id="outerContainer">
<div id="sidebarContainer">
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment