Sanitize svg icons before storing them

2024-11-21 15:53:13 +01:00 · 2024-11-15 10:39:29 +01:00 · 2024-11-15 10:39:29 +01:00 · 93c508e118
commit 93c508e118
parent 738be76c95
9 changed files with 206 additions and 9 deletions
--- 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) {
--- 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
     */
--- 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",
--- 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",
--- 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()
    {
--- 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()
    {
--- 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());
+        }));
+    }
 }
--- 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 = '<script>alert("XSS");</script>';
+
+    const ICON_SVG_DATA_INFECTED = '<?xml version="1.0" standalone="no"?><!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd"><svg width="100" height="100" version="1.1" xmlns="http://www..w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">' . self::ICON_SVG_MALICIOUS_CODE . '</svg>';
+
    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;
--- 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()
    {