From 18ee016ae95b19cd0ff0d52fa0f3d989204b9ed2 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Tue, 8 Aug 2023 14:49:18 +0200 Subject: [PATCH] Implemented modular architecture for filestream security sanitization with an svg-html example --- .../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, 136 insertions(+), 2 deletions(-) create mode 100644 src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs create mode 100644 src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs create mode 100644 src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs create 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 a850a8f371..5790f44521 100644 --- a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +++ b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs @@ -327,6 +327,10 @@ 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 new file mode 100644 index 0000000000..7cf864d9f1 --- /dev/null +++ b/src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs @@ -0,0 +1,46 @@ +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 new file mode 100644 index 0000000000..7dbfaefd0f --- /dev/null +++ b/src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs @@ -0,0 +1,13 @@ +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 new file mode 100644 index 0000000000..33e70e7d74 --- /dev/null +++ b/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); + + /// + /// 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 new file mode 100644 index 0000000000..3c33648d81 --- /dev/null +++ b/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(" + /// 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 5a14a1afc1..a5fd27a415 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs +++ b/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, 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! diff --git a/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs index 223e62d5a3..44164f4262 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs +++ b/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 _logger; private readonly MediaFileManager _mediaFileManager; private ContentSettings _contentSettings; @@ -37,13 +39,15 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v IOptionsMonitor 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! diff --git a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs index 1a8be10a5c..dd7dedd1b4 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs +++ b/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() + "." + ext; + //todo implement Filestreamsecurity + using (Stream fs = file.OpenReadStream()) { mediaFileManager.FileSystem.AddFile(user.Avatar, fs, true);