From 2b3f048db7cf6cbb9f3d921d541e9fd8a3960f70 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Tue, 8 Aug 2023 16:26:59 +0200 Subject: [PATCH] Revert "Implemented modular architecture for filestream security sanitization with an svg-html example" This reverts commit 18ee016ae95b19cd0ff0d52fa0f3d989204b9ed2. --- .../DependencyInjection/UmbracoBuilder.cs | 4 -- ...eStreamSecuritySanitizationOrchestrator.cs | 46 ------------------- ...eStreamSecuritySanitizationOrchestrator.cs | 13 ------ .../Security/IFileStreamSecuritySanitizer.cs | 16 ------- .../SvgFileStreamSecuritySanitizer.cs | 37 --------------- .../FileUploadPropertyValueEditor.cs | 10 +--- .../ImageCropperPropertyValueEditor.cs | 10 +--- .../Controllers/UsersController.cs | 2 - 8 files changed, 2 insertions(+), 136 deletions(-) delete mode 100644 src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs delete mode 100644 src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs delete mode 100644 src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs delete mode 100644 src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs diff --git a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs index 5790f44521..a850a8f371 100644 --- a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +++ b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs @@ -327,10 +327,6 @@ namespace Umbraco.Cms.Core.DependencyInjection Services.AddUnique(provider => new CultureImpactFactory(provider.GetRequiredService>())); Services.AddUnique(); Services.AddUnique(); - - // Register filestream security sanitizers - Services.AddUnique(); - Services.AddSingleton(); } } } diff --git a/src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs b/src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs deleted file mode 100644 index 7cf864d9f1..0000000000 --- a/src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs +++ /dev/null @@ -1,46 +0,0 @@ -namespace Umbraco.Cms.Core.Security; - -public class FileStreamSecuritySanitizationOrchestrator : IFileStreamSecuritySanitizationOrchestrator -{ - private readonly IEnumerable _fileSanitizers; - - public FileStreamSecuritySanitizationOrchestrator(IEnumerable fileSanitizers) - { - _fileSanitizers = fileSanitizers; - } - - /// - /// Reads the file content and executes sanitization routines based on the determined file type - /// - /// Needs to be a Read/Write seekable stream - /// Clear fileContent if sanitization fails? - /// If true, this will make the method return true if sanitization fails and file was cleared - /// Whether the file is considered clean after running the necessary sanitizers - 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; - } -} diff --git a/src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs b/src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs deleted file mode 100644 index 7dbfaefd0f..0000000000 --- a/src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs +++ /dev/null @@ -1,13 +0,0 @@ -namespace Umbraco.Cms.Core.Security; - -public interface IFileStreamSecuritySanitizationOrchestrator -{ - /// - /// Reads the file content and executes sanitization routines based on the determined file type - /// - /// Needs to be a Read/Write seekable stream - /// Clear fileContent if sanitization fails? - /// If true, this will make the method return true if sanitization fails and file was cleared - /// Whether the file is considered clean after running the necessary sanitizers - bool Sanitize(FileStream fileStream, bool clearContentIfSanitizationFails); -} diff --git a/src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs b/src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs deleted file mode 100644 index 33e70e7d74..0000000000 --- a/src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs +++ /dev/null @@ -1,16 +0,0 @@ -namespace Umbraco.Cms.Core.Security; - -public interface IFileStreamSecuritySanitizer -{ - public int MinimumStartBytesRequiredForContentTypeMatching { get; } - public int MinimumEndBytesRequiredForContentTypeMatching{ get; } - - bool FileContentMatchesFileType(byte[] startBytes, byte[] endBytes); - - /// - /// Sanitizes the filestream so it doesn't contain security sensitive content - /// - /// Needs to be a read/Write seekable stream - /// Whether the fileStream is considered clean after the method was run - bool RemoveSensitiveContent(FileStream fileStream); -} diff --git a/src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs b/src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs deleted file mode 100644 index 3c33648d81..0000000000 --- a/src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs +++ /dev/null @@ -1,37 +0,0 @@ -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(" - /// Sanitizes the svg filestream (Remove html and nested Javascript) - /// - /// Needs to be a read/Write seekable stream - /// Whether the fileStream is considered clean after the method was run - 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; - } -} diff --git a/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs index a5fd27a415..5a14a1afc1 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs @@ -5,7 +5,6 @@ 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; @@ -19,7 +18,6 @@ namespace Umbraco.Cms.Core.PropertyEditors; internal class FileUploadPropertyValueEditor : DataValueEditor { private readonly MediaFileManager _mediaFileManager; - private readonly IFileStreamSecuritySanitizationOrchestrator _fileStreamSanitizer; private ContentSettings _contentSettings; public FileUploadPropertyValueEditor( @@ -29,12 +27,10 @@ internal class FileUploadPropertyValueEditor : DataValueEditor IShortStringHelper shortStringHelper, IOptionsMonitor contentSettings, IJsonSerializer jsonSerializer, - IIOHelper ioHelper, - IFileStreamSecuritySanitizationOrchestrator fileStreamSanitizer) + IIOHelper ioHelper) : 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); } @@ -151,10 +147,6 @@ 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! diff --git a/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs index 44164f4262..223e62d5a3 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs @@ -10,7 +10,6 @@ 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; @@ -25,7 +24,6 @@ namespace Umbraco.Cms.Core.PropertyEditors; internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core vs web? { private readonly IDataTypeService _dataTypeService; - private readonly IFileStreamSecuritySanitizationOrchestrator _fileStreamSanitizer; private readonly ILogger _logger; private readonly MediaFileManager _mediaFileManager; private ContentSettings _contentSettings; @@ -39,15 +37,13 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v IOptionsMonitor contentSettings, IJsonSerializer jsonSerializer, IIOHelper ioHelper, - IDataTypeService dataTypeService, - IFileStreamSecuritySanitizationOrchestrator fileStreamSanitizer) + IDataTypeService dataTypeService) : 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); } @@ -240,10 +236,6 @@ 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! diff --git a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs index dd7dedd1b4..1a8be10a5c 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs @@ -187,8 +187,6 @@ 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() + "." + ext; - //todo implement Filestreamsecurity - using (Stream fs = file.OpenReadStream()) { mediaFileManager.FileSystem.AddFile(user.Avatar, fs, true);