Implemented modular architecture for filestream security sanitization with an svg-html example

src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs

@@ -327,6 +327,10 @@ namespace Umbraco.Cms.Core.DependencyInjection
             Services.AddUnique<ICultureImpactFactory>(provider => new CultureImpactFactory(provider.GetRequiredService<IOptionsMonitor<ContentSettings>>()));
             Services.AddUnique<IDictionaryService, DictionaryService>();
             Services.AddUnique<ITemporaryMediaService, TemporaryMediaService>();
+
+            // Register filestream security sanitizers
+            Services.AddUnique<IFileStreamSecuritySanitizationOrchestrator,FileStreamSecuritySanitizationOrchestrator>();
+            Services.AddSingleton<IFileStreamSecuritySanitizer, SvgFileStreamSecuritySanitizer>();
         }
     }
 }

src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs

@@ -0,0 +1,46 @@
+namespace Umbraco.Cms.Core.Security;
+
+public class FileStreamSecuritySanitizationOrchestrator : IFileStreamSecuritySanitizationOrchestrator
+{
+    private readonly IEnumerable<IFileStreamSecuritySanitizer> _fileSanitizers;
+
+    public FileStreamSecuritySanitizationOrchestrator(IEnumerable<IFileStreamSecuritySanitizer> fileSanitizers)
+    {
+        _fileSanitizers = fileSanitizers;
+    }
+
+    /// <summary>
+    /// Reads the file content and executes sanitization routines based on the determined file type
+    /// </summary>
+    /// <param name="fileStream">Needs to be a Read/Write seekable stream</param>
+    /// <param name="clearContentIfSanitizationFails">Clear fileContent if sanitization fails?
+    /// If true, this will make the method return true if sanitization fails and file was cleared</param>
+    /// <returns>Whether the file is considered clean after running the necessary sanitizers</returns>
+    public bool Sanitize(FileStream fileStream, bool clearContentIfSanitizationFails)
+    {
+        var startBuffer = new byte[_fileSanitizers.Max(fs => fs.MinimumStartBytesRequiredForContentTypeMatching)];
+        var endBuffer = new byte[_fileSanitizers.Max(fs => fs.MinimumEndBytesRequiredForContentTypeMatching)];
+        fileStream.Read(startBuffer);
+        fileStream.Seek(endBuffer.Length * -1, SeekOrigin.End);
+        fileStream.Read(endBuffer);
+        fileStream.Seek(0, SeekOrigin.Begin);
+
+        foreach (var fileSanitizer in _fileSanitizers)
+        {
+            if (!fileSanitizer.FileContentMatchesFileType(startBuffer, endBuffer))
+            {
+                continue;
+            }
+
+            var sanitationResult = fileSanitizer.RemoveSensitiveContent(fileStream);
+            if (clearContentIfSanitizationFails == false)
+                return sanitationResult;
+
+            fileStream.SetLength(0);
+            return true;
+        }
+
+        // No sanitizer found, we consider the file to be safe as the implementer has the possibility to add additional sanitizers
+        return true;
+    }
+}

src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs

@@ -0,0 +1,13 @@
+namespace Umbraco.Cms.Core.Security;
+
+public interface IFileStreamSecuritySanitizationOrchestrator
+{
+    /// <summary>
+    /// Reads the file content and executes sanitization routines based on the determined file type
+    /// </summary>
+    /// <param name="fileStream">Needs to be a Read/Write seekable stream</param>
+    /// <param name="clearContentIfSanitizationFails">Clear fileContent if sanitization fails?
+    /// If true, this will make the method return true if sanitization fails and file was cleared</param>
+    /// <returns>Whether the file is considered clean after running the necessary sanitizers</returns>
+    bool Sanitize(FileStream fileStream, bool clearContentIfSanitizationFails);
+}

src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs

@@ -0,0 +1,16 @@
+namespace Umbraco.Cms.Core.Security;
+
+public interface IFileStreamSecuritySanitizer
+{
+    public int MinimumStartBytesRequiredForContentTypeMatching { get; }
+    public int MinimumEndBytesRequiredForContentTypeMatching{ get; }
+
+    bool FileContentMatchesFileType(byte[] startBytes, byte[] endBytes);
+
+    /// <summary>
+    /// Sanitizes the filestream so it doesn't contain security sensitive content
+    /// </summary>
+    /// <param name="fileStream">Needs to be a read/Write seekable stream</param>
+    /// <returns>Whether the fileStream is considered clean after the method was run</returns>
+    bool RemoveSensitiveContent(FileStream fileStream);
+}

src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs

@@ -0,0 +1,37 @@
+namespace Umbraco.Cms.Core.Security;
+
+public class SvgFileStreamSecuritySanitizer : IFileStreamSecuritySanitizer
+{
+    private readonly IHtmlSanitizer _htmlSanitizer;
+    public int MinimumStartBytesRequiredForContentTypeMatching => 256;
+    public int MinimumEndBytesRequiredForContentTypeMatching => 256;
+
+    public SvgFileStreamSecuritySanitizer(IHtmlSanitizer htmlSanitizer)
+    {
+        _htmlSanitizer = htmlSanitizer;
+    }
+
+    public bool FileContentMatchesFileType(byte[] startBytes, byte[] endBytes)
+    {
+        var startString = System.Text.Encoding.UTF8.GetString(startBytes, 0, startBytes.Length);
+        return startString.Contains("<svg") && startString.Contains(" xmlns=\"http://www.w3.org/2000/svg\"");
+    }
+
+    /// <summary>
+    /// Sanitizes the svg filestream (Remove html and nested Javascript)
+    /// </summary>
+    /// <param name="fileStream">Needs to be a read/Write seekable stream</param>
+    /// <returns>Whether the fileStream is considered clean after the method was run</returns>
+    public bool RemoveSensitiveContent(FileStream fileStream)
+    {
+        //todo optimize streams?, make async?
+        using var streamReader = new StreamReader(fileStream);
+        var fileContent = streamReader.ReadToEnd();
+        _htmlSanitizer.Sanitize(fileContent);
+        var outBytes = streamReader.CurrentEncoding.GetBytes(fileContent);
+        fileStream.Seek(0, SeekOrigin.Begin);
+        fileStream.SetLength(outBytes.Length);
+        fileStream.Write(outBytes);
+        return true;
+    }
+}

