Commit 91f15848 authored by Robert Lyon's avatar Robert Lyon
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 11734cf6
...@@ -242,12 +242,15 @@ class PluginBlocktypeInternalmedia extends PluginBlocktype { ...@@ -242,12 +242,15 @@ class PluginBlocktypeInternalmedia extends PluginBlocktype {
$count++; $count++;
$id = 'blocktype_internalmedia_flash_' . time() . $count; $id = 'blocktype_internalmedia_flash_' . time() . $count;
$url = self::get_download_link($artefact, $block); $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> $html = '<a href="' . $url . '">' . hsc($artefact->get('title')) . '</a><br>
<span class="blocktype_internalmedia_mp3" id="' . $id . '">(' <span class="blocktype_internalmedia_mp3" id="' . $id . '">('
. get_string('flashanimation', 'blocktype.file/internalmedia') . ')</span> . get_string('flashanimation', 'blocktype.file/internalmedia') . ')</span>
<script type="text/javascript"> <script type="text/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.addParam("allowfullscreen","false");
so.addVariable("displayheight"," ' . $height . '"); so.addVariable("displayheight"," ' . $height . '");
so.addVariable("type", "swf"); so.addVariable("type", "swf");
...@@ -361,7 +364,7 @@ class PluginBlocktypeInternalmedia extends PluginBlocktype { ...@@ -361,7 +364,7 @@ class PluginBlocktypeInternalmedia extends PluginBlocktype {
public static function wmp_player($artefact, $block, $width, $height) { 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 . '"'; $size = 'width="' . $width . '" height="' . $height . '"';
$autosize = 'false'; $autosize = 'false';
...@@ -437,10 +440,9 @@ class PluginBlocktypeInternalmedia extends PluginBlocktype { ...@@ -437,10 +440,9 @@ class PluginBlocktypeInternalmedia extends PluginBlocktype {
</object></span>'; </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=' return get_config('wwwroot') . 'artefact/file/download.php?file='
. $artefact->get('id') . '&view=' . $instance->get('view') . $artefact->get('id') . '&view=' . $instance->get('view');
. ($wmp ? '&download=1' : '');
} }
private static function get_js_source($asarray = false) { private static function get_js_source($asarray = false) {
......
...@@ -29,7 +29,7 @@ ...@@ -29,7 +29,7 @@
</head> </head>
<body class="no-js"> <body class="no-js">
<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" class="loadingInProgress js"> <div id="outerContainer" class="loadingInProgress js">
<div id="sidebarContainer"> <div id="sidebarContainer">
......
...@@ -18,18 +18,13 @@ require_once('file.php'); ...@@ -18,18 +18,13 @@ require_once('file.php');
$fileid = param_integer('file'); $fileid = param_integer('file');
$viewid = param_integer('view', null); $viewid = param_integer('view', null);
$postid = param_integer('post', null); $postid = param_integer('post', null);
$isembedded = param_integer('embedded', 0);
$size = get_imagesize_parameters(); $size = get_imagesize_parameters();
$forcedl = param_boolean('download');
$options = array(); $options = array();
if ($forcedl) { if (empty($isembedded)) {
$options['forcedownload'] = true; $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) { if ($viewid && $fileid) {
$file = artefact_instance_from_id($fileid); $file = artefact_instance_from_id($fileid);
......
...@@ -180,17 +180,12 @@ function zip_write_contents(&$zip, $filepath, $allfiles) { ...@@ -180,17 +180,12 @@ function zip_write_contents(&$zip, $filepath, $allfiles) {
} }
} }
$forcedl = param_boolean('download'); $embedded = param_boolean('embedded', null);
$options = array(); $options = array();
if ($forcedl) { if (empty($embedded)) {
$options['forcedownload'] = true; $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. // Clean up the temp directory before creating anymore zip files.
zip_clean_temp_dir(); zip_clean_temp_dir();
......
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