Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Merge pull request #10171 from snipe/fixes/xss_svg_in_file_uploads
Fixed SVG XSS vuln
  • Loading branch information
snipe committed Oct 6, 2021
2 parents c06a93e + ccd430c commit fc5efd8
Show file tree
Hide file tree
Showing 9 changed files with 143 additions and 88 deletions.
24 changes: 22 additions & 2 deletions app/Http/Controllers/Assets/AssetFilesController.php
Expand Up @@ -2,14 +2,14 @@

namespace App\Http\Controllers\Assets;


use App\Http\Controllers\Controller;
use App\Http\Requests\AssetFileRequest;
use App\Models\Actionlog;
use App\Models\Asset;
use Illuminate\Support\Facades\Response;
use Illuminate\Support\Facades\Storage;
use App\Helpers\StorageHelper;
use enshrined\svgSanitize\Sanitizer;

class AssetFilesController extends Controller
{
Expand All @@ -36,9 +36,29 @@ public function store(AssetFileRequest $request, $assetId = null)
if (!Storage::exists('private_uploads/assets')) Storage::makeDirectory('private_uploads/assets', 775);

foreach ($request->file('file') as $file) {

$extension = $file->getClientOriginalExtension();
$file_name = 'hardware-'.$asset->id.'-'.str_random(8).'-'.str_slug(basename($file->getClientOriginalName(), '.'.$extension)).'.'.$extension;
Storage::put('private_uploads/assets/'.$file_name, file_get_contents($file));

// Check for SVG and sanitize it
if ($extension=='svg') {
\Log::debug('This is an SVG');

$sanitizer = new Sanitizer();
$dirtySVG = file_get_contents($file->getRealPath());
$cleanSVG = $sanitizer->sanitize($dirtySVG);

try {
Storage::put('private_uploads/assets/'.$file_name, $cleanSVG);
} catch (\Exception $e) {
\Log::debug('Upload no workie :( ');
\Log::debug($e);
}
} else {
Storage::put('private_uploads/assets/'.$file_name, file_get_contents($file));
}


$asset->logUpload($file_name, e($request->get('notes')));
}
return redirect()->back()->with('success', trans('admin/hardware/message.upload.success'));
Expand Down
35 changes: 23 additions & 12 deletions app/Http/Controllers/Licenses/LicenseFilesController.php
Expand Up @@ -6,11 +6,11 @@
use App\Http\Requests\AssetFileRequest;
use App\Models\Actionlog;
use App\Models\License;
use Illuminate\Support\Facades\Input;
use Illuminate\Support\Facades\Response;
use Illuminate\Support\Facades\Storage;
use Symfony\Component\HttpFoundation\JsonResponse;
use App\Helpers\StorageHelper;
use enshrined\svgSanitize\Sanitizer;

class LicenseFilesController extends Controller
{
Expand All @@ -37,28 +37,39 @@ public function store(AssetFileRequest $request, $licenseId = null)

if (!Storage::exists('private_uploads/licenses')) Storage::makeDirectory('private_uploads/licenses', 775);

$upload_success = false;
foreach ($request->file('file') as $file) {

$extension = $file->getClientOriginalExtension();
$file_name = 'license-'.$license->id.'-'.str_random(8).'-'.str_slug(basename($file->getClientOriginalName(), '.'.$extension)).'.'.$extension;

$file_name = 'license-'.$license->id.'-'.str_random(8).'-'.str_slug(basename($file->getClientOriginalName(), '.'.$file->getClientOriginalExtension())).'.'.$file->getClientOriginalExtension();

// Check for SVG and sanitize it
if ($extension == 'svg') {
\Log::debug('This is an SVG');
\Log::debug($file_name);

$upload_success = $file->storeAs('private_uploads/licenses', $file_name);
// $upload_success = $file->storeAs('private_uploads/licenses/'.$file_name, $file);
$sanitizer = new Sanitizer();
$dirtySVG = file_get_contents($file->getRealPath());
$cleanSVG = $sanitizer->sanitize($dirtySVG);

try {
Storage::put('private_uploads/licenses/'.$file_name, $cleanSVG);
} catch (\Exception $e) {
\Log::debug('Upload no workie :( ');
\Log::debug($e);
}

} else {
Storage::put('private_uploads/licenses/'.$file_name, file_get_contents($file));
}

//Log the upload to the log
$license->logUpload($file_name, e($request->input('notes')));
}

// This being called from a modal seems to confuse redirect()->back()
// It thinks we should go to the dashboard. As this is only used
// from the modal at present, hardcode the redirect. Longterm
// maybe we evaluate something else.
if ($upload_success) {

return redirect()->route('licenses.show', $license->id)->with('success', trans('admin/licenses/message.upload.success'));
}
return redirect()->route('licenses.show', $license->id)->with('error', trans('admin/licenses/message.upload.error'));

}
return redirect()->route('licenses.show', $license->id)->with('error', trans('admin/licenses/message.upload.nofiles'));
}
Expand Down
33 changes: 27 additions & 6 deletions app/Http/Controllers/Users/UserFilesController.php
Expand Up @@ -10,6 +10,8 @@
use Illuminate\Support\Facades\Input;
use Illuminate\Support\Facades\Response;
use Symfony\Component\HttpFoundation\JsonResponse;
use enshrined\svgSanitize\Sanitizer;
use Illuminate\Support\Facades\Storage;

class UserFilesController extends Controller
{
Expand Down Expand Up @@ -38,12 +40,31 @@ public function store(AssetFileRequest $request, $userId = null)
return redirect()->back()->with('error', trans('admin/users/message.upload.nofiles'));
}
foreach($files as $file) {

$extension = $file->getClientOriginalExtension();
$filename = 'user-' . $user->id . '-' . str_random(8);
$filename .= '-' . str_slug($file->getClientOriginalName()) . '.' . $extension;
if (!$file->move($destinationPath, $filename)) {
return redirect()->back()->with('error', trans('admin/users/message.upload.invalidfiles'));
}
$file_name = 'user-'.$user->id.'-'.str_random(8).'-'.str_slug(basename($file->getClientOriginalName(), '.'.$extension)).'.'.$extension;


// Check for SVG and sanitize it
if ($extension == 'svg') {
\Log::debug('This is an SVG');
\Log::debug($file_name);

$sanitizer = new Sanitizer();
$dirtySVG = file_get_contents($file->getRealPath());
$cleanSVG = $sanitizer->sanitize($dirtySVG);

try {
Storage::put('private_uploads/users/'.$file_name, $cleanSVG);
} catch (\Exception $e) {
\Log::debug('Upload no workie :( ');
\Log::debug($e);
}

} else {
Storage::put('private_uploads/users/'.$file_name, file_get_contents($file));
}

//Log the uploaded file to the log
$logAction = new Actionlog();
$logAction->item_id = $user->id;
Expand All @@ -52,7 +73,7 @@ public function store(AssetFileRequest $request, $userId = null)
$logAction->note = $request->input('notes');
$logAction->target_id = null;
$logAction->created_at = date("Y-m-d H:i:s");
$logAction->filename = $filename;
$logAction->filename = $file_name;
$logAction->action_type = 'uploaded';

if (!$logAction->save()) {
Expand Down

0 comments on commit fc5efd8

Please sign in to comment.