src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs

@@ -5,6 +5,7 @@ using Microsoft.Extensions.Options;
 using Umbraco.Cms.Core.Configuration.Models;
 using Umbraco.Cms.Core.IO;
 using Umbraco.Cms.Core.Models.Editors;
+using Umbraco.Cms.Core.Security;
 using Umbraco.Cms.Core.Serialization;
 using Umbraco.Cms.Core.Services;
 using Umbraco.Cms.Core.Strings;
@@ -18,6 +19,7 @@ namespace Umbraco.Cms.Core.PropertyEditors;
 internal class FileUploadPropertyValueEditor : DataValueEditor
 {
     private readonly MediaFileManager _mediaFileManager;
+    private readonly IFileStreamSecuritySanitizationOrchestrator _fileStreamSanitizer;
     private ContentSettings _contentSettings;
 
     public FileUploadPropertyValueEditor(
@@ -27,10 +29,12 @@ internal class FileUploadPropertyValueEditor : DataValueEditor
         IShortStringHelper shortStringHelper,
         IOptionsMonitor<ContentSettings> contentSettings,
         IJsonSerializer jsonSerializer,
-        IIOHelper ioHelper)
+        IIOHelper ioHelper,
+        IFileStreamSecuritySanitizationOrchestrator fileStreamSanitizer)
         : base(localizedTextService, shortStringHelper, jsonSerializer, ioHelper, attribute)
     {
         _mediaFileManager = mediaFileManager ?? throw new ArgumentNullException(nameof(mediaFileManager));
+        _fileStreamSanitizer = fileStreamSanitizer;
         _contentSettings = contentSettings.CurrentValue ?? throw new ArgumentNullException(nameof(contentSettings));
         contentSettings.OnChange(x => _contentSettings = x);
     }
@@ -147,6 +151,10 @@ internal class FileUploadPropertyValueEditor : DataValueEditor
 
         using (FileStream filestream = File.OpenRead(file.TempFilePath))
         {
+            //todo? are we allowed to return null here?
+            if (_fileStreamSanitizer.Sanitize(filestream, false) == false)
+                return null;
+
             // TODO: Here it would make sense to do the auto-fill properties stuff but the API doesn't allow us to do that right
             // since we'd need to be able to return values for other properties from these methods
             _mediaFileManager.FileSystem.AddFile(filepath, filestream, true); // must overwrite!

src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs

@@ -10,6 +10,7 @@ using Umbraco.Cms.Core.IO;
 using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Models.Editors;
 using Umbraco.Cms.Core.PropertyEditors.ValueConverters;
+using Umbraco.Cms.Core.Security;
 using Umbraco.Cms.Core.Serialization;
 using Umbraco.Cms.Core.Services;
 using Umbraco.Cms.Core.Strings;
@@ -24,6 +25,7 @@ namespace Umbraco.Cms.Core.PropertyEditors;
 internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core vs web?
 {
     private readonly IDataTypeService _dataTypeService;
+    private readonly IFileStreamSecuritySanitizationOrchestrator _fileStreamSanitizer;
     private readonly ILogger<ImageCropperPropertyValueEditor> _logger;
     private readonly MediaFileManager _mediaFileManager;
     private ContentSettings _contentSettings;
@@ -37,13 +39,15 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v
         IOptionsMonitor<ContentSettings> contentSettings,
         IJsonSerializer jsonSerializer,
         IIOHelper ioHelper,
-        IDataTypeService dataTypeService)
+        IDataTypeService dataTypeService,
+        IFileStreamSecuritySanitizationOrchestrator fileStreamSanitizer)
         : base(localizedTextService, shortStringHelper, jsonSerializer, ioHelper, attribute)
     {
         _logger = logger ?? throw new ArgumentNullException(nameof(logger));
         _mediaFileManager = mediaFileSystem ?? throw new ArgumentNullException(nameof(mediaFileSystem));
         _contentSettings = contentSettings.CurrentValue;
         _dataTypeService = dataTypeService;
+        _fileStreamSanitizer = fileStreamSanitizer;
         contentSettings.OnChange(x => _contentSettings = x);
     }
 
@@ -236,6 +240,10 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v
 
         using (FileStream filestream = File.OpenRead(file.TempFilePath))
         {
+            //??? are we allowed to return null here?
+            if (_fileStreamSanitizer.Sanitize(filestream, false) == false)
+                return null;
+
             // TODO: Here it would make sense to do the auto-fill properties stuff but the API doesn't allow us to do that right
             // since we'd need to be able to return values for other properties from these methods
             _mediaFileManager.FileSystem.AddFile(filepath, filestream, true); // must overwrite!

src/Umbraco.Web.BackOffice/Controllers/UsersController.cs

@@ -187,6 +187,8 @@ public class UsersController : BackOfficeNotificationsController
             //generate a path of known data, we don't want this path to be guessable
             user.Avatar = "UserAvatars/" + (user.Id + safeFileName).GenerateHash<SHA1>() + "." + ext;
 
+            //todo implement Filestreamsecurity
+
             using (Stream fs = file.OpenReadStream())
             {
                 mediaFileManager.FileSystem.AddFile(user.Avatar, fs, true);