From 93c508e118f483f3c93ac36e1f91face95af642d Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Fri, 15 Nov 2024 10:39:29 +0100 Subject: [PATCH] Sanitize svg icons before storing them --- app/Providers/TwoFAuthServiceProvider.php | 5 +- app/Services/IconStoreService.php | 34 +++++++++++++- composer.json | 1 + composer.lock | 47 ++++++++++++++++++- .../Api/v1/Controllers/IconControllerTest.php | 34 +++++++++++++- .../Controllers/TwoFAccountControllerTest.php | 21 +++++++-- tests/Classes/LocalFileFactory.php | 16 +++++++ tests/Data/OtpTestData.php | 10 ++++ .../Feature/Services/IconStoreServiceTest.php | 47 +++++++++++++++++++ 9 files changed, 206 insertions(+), 9 deletions(-) diff --git a/app/Providers/TwoFAuthServiceProvider.php b/app/Providers/TwoFAuthServiceProvider.php index 4792bdfd..b6ca21e6 100644 --- a/app/Providers/TwoFAuthServiceProvider.php +++ b/app/Providers/TwoFAuthServiceProvider.php @@ -9,6 +9,7 @@ use App\Services\ReleaseRadarService; use App\Services\SettingService; use App\Services\TwoFAccountService; +use enshrined\svgSanitize\Sanitizer; use Illuminate\Contracts\Support\DeferrableProvider; use Illuminate\Support\ServiceProvider; use Zxing\QrReader; @@ -30,8 +31,8 @@ public function register() return new SettingService; }); - $this->app->singleton(IconStoreService::class, function () { - return new IconStoreService; + $this->app->singleton(IconStoreService::class, function ($app) { + return new IconStoreService($app->make(Sanitizer::class)); }); $this->app->singleton(LogoService::class, function ($app) { diff --git a/app/Services/IconStoreService.php b/app/Services/IconStoreService.php index a36d7380..64b888ca 100644 --- a/app/Services/IconStoreService.php +++ b/app/Services/IconStoreService.php @@ -6,6 +6,7 @@ use App\Facades\Settings; use App\Models\Icon; use App\Models\TwoFAccount; +use enshrined\svgSanitize\Sanitizer; use Illuminate\Contracts\Filesystem\Filesystem; use Illuminate\Support\Arr; use Illuminate\Support\Collection; @@ -26,13 +27,21 @@ class IconStoreService */ protected bool $usesDatabase; + /** + * The SVG sanitizer + */ + protected Sanitizer $svgSanitizer; + /** * */ - public function __construct() + public function __construct(Sanitizer $svgSanitizer) { $this->usesDatabase = Settings::get('storeIconsInDatabase'); $this->setDisk(); + + $this->svgSanitizer = $svgSanitizer; + $this->svgSanitizer->removeRemoteReferences(true); } /** @@ -207,6 +216,21 @@ public function store(string $name, string $content) : bool { $storedToDisk = $this->storeToDisk($name, $content); + if ($this->mimeType($name) == 'image/svg+xml') { + $sanitized = $this->sanitize($content); + + if (! $sanitized) { + $this->delete($name); + + return false; + } + + if ($content != $sanitized) { + $content = $sanitized; + $storedToDisk = $this->storeToDisk($name, $content); + } + } + if ($this->usesDatabase) { return $this->storeToDatabase($name, $content); } @@ -214,6 +238,14 @@ public function store(string $name, string $content) : bool return $storedToDisk; } + /** + * Sanitize the given content (when icon is an svg image) + */ + protected function sanitize(string $content) : string + { + return $this->svgSanitizer->sanitize($content); + } + /** * Create the given icon in the disk */ diff --git a/composer.json b/composer.json index e2354fa6..7d238369 100644 --- a/composer.json +++ b/composer.json @@ -28,6 +28,7 @@ "ext-xml": "*", "chillerlan/php-qrcode": "^5.0", "doctormckay/steam-totp": "^1.0", + "enshrined/svg-sanitize": "^0.20.0", "google/protobuf": "^4.26", "jackiedo/dotenv-editor": "dev-master", "jenssegers/agent": "^2.6", diff --git a/composer.lock b/composer.lock index b71c3778..cb9123a3 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "bff445ed39877e4dfccfb5b451e0d96a", + "content-hash": "da7b7e586a5f017685b05bf7189d8c5e", "packages": [ { "name": "brick/math", @@ -770,6 +770,51 @@ ], "time": "2023-10-06T06:47:41+00:00" }, + { + "name": "enshrined/svg-sanitize", + "version": "0.20.0", + "source": { + "type": "git", + "url": "https://github.com/darylldoyle/svg-sanitizer.git", + "reference": "068d9fcf912c88a0471d101d95a2caa87c50aee7" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/darylldoyle/svg-sanitizer/zipball/068d9fcf912c88a0471d101d95a2caa87c50aee7", + "reference": "068d9fcf912c88a0471d101d95a2caa87c50aee7", + "shasum": "" + }, + "require": { + "ext-dom": "*", + "ext-libxml": "*", + "php": "^7.1 || ^8.0" + }, + "require-dev": { + "phpunit/phpunit": "^6.5 || ^8.5" + }, + "type": "library", + "autoload": { + "psr-4": { + "enshrined\\svgSanitize\\": "src" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "GPL-2.0-or-later" + ], + "authors": [ + { + "name": "Daryll Doyle", + "email": "daryll@enshrined.co.uk" + } + ], + "description": "An SVG sanitizer for PHP", + "support": { + "issues": "https://github.com/darylldoyle/svg-sanitizer/issues", + "source": "https://github.com/darylldoyle/svg-sanitizer/tree/0.20.0" + }, + "time": "2024-09-05T10:18:12+00:00" + }, { "name": "firebase/php-jwt", "version": "v6.10.1", diff --git a/tests/Api/v1/Controllers/IconControllerTest.php b/tests/Api/v1/Controllers/IconControllerTest.php index e9bba6af..b0e1188f 100644 --- a/tests/Api/v1/Controllers/IconControllerTest.php +++ b/tests/Api/v1/Controllers/IconControllerTest.php @@ -13,6 +13,7 @@ use Illuminate\Support\Facades\Storage; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use Tests\Classes\LocalFile; use Tests\Data\HttpRequestTestData; use Tests\Data\OtpTestData; use Tests\FeatureTestCase; @@ -41,7 +42,7 @@ public function setUp() : void LogoService::TFA_URL => Http::response(HttpRequestTestData::TFA_JSON_BODY, 200), ]); Http::fake([ - OtpTestData::EXTERNAL_IMAGE_URL_DECODED => Http::response((new FileFactory)->image('file.png', 10, 10)->tempFile, 200), + OtpTestData::EXTERNAL_IMAGE_URL_DECODED => Http::response((new FileFactory)->image('file.png', 10, 10)->tempFile, 200), ]); $this->user = User::factory()->create(); @@ -84,6 +85,21 @@ public function test_upload_with_invalid_data_returns_validation_error() ->assertStatus(422); } + #[Test] + public function test_upload_infected_svg_data_stores_stores_sanitized_svg_content() + { + $file = LocalFile::fake()->infectedSvgIconFile(); + + $response = $this->actingAs($this->user, 'api-guard') + ->json('POST', '/api/v1/icons', [ + 'icon' => $file, + ]) + ->assertCreated(); + + $svgContent = IconStore::get($response->getData()->filename); + $this->assertStringNotContainsString(OtpTestData::ICON_SVG_MALICIOUS_CODE, $svgContent); + } + #[Test] public function test_fetch_logo_returns_filename() { @@ -97,6 +113,22 @@ public function test_fetch_logo_returns_filename() ]); } + #[Test] + public function test_fetch_logo_with_infected_svg_data_stores_sanitized_svg_content() + { + $response = $this->actingAs($this->user, 'api-guard') + ->json('POST', '/api/v1/icons/default', [ + 'service' => 'service', + ]) + ->assertStatus(201) + ->assertJsonStructure([ + 'filename', + ]); + + $svgContent = IconStore::get($response->getData()->filename); + $this->assertStringNotContainsString(OtpTestData::ICON_SVG_MALICIOUS_CODE, $svgContent); + } + #[Test] public function test_fetch_unknown_logo_returns_nothing() { diff --git a/tests/Api/v1/Controllers/TwoFAccountControllerTest.php b/tests/Api/v1/Controllers/TwoFAccountControllerTest.php index 0f0a43e5..39a93edd 100644 --- a/tests/Api/v1/Controllers/TwoFAccountControllerTest.php +++ b/tests/Api/v1/Controllers/TwoFAccountControllerTest.php @@ -8,6 +8,7 @@ use App\Api\v1\Resources\TwoFAccountExportResource; use App\Api\v1\Resources\TwoFAccountReadResource; use App\Api\v1\Resources\TwoFAccountStoreResource; +use App\Facades\IconStore; use App\Facades\Settings; use App\Models\Group; use App\Models\TwoFAccount; @@ -242,11 +243,8 @@ public function setUp() : void Http::fake([ LogoService::TFA_IMG_URL . '*' => Http::response(HttpRequestTestData::SVG_LOGO_BODY, 200), LogoService::TFA_URL => Http::response(HttpRequestTestData::TFA_JSON_BODY, 200), - ]); - Http::fake([ OtpTestData::EXTERNAL_IMAGE_URL_DECODED => Http::response((new FileFactory)->image('file.png', 10, 10)->tempFile, 200), - ]); - Http::fake([ + OtpTestData::EXTERNAL_INFECTED_IMAGE_URL_DECODED => Http::response((new FileFactory)->createWithContent('infected.svg', OtpTestData::ICON_SVG_DATA_INFECTED)->tempFile, 200), 'example.com/*' => Http::response(null, 400), ]); @@ -1218,6 +1216,21 @@ public function test_preview_with_unreachable_image_returns_success_with_no_icon ]); } + #[Test] + public function test_preview_with_infected_svg_image_stores_sanitized_image() + { + $this->user['preferences->getOfficialIcons'] = true; + + $response = $this->actingAs($this->user, 'api-guard') + ->json('POST', '/api/v1/twofaccounts/preview', [ + 'uri' => OtpTestData::TOTP_URI_WITH_INFECTED_SVG_IMAGE, + ]) + ->assertOk(); + + $svgContent = IconStore::get($response->getData()->icon); + $this->assertStringNotContainsString(OtpTestData::ICON_SVG_MALICIOUS_CODE, $svgContent); + } + #[Test] public function test_export_returns_json_migration_resource() { diff --git a/tests/Classes/LocalFileFactory.php b/tests/Classes/LocalFileFactory.php index fc257b9c..67cd8a89 100644 --- a/tests/Classes/LocalFileFactory.php +++ b/tests/Classes/LocalFileFactory.php @@ -215,4 +215,20 @@ public function invalidPlainTextFileEmpty() fwrite($temp, ob_get_clean()); })); } + + /** + * Create a new local infected SVG file. + * + * @return \Illuminate\Http\Testing\File + */ + public function infectedSvgIconFile() + { + return new File('infectedSvgIcon.svg', tap(tmpfile(), function ($temp) { + ob_start(); + + echo OtpTestData::ICON_SVG_DATA_INFECTED; + + fwrite($temp, ob_get_clean()); + })); + } } diff --git a/tests/Data/OtpTestData.php b/tests/Data/OtpTestData.php index f0f25d6a..81c83fce 100644 --- a/tests/Data/OtpTestData.php +++ b/tests/Data/OtpTestData.php @@ -36,6 +36,10 @@ class OtpTestData const EXTERNAL_IMAGE_URL_ENCODED = 'https%3A%2F%2Fen.opensuse.org%2Fimages%2F4%2F44%2FButton-filled-colour.png'; + const EXTERNAL_INFECTED_IMAGE_URL_DECODED = 'https://image.com/infected.svg'; + + const EXTERNAL_INFECTED_IMAGE_URL_ENCODED = 'https%3A%2F%2Fimage.com%2Finfected.svg'; + const ICON_PNG = 'test.png'; const ICON_PNG_DATA = 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACXBIWXMAAA7EAAAOxAGVKw4bAAAAC0lEQVQImWP4DwQACfsD/eNV8pwAAAAASUVORK5CYII='; @@ -58,6 +62,10 @@ class OtpTestData const ICON_SVG_DATA_ENCODED = 'PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHZpZXdCb3g9IjAgMCAxMDI0IDEwMjQiPg0KICAgPGNpcmNsZSBjeD0iNTEyIiBjeT0iNTEyIiByPSI1MTIiIHN0eWxlPSJmaWxsOiMwMDBlOWMiLz4NCiAgIDxwYXRoIGQ9Im03MDAuMiA0NjYuNSA2MS4yLTEwNi4zYzIzLjYgNDEuNiAzNy4yIDg5LjggMzcuMiAxNDEuMSAwIDY4LjgtMjQuMyAxMzEuOS02NC43IDE4MS40SDU3NS44bDQ4LjctODQuNmgtNjQuNGw3NS44LTEzMS43IDY0LjMuMXptLTU1LjQtMTI1LjJMNDQ4LjMgNjgyLjVsLjEuMkgyOTAuMWMtNDAuNS00OS41LTY0LjctMTEyLjYtNjQuNy0xODEuNCAwLTUxLjQgMTMuNi05OS42IDM3LjMtMTQxLjNsMTAyLjUgMTc4LjIgMTEzLjMtMTk3aDE2Ni4zeiIgc3R5bGU9ImZpbGw6I2ZmZiIvPg0KPC9zdmc+DQo='; + const ICON_SVG_MALICIOUS_CODE = ''; + + const ICON_SVG_DATA_INFECTED = '' . self::ICON_SVG_MALICIOUS_CODE . ''; + const ICON_GIF = 'test.gif'; const ICON_GIF_DATA = 'R0lGODlhAQACAPcAAAAAAAAAMwAAZgAAmQAAzAAA/wArAAArMwArZgArmQArzAAr/wBVAABVMwBVZgBVmQBVzABV/wCAAACAMwCAZgCAmQCAzACA/wCqAACqMwCqZgCqmQCqzACq/wDVAADVMwDVZgDVmQDVzADV/wD/AAD/MwD/ZgD/mQD/zAD//zMAADMAMzMAZjMAmTMAzDMA/zMrADMrMzMrZjMrmTMrzDMr/zNVADNVMzNVZjNVmTNVzDNV/zOAADOAMzOAZjOAmTOAzDOA/zOqADOqMzOqZjOqmTOqzDOq/zPVADPVMzPVZjPVmTPVzDPV/zP/ADP/MzP/ZjP/mTP/zDP//2YAAGYAM2YAZmYAmWYAzGYA/2YrAGYrM2YrZmYrmWYrzGYr/2ZVAGZVM2ZVZmZVmWZVzGZV/2aAAGaAM2aAZmaAmWaAzGaA/2aqAGaqM2aqZmaqmWaqzGaq/2bVAGbVM2bVZmbVmWbVzGbV/2b/AGb/M2b/Zmb/mWb/zGb//5kAAJkAM5kAZpkAmZkAzJkA/5krAJkrM5krZpkrmZkrzJkr/5lVAJlVM5lVZplVmZlVzJlV/5mAAJmAM5mAZpmAmZmAzJmA/5mqAJmqM5mqZpmqmZmqzJmq/5nVAJnVM5nVZpnVmZnVzJnV/5n/AJn/M5n/Zpn/mZn/zJn//8wAAMwAM8wAZswAmcwAzMwA/8wrAMwrM8wrZswrmcwrzMwr/8xVAMxVM8xVZsxVmcxVzMxV/8yAAMyAM8yAZsyAmcyAzMyA/8yqAMyqM8yqZsyqmcyqzMyq/8zVAMzVM8zVZszVmczVzMzV/8z/AMz/M8z/Zsz/mcz/zMz///8AAP8AM/8AZv8Amf8AzP8A//8rAP8rM/8rZv8rmf8rzP8r//9VAP9VM/9VZv9Vmf9VzP9V//+AAP+AM/+AZv+Amf+AzP+A//+qAP+qM/+qZv+qmf+qzP+q///VAP/VM//VZv/Vmf/VzP/V////AP//M///Zv//mf//zP///wAAAAAAAAAAAAAAACH5BAEAAPwALAAAAAABAAIAAAgFAPftCwgAOw=='; @@ -86,6 +94,8 @@ class OtpTestData const TOTP_URI_WITH_UNREACHABLE_IMAGE = 'otpauth://totp/service:account?secret=A4GRFHVVRBGY7UIW&image=' . self::UNREACHABLE_IMAGE_URL; + const TOTP_URI_WITH_INFECTED_SVG_IMAGE = 'otpauth://totp/service:account?secret=A4GRFHVVRBGY7UIW&image=' . self::EXTERNAL_INFECTED_IMAGE_URL_ENCODED; + const INVALID_OTPAUTH_URI = 'otpauth://Xotp/' . self::ACCOUNT . '?secret=' . self::SECRET; const INVALID_OTPAUTH_URI_MISMATCHING_ISSUER = 'otpauth://totp/' . self::MICROSOFT . ':' . self::ACCOUNT . '?secret=' . self::SECRET . '&issuer=' . self::SERVICE; diff --git a/tests/Feature/Services/IconStoreServiceTest.php b/tests/Feature/Services/IconStoreServiceTest.php index 19c35621..7f33abdb 100644 --- a/tests/Feature/Services/IconStoreServiceTest.php +++ b/tests/Feature/Services/IconStoreServiceTest.php @@ -482,11 +482,58 @@ public function test_store_returns_false_when_it_fails() ->with($iconName, $iconContent) ->andReturn(false); + Storage::shouldReceive('disk->mimeType') + ->with($iconName) + ->andReturn('image/png'); + $result = $this->iconStore->store($iconName, $iconContent); $this->assertFalse($result); } + #[Test] + public function test_store_stores_sanitized_svg_content() + { + Settings::set('storeIconsInDatabase', true); + + $result = $this->iconStore->store(OtpTestData::ICON_SVG, OtpTestData::ICON_SVG_DATA_INFECTED); + + $this->assertTrue($result); + + $this->assertStringNotContainsString( + OtpTestData::ICON_SVG_MALICIOUS_CODE, + Storage::disk('icons')->get(OtpTestData::ICON_SVG) + ); + + $dbRecord = DB::table('icons')->where('name', OtpTestData::ICON_SVG)->first(); + + $this->assertStringNotContainsString( + OtpTestData::ICON_SVG_MALICIOUS_CODE, + $dbRecord->content, + ); + } + + #[Test] + public function test_store_returns_false_when_svg_sanitize_failed() + { + $result = $this->iconStore->store(OtpTestData::ICON_SVG, 'this_will_make_svg_data_invalid' . OtpTestData::ICON_SVG_DATA); + + $this->assertFalse($result); + } + + #[Test] + public function test_store_deletes_svg_icon_that_cannot_be_sanitized() + { + Settings::set('storeIconsInDatabase', true); + + $result = $this->iconStore->store(OtpTestData::ICON_SVG, 'this_will_make_svg_data_invalid' . OtpTestData::ICON_SVG_DATA); + + Storage::disk('icons')->assertMissing(OtpTestData::ICON_SVG); + $this->assertDatabaseMissing('icons', [ + 'name' => OtpTestData::ICON_SVG, + ]); + } + #[Test] public function test_exists_returns_true() {