Subverting SS4 assets module and potentially weakening the security of the module

[maxime-rainville]

Silverstripe Gridfield Queued Export stores CSV files in the assets/.exports dir. These files get deleted once downloaded, but if for some reason, you never download the file, the file will be stuck in limbo forever and never get deleted.

It also completely bypasses all the assets logic and directly writes and unlinks the files. There's some attempt to write a .htaccess file to block direct download from the file, but that method is fallible because your webserver could be configured to ignore .htaccess files or you might be running your site on NGINX or IIS. The file names are also random, which minimise the risk that someone will stumble on them.

It's arguable whatever this is an actual security vulnerability. I guess you need a lot of things to go wrong for the files to be disclosed publicly. It sure is not good security architecture.

At the very least, it's a GDPR problem because the CSV data could be stuck there without a way to delete it.

This is the bit that creates the file.

silverstripe-gridfieldqueuedexport/src/Jobs/GenerateCSVJob.php

Lines 176 to 247 in 06a09ca

	protected function makeDir($path)
	{
	if (!is_dir($path)) {
	// whether to use 'chmod' to override 'mkdir' perms which obey umask
	$ignore_umask = $this->config()->get('ignore_umask');
	// perms mode given to 'mkdir' and 'chmod'
	$permission_mode = $this->config()->get('permission_mode');
	// only permit numeric strings as they work with or without the leading zero
	if (!is_string($permission_mode) || !is_numeric($permission_mode)) {
	throw new Exception("Only string values are allowed for 'permission_mode'");
	}
	// convert from octal to decimal for mkdir
	$permission_mode = octdec($permission_mode);
	// make dir with perms that obey the executing user's umask
	mkdir($path, $permission_mode, true);
	// override perms to ignore user's umask?
	if ($ignore_umask) {
	chmod($path, $permission_mode);
	}
	}
	}
	protected function getOutputPath()
	{
	$base = ASSETS_PATH . '/.exports';
	$this->makeDir($base);
	// Although the string is random, so should be hard to guess, also try and block access directly.
	// Only works in Apache though
	if (!file_exists("$base/.htaccess")) {
	file_put_contents("$base/.htaccess", "Deny from all\nRewriteRule .* - [F]\n");
	}
	$folder = $base . '/' . $this->getSignature();
	$this->makeDir($folder);
	return $folder . '/' . $this->getSignature() . '.csv';
	}
	/**
	* @return Writer
	*/
	protected function getCSVWriter()
	{
	if (!$this->writer) {
	$csvWriter = Writer::createFromPath($this->getOutputPath(), 'w');
	$csvWriter->setDelimiter($this->Seperator);
	$csvWriter->setNewline("\r\n"); //use windows line endings for compatibility with some csv libraries
	$csvWriter->setOutputBOM(Writer::BOM_UTF8);
	if (!Config::inst()->get(GridFieldExportButton::class, 'xls_export_disabled')) {
	$csvWriter->addFormatter(function (array $row) {
	foreach ($row as &$item) {
	// [SS-2017-007] Sanitise XLS executable column values with a leading tab
	if (preg_match('/^[-@=+].*/', $item)) {
	$item = "\t" . $item;
	}
	}
	return $row;
	});
	}
	$this->writer = $csvWriter;
	}
	return $this->writer;
	}

This is the bit that serves the file and delete it.

silverstripe-gridfieldqueuedexport/src/Forms/GridFieldQueuedExportButton.php

Lines 234 to 261 in 06a09ca

	/**
	* @param GridField $gridField
	* @param HTTPRequest $request
	* @return HTTPResponse
	*/
	public function downloadExport($gridField, $request = null)
	{
	$id = $request->param('ID');
	$job = QueuedJobDescriptor::get()->filter('Signature', $id)->first();
	if ((int)$job->RunAsID !== Security::getCurrentUser()->ID) {
	return Security::permissionFailure();
	}
	$now = Date("d-m-Y-H-i");
	$servedName = "export-$now.csv";
	$path = $this->getExportPath($id);
	$content = file_get_contents($path);
	unlink($path);
	rmdir(dirname($path));
	$response = HTTPRequest::send_file($content, $servedName, 'text/csv');
	$response->addHeader('Set-Cookie', 'downloaded_' . $id . '=true; Path=/');
	$response->output();
	exit;
	}

Notes

This was initially reported as a security issue. We decided to threat it as a regular issue since there isn't anything directly exploitable.