diff --git a/src/Umbraco.Core/IO/IOHelper.cs b/src/Umbraco.Core/IO/IOHelper.cs index 2d87f634f4..8378081394 100644 --- a/src/Umbraco.Core/IO/IOHelper.cs +++ b/src/Umbraco.Core/IO/IOHelper.cs @@ -152,12 +152,7 @@ namespace Umbraco.Core.IO /// A value indicating whether the filepath is valid. internal static bool VerifyEditPath(string filePath, string validDir) { - if (filePath.StartsWith(MapPath(SystemDirectories.Root)) == false) - filePath = MapPath(filePath); - if (validDir.StartsWith(MapPath(SystemDirectories.Root)) == false) - validDir = MapPath(validDir); - - return filePath.StartsWith(validDir); + return VerifyEditPath(filePath, new[] { validDir }); } /// @@ -182,12 +177,17 @@ namespace Umbraco.Core.IO /// A value indicating whether the filepath is valid. internal static bool VerifyEditPath(string filePath, IEnumerable validDirs) { + var mappedRoot = MapPath(SystemDirectories.Root); + if (filePath.StartsWith(mappedRoot) == false) + filePath = MapPath(filePath); + + // don't trust what we get, it may contain relative segments + filePath = Path.GetFullPath(filePath); + foreach (var dir in validDirs) { var validDir = dir; - if (filePath.StartsWith(MapPath(SystemDirectories.Root)) == false) - filePath = MapPath(filePath); - if (validDir.StartsWith(MapPath(SystemDirectories.Root)) == false) + if (validDir.StartsWith(mappedRoot) == false) validDir = MapPath(validDir); if (filePath.StartsWith(validDir)) @@ -219,11 +219,8 @@ namespace Umbraco.Core.IO /// A value indicating whether the filepath is valid. internal static bool VerifyFileExtension(string filePath, List validFileExtensions) { - if (filePath.StartsWith(MapPath(SystemDirectories.Root)) == false) - filePath = MapPath(filePath); - var f = new FileInfo(filePath); - - return validFileExtensions.Contains(f.Extension.Substring(1)); + var ext = Path.GetExtension(filePath); + return ext != null && validFileExtensions.Contains(ext.TrimStart('.')); } /// diff --git a/src/Umbraco.Core/IO/PhysicalFileSystem.cs b/src/Umbraco.Core/IO/PhysicalFileSystem.cs index 13df315960..9bef38b68f 100644 --- a/src/Umbraco.Core/IO/PhysicalFileSystem.cs +++ b/src/Umbraco.Core/IO/PhysicalFileSystem.cs @@ -177,9 +177,23 @@ namespace Umbraco.Core.IO path = GetRelativePath(path); } - return !path.StartsWith(RootPath) - ? Path.Combine(RootPath, path) - : path; + // if already a full path, return + if (path.StartsWith(RootPath)) + return path; + + // else combine and sanitize, ie GetFullPath will take care of any relative + // segments in path, eg '../../foo.tmp' - it may throw a SecurityException + // if the combined path reaches illegal parts of the filesystem + var fpath = Path.Combine(RootPath, path); + fpath = Path.GetFullPath(fpath); + + // at that point, path is within legal parts of the filesystem, ie we have + // permissions to reach that path, but it may nevertheless be outside of + // our root path, due to relative segments, so better check + if (fpath.StartsWith(RootPath)) + return fpath; + + throw new FileSecurityException("File '" + path + "' is outside this filesystem's root."); } public string GetUrl(string path) diff --git a/src/Umbraco.Core/Persistence/Repositories/ScriptRepository.cs b/src/Umbraco.Core/Persistence/Repositories/ScriptRepository.cs index b48e096e3f..f8fc459aec 100644 --- a/src/Umbraco.Core/Persistence/Repositories/ScriptRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/ScriptRepository.cs @@ -105,12 +105,22 @@ namespace Umbraco.Core.Persistence.Repositories dirs += "," + SystemDirectories.MvcViews;*/ //Validate file - var validFile = IOHelper.VerifyEditPath(script.VirtualPath, dirs.Split(',')); + string fullPath; + try + { + // may throw for security reasons + fullPath = FileSystem.GetFullPath(script.Path); + } + catch + { + return false; + } + var isValidPath = IOHelper.VerifyEditPath(fullPath, dirs.Split(',')); //Validate extension - var validExtension = IOHelper.VerifyFileExtension(script.VirtualPath, exts); + var isValidExtension = IOHelper.VerifyFileExtension(script.Path, exts); - return validFile && validExtension; + return isValidPath && isValidExtension; } #endregion diff --git a/src/Umbraco.Core/Persistence/Repositories/StylesheetRepository.cs b/src/Umbraco.Core/Persistence/Repositories/StylesheetRepository.cs index f4d7dcda29..425f7c8253 100644 --- a/src/Umbraco.Core/Persistence/Repositories/StylesheetRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/StylesheetRepository.cs @@ -99,19 +99,29 @@ namespace Umbraco.Core.Persistence.Repositories return FileSystem.GetFiles(rootPath ?? string.Empty, "*.css").Select(Get); } + private static readonly List ValidExtensions = new List { "css" }; + public bool ValidateStylesheet(Stylesheet stylesheet) { var dirs = SystemDirectories.Css; //Validate file - var validFile = IOHelper.VerifyEditPath(stylesheet.VirtualPath, dirs.Split(',')); + string fullPath; + try + { + // may throw for security reasons + fullPath = FileSystem.GetFullPath(stylesheet.Path); + } + catch + { + return false; + } + var isValidPath = IOHelper.VerifyEditPath(fullPath, dirs.Split(',')); //Validate extension - var validExtension = IOHelper.VerifyFileExtension(stylesheet.VirtualPath, new List { "css" }); + var isValidExtension = IOHelper.VerifyFileExtension(stylesheet.Path, ValidExtensions); - var fileValid = validFile && validExtension; - - return fileValid; + return isValidPath && isValidExtension; } #endregion diff --git a/src/Umbraco.Tests/IO/PhysicalFileSystemTests.cs b/src/Umbraco.Tests/IO/PhysicalFileSystemTests.cs index 418cb3dda2..4ee178a954 100644 --- a/src/Umbraco.Tests/IO/PhysicalFileSystemTests.cs +++ b/src/Umbraco.Tests/IO/PhysicalFileSystemTests.cs @@ -27,6 +27,8 @@ namespace Umbraco.Tests.IO public void TearDown() { var path = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "FileSysTests"); + if (Directory.Exists(path) == false) return; + var files = Directory.GetFiles(path); foreach (var f in files) { @@ -39,5 +41,31 @@ namespace Umbraco.Tests.IO { return "/Media/" + path; } + + [Test] + public void GetFullPathTest() + { + // outside of tests, one initializes the PhysicalFileSystem with eg ~/Dir + // and then, rootPath = /path/to/Dir and rootUrl = /Dir/ + // here we initialize the PhysicalFileSystem with + // rootPath = /path/to/FileSysTests + // rootUrl = /Media/ + + var basePath = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "FileSysTests"); + + // ensure that GetFullPath + // - does return the proper full path + // - does properly normalize separators + // - does throw on invalid paths + + var path = _fileSystem.GetFullPath("foo.tmp"); + Assert.AreEqual(Path.Combine(basePath, @"foo.tmp"), path); + + path = _fileSystem.GetFullPath("foo/bar.tmp"); + Assert.AreEqual(Path.Combine(basePath, @"foo\bar.tmp"), path); + + // that path is invalid as it would be outside the root directory + Assert.Throws(() => _fileSystem.GetFullPath("../../foo.tmp")); + } } }