From 311d322129d6db5ed5c3f1be98ee58e4e35ed2d4 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Mon, 21 Aug 2023 13:08:26 +0200 Subject: [PATCH 1/2] Add code infrastructure to validate file content (#14657) * Implemented modular architecture for filestream security sanitization with an svg-html example * 31440: Refactoring, applied to more entry points and removed test analyzer * 31440 Added Unittests for FileStreamSecurityValidator * PR fixes and better unittest mock names --------- Co-authored-by: Sven Geusens --- .../DependencyInjection/UmbracoBuilder.cs | 3 + .../EmbeddedResources/Lang/en.xml | 1 + .../EmbeddedResources/Lang/en_us.xml | 1 + .../EmbeddedResources/Lang/nl.xml | 1 + .../Security/FileStreamSecurityValidator.cs | 38 ++++++ .../Security/IFileStreamSecurityAnalyzer.cs | 20 +++ .../Security/IFileStreamSecurityValidator.cs | 11 ++ .../FileUploadPropertyValueEditor.cs | 11 +- .../ImageCropperPropertyValueEditor.cs | 11 +- .../Controllers/CurrentUserController.cs | 35 +++++ .../Controllers/MediaController.cs | 68 +++++++++- .../Controllers/UsersController.cs | 68 +++++++++- .../FileStreamSecurityValidatorTests.cs | 123 ++++++++++++++++++ 13 files changed, 382 insertions(+), 9 deletions(-) create mode 100644 src/Umbraco.Core/Security/FileStreamSecurityValidator.cs create mode 100644 src/Umbraco.Core/Security/IFileStreamSecurityAnalyzer.cs create mode 100644 src/Umbraco.Core/Security/IFileStreamSecurityValidator.cs create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/FileStreamSecurityValidatorTests.cs diff --git a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs index a850a8f371..dcf1ee5183 100644 --- a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +++ b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs @@ -327,6 +327,9 @@ namespace Umbraco.Cms.Core.DependencyInjection Services.AddUnique(provider => new CultureImpactFactory(provider.GetRequiredService>())); Services.AddUnique(); Services.AddUnique(); + + // Register filestream security analyzers + Services.AddUnique(); } } } diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/en.xml b/src/Umbraco.Core/EmbeddedResources/Lang/en.xml index 3b7bf261bd..a2ab932701 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/en.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/en.xml @@ -341,6 +341,7 @@ Failed to create a folder under parent id %0% Failed to rename the folder with id %0% Drag and drop your file(s) into the area + One or more file security validations have failed Create a new member diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml b/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml index 44d07b2511..699bf69ebd 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml @@ -352,6 +352,7 @@ Failed to rename the folder with id %0% Drag and drop your file(s) into the area Upload is not allowed in this location. + One or more file security validations have failed Create a new member diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml b/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml index eab1f856d9..847f6807d4 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml @@ -340,6 +340,7 @@ Kan de map met id %0% niet hernoemen Sleep en zet je bestand(en) neer in dit gebied Upload is niet toegelaten in deze locatie. + Een of meerdere veiligheid validaties zijn gefaald voor het bestand Maak nieuw lid aan diff --git a/src/Umbraco.Core/Security/FileStreamSecurityValidator.cs b/src/Umbraco.Core/Security/FileStreamSecurityValidator.cs new file mode 100644 index 0000000000..764ea37d3d --- /dev/null +++ b/src/Umbraco.Core/Security/FileStreamSecurityValidator.cs @@ -0,0 +1,38 @@ +namespace Umbraco.Cms.Core.Security; + +public class FileStreamSecurityValidator : IFileStreamSecurityValidator +{ + private readonly IEnumerable _fileAnalyzers; + + public FileStreamSecurityValidator(IEnumerable fileAnalyzers) + { + _fileAnalyzers = fileAnalyzers; + } + + /// + /// Analyzes whether the file content is considered safe with registered IFileStreamSecurityAnalyzers + /// + /// Needs to be a Read seekable stream + /// Whether the file is considered safe after running the necessary analyzers + public bool IsConsideredSafe(Stream fileStream) + { + foreach (var fileAnalyzer in _fileAnalyzers) + { + fileStream.Seek(0, SeekOrigin.Begin); + if (!fileAnalyzer.ShouldHandle(fileStream)) + { + continue; + } + + fileStream.Seek(0, SeekOrigin.Begin); + if (fileAnalyzer.IsConsideredSafe(fileStream) == false) + { + return false; + } + } + fileStream.Seek(0, SeekOrigin.Begin); + // If no analyzer we consider the file to be safe as the implementer has the possibility to add additional analyzers + // Or all analyzers deem te file to be safe + return true; + } +} diff --git a/src/Umbraco.Core/Security/IFileStreamSecurityAnalyzer.cs b/src/Umbraco.Core/Security/IFileStreamSecurityAnalyzer.cs new file mode 100644 index 0000000000..408f161f21 --- /dev/null +++ b/src/Umbraco.Core/Security/IFileStreamSecurityAnalyzer.cs @@ -0,0 +1,20 @@ +namespace Umbraco.Cms.Core.Security; + +public interface IFileStreamSecurityAnalyzer +{ + + /// + /// Indicates whether the analyzer should process the file + /// The implementation should be considerably faster than IsConsideredSafe + /// + /// + /// + bool ShouldHandle(Stream fileStream); + + /// + /// Analyzes whether the file content is considered safe + /// + /// Needs to be a Read/Write seekable stream + /// Whether the file is considered safe + bool IsConsideredSafe(Stream fileStream); +} diff --git a/src/Umbraco.Core/Security/IFileStreamSecurityValidator.cs b/src/Umbraco.Core/Security/IFileStreamSecurityValidator.cs new file mode 100644 index 0000000000..9dc60507ac --- /dev/null +++ b/src/Umbraco.Core/Security/IFileStreamSecurityValidator.cs @@ -0,0 +1,11 @@ +namespace Umbraco.Cms.Core.Security; + +public interface IFileStreamSecurityValidator +{ + /// + /// Analyzes wether the file content is considered safe with registered IFileStreamSecurityAnalyzers + /// + /// Needs to be a Read seekable stream + /// Whether the file is considered safe after running the necessary analyzers + bool IsConsideredSafe(Stream fileStream); +} diff --git a/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs index 5a14a1afc1..34a4d33fd9 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 IFileStreamSecurityValidator _fileStreamSecurityValidator; private ContentSettings _contentSettings; public FileUploadPropertyValueEditor( @@ -27,10 +29,12 @@ internal class FileUploadPropertyValueEditor : DataValueEditor IShortStringHelper shortStringHelper, IOptionsMonitor contentSettings, IJsonSerializer jsonSerializer, - IIOHelper ioHelper) + IIOHelper ioHelper, + IFileStreamSecurityValidator fileStreamSecurityValidator) : base(localizedTextService, shortStringHelper, jsonSerializer, ioHelper, attribute) { _mediaFileManager = mediaFileManager ?? throw new ArgumentNullException(nameof(mediaFileManager)); + _fileStreamSecurityValidator = fileStreamSecurityValidator; _contentSettings = contentSettings.CurrentValue ?? throw new ArgumentNullException(nameof(contentSettings)); contentSettings.OnChange(x => _contentSettings = x); } @@ -147,6 +151,11 @@ internal class FileUploadPropertyValueEditor : DataValueEditor using (FileStream filestream = File.OpenRead(file.TempFilePath)) { + if (_fileStreamSecurityValidator.IsConsideredSafe(filestream) == 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..2c44ef57e3 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 IFileStreamSecurityValidator _fileStreamSecurityValidator; 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, + IFileStreamSecurityValidator fileStreamSecurityValidator) : 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; + _fileStreamSecurityValidator = fileStreamSecurityValidator; contentSettings.OnChange(x => _contentSettings = x); } @@ -236,6 +240,11 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v using (FileStream filestream = File.OpenRead(file.TempFilePath)) { + if (_fileStreamSecurityValidator.IsConsideredSafe(filestream) == 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/CurrentUserController.cs b/src/Umbraco.Web.BackOffice/Controllers/CurrentUserController.cs index f867ccc5a1..4992c4921e 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/CurrentUserController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/CurrentUserController.cs @@ -48,9 +48,43 @@ public class CurrentUserController : UmbracoAuthorizedJsonController private readonly IShortStringHelper _shortStringHelper; private readonly IUmbracoMapper _umbracoMapper; private readonly IUserDataService _userDataService; + private readonly IFileStreamSecurityValidator? _fileStreamSecurityValidator; // make non nullable in v14 private readonly IUserService _userService; [ActivatorUtilitiesConstructor] + public CurrentUserController( + MediaFileManager mediaFileManager, + IOptionsSnapshot contentSettings, + IHostingEnvironment hostingEnvironment, + IImageUrlGenerator imageUrlGenerator, + IBackOfficeSecurityAccessor backofficeSecurityAccessor, + IUserService userService, + IUmbracoMapper umbracoMapper, + IBackOfficeUserManager backOfficeUserManager, + ILocalizedTextService localizedTextService, + AppCaches appCaches, + IShortStringHelper shortStringHelper, + IPasswordChanger passwordChanger, + IUserDataService userDataService, + IFileStreamSecurityValidator fileStreamSecurityValidator) + { + _mediaFileManager = mediaFileManager; + _contentSettings = contentSettings.Value; + _hostingEnvironment = hostingEnvironment; + _imageUrlGenerator = imageUrlGenerator; + _backofficeSecurityAccessor = backofficeSecurityAccessor; + _userService = userService; + _umbracoMapper = umbracoMapper; + _backOfficeUserManager = backOfficeUserManager; + _localizedTextService = localizedTextService; + _appCaches = appCaches; + _shortStringHelper = shortStringHelper; + _passwordChanger = passwordChanger; + _userDataService = userDataService; + _fileStreamSecurityValidator = fileStreamSecurityValidator; + } + + [Obsolete("Use constructor overload that has fileStreamSecurityValidator, scheduled for removal in v14")] public CurrentUserController( MediaFileManager mediaFileManager, IOptionsSnapshot contentSettings, @@ -285,6 +319,7 @@ public class CurrentUserController : UmbracoAuthorizedJsonController _contentSettings, _hostingEnvironment, _imageUrlGenerator, + _fileStreamSecurityValidator, _backofficeSecurityAccessor.BackOfficeSecurity?.GetUserId().Result ?? 0); } diff --git a/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs b/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs index a26934b3c1..807061e5aa 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs @@ -5,6 +5,7 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Infrastructure; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Microsoft.Extensions.Primitives; @@ -50,6 +51,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers; public class MediaController : ContentControllerBase { private readonly AppCaches _appCaches; + private readonly IFileStreamSecurityValidator? _fileStreamSecurityValidator; // make non nullable in v14 private readonly IAuthorizationService _authorizationService; private readonly IBackOfficeSecurityAccessor _backofficeSecurityAccessor; private readonly ContentSettings _contentSettings; @@ -66,6 +68,58 @@ public class MediaController : ContentControllerBase private readonly ISqlContext _sqlContext; private readonly IUmbracoMapper _umbracoMapper; + [ActivatorUtilitiesConstructor] + public MediaController( + ICultureDictionary cultureDictionary, + ILoggerFactory loggerFactory, + IShortStringHelper shortStringHelper, + IEventMessagesFactory eventMessages, + ILocalizedTextService localizedTextService, + IOptionsSnapshot contentSettings, + IMediaTypeService mediaTypeService, + IMediaService mediaService, + IEntityService entityService, + IBackOfficeSecurityAccessor backofficeSecurityAccessor, + IUmbracoMapper umbracoMapper, + IDataTypeService dataTypeService, + ISqlContext sqlContext, + IContentTypeBaseServiceProvider contentTypeBaseServiceProvider, + IRelationService relationService, + PropertyEditorCollection propertyEditors, + MediaFileManager mediaFileManager, + MediaUrlGeneratorCollection mediaUrlGenerators, + IHostingEnvironment hostingEnvironment, + IImageUrlGenerator imageUrlGenerator, + IJsonSerializer serializer, + IAuthorizationService authorizationService, + AppCaches appCaches, + IFileStreamSecurityValidator streamSecurityValidator) + : base(cultureDictionary, loggerFactory, shortStringHelper, eventMessages, localizedTextService, serializer) + { + _shortStringHelper = shortStringHelper; + _contentSettings = contentSettings.Value; + _mediaTypeService = mediaTypeService; + _mediaService = mediaService; + _entityService = entityService; + _backofficeSecurityAccessor = backofficeSecurityAccessor; + _umbracoMapper = umbracoMapper; + _dataTypeService = dataTypeService; + _localizedTextService = localizedTextService; + _sqlContext = sqlContext; + _contentTypeBaseServiceProvider = contentTypeBaseServiceProvider; + _relationService = relationService; + _propertyEditors = propertyEditors; + _mediaFileManager = mediaFileManager; + _mediaUrlGenerators = mediaUrlGenerators; + _hostingEnvironment = hostingEnvironment; + _logger = loggerFactory.CreateLogger(); + _imageUrlGenerator = imageUrlGenerator; + _authorizationService = authorizationService; + _appCaches = appCaches; + _fileStreamSecurityValidator = streamSecurityValidator; + } + + [Obsolete("Use constructor overload that has fileStreamSecurityValidator, scheduled for removal in v14")] public MediaController( ICultureDictionary cultureDictionary, ILoggerFactory loggerFactory, @@ -724,6 +778,17 @@ public class MediaController : ContentControllerBase continue; } + using var stream = new MemoryStream(); + await formFile.CopyToAsync(stream); + if (_fileStreamSecurityValidator != null && _fileStreamSecurityValidator.IsConsideredSafe(stream) == false) + { + tempFiles.Notifications.Add(new BackOfficeNotification( + _localizedTextService.Localize("speechBubbles", "operationFailedHeader"), + _localizedTextService.Localize("media", "fileSecurityValidationFailure"), + NotificationStyle.Warning)); + continue; + } + if (string.IsNullOrEmpty(mediaTypeAlias)) { mediaTypeAlias = Constants.Conventions.MediaTypes.File; @@ -801,11 +866,8 @@ public class MediaController : ContentControllerBase IMedia createdMediaItem = _mediaService.CreateMedia(mediaItemName, parentId.Value, mediaTypeAlias, _backofficeSecurityAccessor.BackOfficeSecurity?.CurrentUser?.Id ?? -1); - await using (Stream stream = formFile.OpenReadStream()) - { createdMediaItem.SetValue(_mediaFileManager, _mediaUrlGenerators, _shortStringHelper, _contentTypeBaseServiceProvider, Constants.Conventions.Media.File, fileName, stream); - } Attempt saveResult = _mediaService.Save(createdMediaItem, _backofficeSecurityAccessor.BackOfficeSecurity?.CurrentUser?.Id ?? -1); diff --git a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs index 1a8be10a5c..d8e6aa8924 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs @@ -55,6 +55,7 @@ public class UsersController : BackOfficeNotificationsController private readonly GlobalSettings _globalSettings; private readonly IHostingEnvironment _hostingEnvironment; private readonly IHttpContextAccessor _httpContextAccessor; + private readonly IFileStreamSecurityValidator? _fileStreamSecurityValidator; // make non nullable in v14 private readonly IImageUrlGenerator _imageUrlGenerator; private readonly LinkGenerator _linkGenerator; private readonly ILocalizedTextService _localizedTextService; @@ -72,6 +73,58 @@ public class UsersController : BackOfficeNotificationsController private readonly WebRoutingSettings _webRoutingSettings; [ActivatorUtilitiesConstructor] + public UsersController( + MediaFileManager mediaFileManager, + IOptionsSnapshot contentSettings, + IHostingEnvironment hostingEnvironment, + ISqlContext sqlContext, + IImageUrlGenerator imageUrlGenerator, + IOptionsSnapshot securitySettings, + IEmailSender emailSender, + IBackOfficeSecurityAccessor backofficeSecurityAccessor, + AppCaches appCaches, + IShortStringHelper shortStringHelper, + IUserService userService, + ILocalizedTextService localizedTextService, + IUmbracoMapper umbracoMapper, + IOptionsSnapshot globalSettings, + IBackOfficeUserManager backOfficeUserManager, + ILoggerFactory loggerFactory, + LinkGenerator linkGenerator, + IBackOfficeExternalLoginProviders externalLogins, + UserEditorAuthorizationHelper userEditorAuthorizationHelper, + IPasswordChanger passwordChanger, + IHttpContextAccessor httpContextAccessor, + IOptions webRoutingSettings, + IFileStreamSecurityValidator fileStreamSecurityValidator) + { + _mediaFileManager = mediaFileManager; + _contentSettings = contentSettings.Value; + _hostingEnvironment = hostingEnvironment; + _sqlContext = sqlContext; + _imageUrlGenerator = imageUrlGenerator; + _securitySettings = securitySettings.Value; + _emailSender = emailSender; + _backofficeSecurityAccessor = backofficeSecurityAccessor; + _appCaches = appCaches; + _shortStringHelper = shortStringHelper; + _userService = userService; + _localizedTextService = localizedTextService; + _umbracoMapper = umbracoMapper; + _globalSettings = globalSettings.Value; + _userManager = backOfficeUserManager; + _loggerFactory = loggerFactory; + _linkGenerator = linkGenerator; + _externalLogins = externalLogins; + _userEditorAuthorizationHelper = userEditorAuthorizationHelper; + _passwordChanger = passwordChanger; + _logger = _loggerFactory.CreateLogger(); + _httpContextAccessor = httpContextAccessor; + _fileStreamSecurityValidator = fileStreamSecurityValidator; + _webRoutingSettings = webRoutingSettings.Value; + } + + [Obsolete("Use constructor overload that has fileStreamSecurityValidator, scheduled for removal in v14")] public UsersController( MediaFileManager mediaFileManager, IOptionsSnapshot contentSettings, @@ -139,13 +192,15 @@ public class UsersController : BackOfficeNotificationsController [AppendUserModifiedHeader("id")] [Authorize(Policy = AuthorizationPolicies.AdminUserEditsRequireAdmin)] - public IActionResult PostSetAvatar(int id, IList file) => PostSetAvatarInternal(file, _userService, + public IActionResult PostSetAvatar(int id, IList file) + => PostSetAvatarInternal(file, _userService, _appCaches.RuntimeCache, _mediaFileManager, _shortStringHelper, _contentSettings, _hostingEnvironment, - _imageUrlGenerator, id); + _imageUrlGenerator,_fileStreamSecurityValidator, id); internal static IActionResult PostSetAvatarInternal(IList files, IUserService userService, IAppCache cache, MediaFileManager mediaFileManager, IShortStringHelper shortStringHelper, ContentSettings contentSettings, IHostingEnvironment hostingEnvironment, IImageUrlGenerator imageUrlGenerator, + IFileStreamSecurityValidator? fileStreamSecurityValidator, int id) { if (files is null) @@ -187,9 +242,14 @@ 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; - using (Stream fs = file.OpenReadStream()) + //todo implement Filestreamsecurity + using (var ms = new MemoryStream()) { - mediaFileManager.FileSystem.AddFile(user.Avatar, fs, true); + file.CopyTo(ms); + if(fileStreamSecurityValidator != null && fileStreamSecurityValidator.IsConsideredSafe(ms) == false) + return new ValidationErrorResult("One or more file security analyzers deemed the contents of the file to be unsafe"); + + mediaFileManager.FileSystem.AddFile(user.Avatar, ms, true); } userService.Save(user); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/FileStreamSecurityValidatorTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/FileStreamSecurityValidatorTests.cs new file mode 100644 index 0000000000..7cdee69daa --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/FileStreamSecurityValidatorTests.cs @@ -0,0 +1,123 @@ +using Moq; +using NUnit.Framework; +using Umbraco.Cms.Core.Security; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Security; + +public class FileStreamSecurityValidatorTests +{ + [Test] + public void IsConsideredSafe_True_NoAnalyzersPresent() + { + // Arrange + var sut = new FileStreamSecurityValidator(Enumerable.Empty()); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsTrue(validationResult); + } + + [Test] + public void IsConsideredSafe_True_NoAnalyzerMatchesType() + { + // Arrange + var analyzerOne = new Mock(); + analyzerOne.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + var analyzerTwo = new Mock(); + analyzerTwo.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + + var sut = new FileStreamSecurityValidator(new List{analyzerOne.Object,analyzerTwo.Object}); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsTrue(validationResult); + } + + [Test] + public void IsConsideredSafe_True_AllMatchingAnalyzersReturnTrue() + { + // Arrange + var matchingAnalyzerOne = new Mock(); + matchingAnalyzerOne.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + matchingAnalyzerOne.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(true); + + var matchingAnalyzerTwo = new Mock(); + matchingAnalyzerTwo.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + matchingAnalyzerTwo.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(true); + + var unmatchedAnalyzer = new Mock(); + unmatchedAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + + var sut = new FileStreamSecurityValidator(new List{matchingAnalyzerOne.Object,matchingAnalyzerTwo.Object}); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsTrue(validationResult); + } + + [Test] + public void IsConsideredSafe_False_AnyMatchingAnalyzersReturnFalse() + { + // Arrange + var saveMatchingAnalyzer = new Mock(); + saveMatchingAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + saveMatchingAnalyzer.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(true); + + var unsafeMatchingAnalyzer = new Mock(); + unsafeMatchingAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + unsafeMatchingAnalyzer.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(false); + + var unmatchedAnalyzer = new Mock(); + unmatchedAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + + var sut = new FileStreamSecurityValidator(new List{saveMatchingAnalyzer.Object,unsafeMatchingAnalyzer.Object}); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsFalse(validationResult); + } +} From 0dc18982ee9f2e078334b5a28de0908e79195176 Mon Sep 17 00:00:00 2001 From: Kenn Jacobsen Date: Mon, 21 Aug 2023 13:17:15 +0200 Subject: [PATCH 2/2] Do not allow content type property aliases that conflict with IPublishedElement (#14683) --- src/Umbraco.Core/Extensions/TypeExtensions.cs | 159 +++++++++--------- .../ContentTypeModelValidatorBase.cs | 6 +- 2 files changed, 82 insertions(+), 83 deletions(-) diff --git a/src/Umbraco.Core/Extensions/TypeExtensions.cs b/src/Umbraco.Core/Extensions/TypeExtensions.cs index e3da8d9ee1..1416888e73 100644 --- a/src/Umbraco.Core/Extensions/TypeExtensions.cs +++ b/src/Umbraco.Core/Extensions/TypeExtensions.cs @@ -219,96 +219,52 @@ public static class TypeExtensions /// public static PropertyInfo[] GetAllProperties(this Type type) { - if (type.IsInterface) - { - var propertyInfos = new List(); - - var considered = new List(); - var queue = new Queue(); - considered.Add(type); - queue.Enqueue(type); - while (queue.Count > 0) - { - Type subType = queue.Dequeue(); - foreach (Type subInterface in subType.GetInterfaces()) - { - if (considered.Contains(subInterface)) - { - continue; - } - - considered.Add(subInterface); - queue.Enqueue(subInterface); - } - - PropertyInfo[] typeProperties = subType.GetProperties( - BindingFlags.FlattenHierarchy - | BindingFlags.Public - | BindingFlags.NonPublic - | BindingFlags.Instance); - - IEnumerable newPropertyInfos = typeProperties - .Where(x => !propertyInfos.Contains(x)); - - propertyInfos.InsertRange(0, newPropertyInfos); - } - - return propertyInfos.ToArray(); - } - - return type.GetProperties(BindingFlags.FlattenHierarchy - | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance); + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.NonPublic + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetProperties(bindingFlags)); } /// - /// Returns all public properties including inherited properties even for interfaces + /// Returns public properties including inherited properties even for interfaces /// /// /// - /// - /// taken from - /// http://stackoverflow.com/questions/358835/getproperties-to-return-all-properties-for-an-interface-inheritance-hierarchy - /// public static PropertyInfo[] GetPublicProperties(this Type type) { - if (type.IsInterface) - { - var propertyInfos = new List(); + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetProperties(bindingFlags)); + } - var considered = new List(); - var queue = new Queue(); - considered.Add(type); - queue.Enqueue(type); - while (queue.Count > 0) - { - Type subType = queue.Dequeue(); - foreach (Type subInterface in subType.GetInterfaces()) - { - if (considered.Contains(subInterface)) - { - continue; - } + /// + /// Returns public methods including inherited methods even for interfaces + /// + /// + /// + public static MethodInfo[] GetPublicMethods(this Type type) + { + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetMethods(bindingFlags)); + } - considered.Add(subInterface); - queue.Enqueue(subInterface); - } - - PropertyInfo[] typeProperties = subType.GetProperties( - BindingFlags.FlattenHierarchy - | BindingFlags.Public - | BindingFlags.Instance); - - IEnumerable newPropertyInfos = typeProperties - .Where(x => !propertyInfos.Contains(x)); - - propertyInfos.InsertRange(0, newPropertyInfos); - } - - return propertyInfos.ToArray(); - } - - return type.GetProperties(BindingFlags.FlattenHierarchy - | BindingFlags.Public | BindingFlags.Instance); + /// + /// Returns all methods including inherited methods even for interfaces + /// + /// Includes both Public and Non-Public methods + /// + /// + public static MethodInfo[] GetAllMethods(this Type type) + { + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.NonPublic + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetMethods(bindingFlags)); } /// @@ -512,4 +468,47 @@ public static class TypeExtensions return attempt; } + + /// + /// taken from + /// http://stackoverflow.com/questions/358835/getproperties-to-return-all-properties-for-an-interface-inheritance-hierarchy + /// + private static T[] GetAllMemberInfos(this Type type, Func getMemberInfos) + where T : MemberInfo + { + if (type.IsInterface is false) + { + return getMemberInfos(type); + } + + var memberInfos = new List(); + + var considered = new List(); + var queue = new Queue(); + considered.Add(type); + queue.Enqueue(type); + while (queue.Count > 0) + { + Type subType = queue.Dequeue(); + foreach (Type subInterface in subType.GetInterfaces()) + { + if (considered.Contains(subInterface)) + { + continue; + } + + considered.Add(subInterface); + queue.Enqueue(subInterface); + } + + T[] typeMethodInfos = getMemberInfos(subType); + + IEnumerable newMethodInfos = typeMethodInfos + .Where(x => !memberInfos.Contains(x)); + + memberInfos.InsertRange(0, newMethodInfos); + } + + return memberInfos.ToArray(); + } } diff --git a/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs b/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs index ef59126c1c..47ad9e4e80 100644 --- a/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs +++ b/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs @@ -68,10 +68,10 @@ public abstract class ContentTypeModelValidatorBase : EditorV private ValidationResult? ValidateProperty(PropertyTypeBasic property, int groupIndex, int propertyIndex) { - // don't let them match any properties or methods in IPublishedContent + // don't let them match any properties or methods in IPublishedContent (including those defined in any base interfaces like IPublishedElement) // TODO: There are probably more! - var reservedProperties = typeof(IPublishedContent).GetProperties().Select(x => x.Name).ToArray(); - var reservedMethods = typeof(IPublishedContent).GetMethods().Select(x => x.Name).ToArray(); + var reservedProperties = typeof(IPublishedContent).GetPublicProperties().Select(x => x.Name).ToArray(); + var reservedMethods = typeof(IPublishedContent).GetPublicMethods().Select(x => x.Name).ToArray(); var alias = property.Alias;