Skip to content

Commit

Permalink
fix(files): mitigate issues with special chars in file names
Browse files Browse the repository at this point in the history
Relative paths to files that contain special characters in the name
will now be encoded with base64 to avoid malformatted URLs and
HMAC mismatches resulting from unescaped characters.
URLs generated prior to this change will continue working.

Refs #10608
  • Loading branch information
hypeJunction authored and mrclay committed Dec 25, 2016
1 parent 11c4640 commit 4a7b74e
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 6 deletions.
8 changes: 7 additions & 1 deletion engine/classes/Elgg/Application/ServeFileHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Elgg\Http\Request;
use Symfony\Component\HttpFoundation\BinaryFileResponse;
use Symfony\Component\HttpFoundation\Response;
use Elgg\Security\Base64Url;

/**
* File server handler
Expand Down Expand Up @@ -51,7 +52,7 @@ public function getResponse(Request $request) {
$response = new Response();
$response->prepare($request);

$path = implode('/', $request->getUrlSegments());
$path = implode('/', $request->getUrlSegments(true));
if (!preg_match('~serve-file/e(\d+)/l(\d+)/d([ia])/c([01])/([a-zA-Z0-9\-_]+)/(.*)$~', $path, $m)) {
return $response->setStatusCode(400)->setContent('Malformatted request URL');
}
Expand Down Expand Up @@ -79,6 +80,11 @@ public function getResponse(Request $request) {
return $response->setStatusCode(403)->setContent('HMAC mistmatch');
}

// Path may have been encoded to avoid problems with special chars in URLs
if (0 === strpos($path_from_dataroot, ':')) {
$path_from_dataroot = Base64Url::decode(substr($path_from_dataroot, 1));
}

$dataroot = $this->config->getDataPath();
$filenameonfilestore = "{$dataroot}{$path_from_dataroot}";

Expand Down
8 changes: 8 additions & 0 deletions engine/classes/Elgg/FileService/File.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace Elgg\FileService;

use Elgg\Security\Base64Url;

/**
* File service
*
Expand Down Expand Up @@ -106,6 +108,12 @@ public function getURL() {
return false;
}

if (preg_match('~[^a-zA-Z0-9_\./ ]~', $relative_path)) {
// Filenames may contain special characters that result in malformatted URLs
// and/or HMAC mismatches. We want to avoid that by encoding the path.
$relative_path = ':' . Base64Url::encode($relative_path);
}

$data = array(
'expires' => isset($this->expires) ? $this->expires : 0,
'last_updated' => filemtime($this->file->getFilenameOnFilestore()),
Expand Down
33 changes: 33 additions & 0 deletions engine/classes/Elgg/Security/Base64Url.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php
namespace Elgg\Security;

/**
* Encode and decode Base 64 URL
*
* @access private
*/
class Base64Url {

/**
* Encode base 64 URL
*
* @param string $bytes Bytes to encode
* @return string
*/
public static function encode($bytes) {
$bytes = base64_encode($bytes);
$bytes = rtrim($bytes, '=');
return strtr($bytes, '+/', '-_');
}

/**
* Decode base 64 URL
*
* @param string $bytes Bytes to decode
* @return string|false
*/
public static function decode($bytes) {
$bytes = strtr($bytes, '-_', '+/');
return base64_decode($bytes);
}
}
2 changes: 1 addition & 1 deletion engine/classes/Elgg/Security/Hmac.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public function __construct($key, callable $comparator, $data, $algo = 'sha256')
*/
public function getToken() {
$bytes = hash_hmac($this->algo, $this->data, $this->key, true);
return strtr(rtrim(base64_encode($bytes), '='), '+/', '-_');
return Base64Url::encode($bytes);
}

/**
Expand Down
16 changes: 12 additions & 4 deletions engine/tests/phpunit/Elgg/Application/ServeFileHandlerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ class ServeFileHandlerTest extends PHPUnit_Framework_TestCase {
protected $file;

public function setUp() {
$app = _elgg_testing_application();
$dataroot = _elgg_testing_config()->getDataPath();

$session = \ElggSession::getMock();
_elgg_services()->setValue('session', $session);
_elgg_services()->session->start();
Expand All @@ -28,11 +25,22 @@ public function setUp() {

$file = new \ElggFile();
$file->owner_guid = 1;
$file->setFilename("foobar.txt");

// Using special characters to test against files that have been
// uploaded prior to implementation of filename sanitization
// See #10608
$file->setFilename("Iñtërn'âtiônàl-izætiøn.txt");
$file->open('write');
$file->write('Test file!');
$file->close();

$this->file = $file;
}

public function tearDown() {
$this->file->delete();
}

function createRequest(\Elgg\FileService\File $file) {
$site_url = elgg_get_site_url();
$url = $file->getURL();
Expand Down

0 comments on commit 4a7b74e

Please sign in to comment.