From 0ac6091b74a99bff89992ed564caedf1b87dd851 Mon Sep 17 00:00:00 2001 From: Renaud Paquay Date: Wed, 25 May 2011 17:36:27 -0700 Subject: [PATCH] 17875: Too many InvalidOperationExceptions during startup Prevent common cases exception by explicitly rejecting invalid virtual paths. --HG-- branch : 1.x --- .../DefaultVirtualPathProviderTests.cs | 40 ++++++-- .../Loaders/DynamicExtensionLoader.cs | 9 +- .../VirtualPath/DefaultVirtualPathMonitor.cs | 2 +- .../VirtualPath/DefaultVirtualPathProvider.cs | 96 ++++++++++++++----- 4 files changed, 108 insertions(+), 39 deletions(-) diff --git a/src/Orchard.Tests/FileSystems/VirtualPath/DefaultVirtualPathProviderTests.cs b/src/Orchard.Tests/FileSystems/VirtualPath/DefaultVirtualPathProviderTests.cs index 2c38f595e..f0e1300da 100644 --- a/src/Orchard.Tests/FileSystems/VirtualPath/DefaultVirtualPathProviderTests.cs +++ b/src/Orchard.Tests/FileSystems/VirtualPath/DefaultVirtualPathProviderTests.cs @@ -9,13 +9,39 @@ namespace Orchard.Tests.FileSystems.VirtualPath { public void TryFileExistsTest() { StubDefaultVirtualPathProvider defaultVirtualPathProvider = new StubDefaultVirtualPathProvider(); - Assert.That(defaultVirtualPathProvider.TryFileExists("~\\a.txt"), Is.True); - Assert.That(defaultVirtualPathProvider.TryFileExists("~\\..\\a.txt"), Is.False); - Assert.That(defaultVirtualPathProvider.TryFileExists("~\\a\\..\\a.txt"), Is.True); - Assert.That(defaultVirtualPathProvider.TryFileExists("~\\a\\b\\..\\a.txt"), Is.True); - Assert.That(defaultVirtualPathProvider.TryFileExists("~\\a\\b\\..\\..\\a.txt"), Is.True); - Assert.That(defaultVirtualPathProvider.TryFileExists("~\\a\\b\\..\\..\\..\\a.txt"), Is.False); - Assert.That(defaultVirtualPathProvider.TryFileExists("~\\a\\..\\..\\b\\c.txt"), Is.False); + Assert.That(defaultVirtualPathProvider.TryFileExists("~/a.txt"), Is.True); + Assert.That(defaultVirtualPathProvider.TryFileExists("~/../a.txt"), Is.False); + Assert.That(defaultVirtualPathProvider.TryFileExists("~/a/../a.txt"), Is.True); + Assert.That(defaultVirtualPathProvider.TryFileExists("~/a/b/../a.txt"), Is.True); + Assert.That(defaultVirtualPathProvider.TryFileExists("~/a/b/../../a.txt"), Is.True); + Assert.That(defaultVirtualPathProvider.TryFileExists("~/a/b/../../../a.txt"), Is.False); + Assert.That(defaultVirtualPathProvider.TryFileExists("~/a/../../b/c.txt"), Is.False); + } + + [Test] + public void RejectMalformedVirtualPathTests() { + StubDefaultVirtualPathProvider defaultVirtualPathProvider = new StubDefaultVirtualPathProvider(); + + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("~/a.txt"), Is.False); + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("/a.txt"), Is.False); + + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("~/../a.txt"), Is.True); + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("/../a.txt"), Is.True); + + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("~/a/../a.txt"), Is.False); + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("/a/../a.txt"), Is.False); + + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("~/a/b/../a.txt"), Is.False); + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("/a/b/../a.txt"), Is.False); + + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("~/a/b/../../a.txt"), Is.False); + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("/a/b/../../a.txt"), Is.False); + + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("~/a/b/../../../a.txt"), Is.True); + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("/a/b/../../../a.txt"), Is.True); + + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("~/a/../../b//.txt"), Is.True); + Assert.That(defaultVirtualPathProvider.RejectMalformedVirtualPath("/a/../../b//.txt"), Is.True); } } diff --git a/src/Orchard/Environment/Extensions/Loaders/DynamicExtensionLoader.cs b/src/Orchard/Environment/Extensions/Loaders/DynamicExtensionLoader.cs index 8d7abcccd..63005e5f9 100644 --- a/src/Orchard/Environment/Extensions/Loaders/DynamicExtensionLoader.cs +++ b/src/Orchard/Environment/Extensions/Loaders/DynamicExtensionLoader.cs @@ -207,14 +207,7 @@ namespace Orchard.Environment.Extensions.Loaders { // Normalize the virtual path (avoid ".." in the path name) if (!string.IsNullOrEmpty(path)) { - try { - path = _virtualPathProvider.ToAppRelative(path); - } - catch (Exception e) { - // The initial path might have been invalid (e.g. path indicates a path outside the application root) - Logger.Information(e, "Path '{0}' cannot be made app relative", path); - path = null; - } + path = _virtualPathProvider.ToAppRelative(path); } // Attempt to reference the project / library file diff --git a/src/Orchard/FileSystems/VirtualPath/DefaultVirtualPathMonitor.cs b/src/Orchard/FileSystems/VirtualPath/DefaultVirtualPathMonitor.cs index b5a5db7ef..d6bd4c81d 100644 --- a/src/Orchard/FileSystems/VirtualPath/DefaultVirtualPathMonitor.cs +++ b/src/Orchard/FileSystems/VirtualPath/DefaultVirtualPathMonitor.cs @@ -31,7 +31,7 @@ namespace Orchard.FileSystems.VirtualPath { catch (HttpException e) { // This exception happens if trying to monitor a directory or file // inside a directory which doesn't exist - Logger.Warning(e, "Error monitoring file changes on virtual path '{0}'", virtualPath); + Logger.Information(e, "Error monitoring file changes on virtual path '{0}'", virtualPath); //TODO: Return a token monitoring first existing parent directory. } diff --git a/src/Orchard/FileSystems/VirtualPath/DefaultVirtualPathProvider.cs b/src/Orchard/FileSystems/VirtualPath/DefaultVirtualPathProvider.cs index 3678ec65b..cd5fea096 100644 --- a/src/Orchard/FileSystems/VirtualPath/DefaultVirtualPathProvider.cs +++ b/src/Orchard/FileSystems/VirtualPath/DefaultVirtualPathProvider.cs @@ -4,9 +4,16 @@ using System.IO; using System.Linq; using System.Web; using System.Web.Hosting; +using Orchard.Logging; namespace Orchard.FileSystems.VirtualPath { public class DefaultVirtualPathProvider : IVirtualPathProvider { + public DefaultVirtualPathProvider() { + Logger = NullLogger.Instance; + } + + public ILogger Logger { get; set; } + public virtual string GetDirectoryName(string virtualPath) { return Path.GetDirectoryName(virtualPath).Replace(Path.DirectorySeparatorChar, '/'); } @@ -17,7 +24,7 @@ namespace Orchard.FileSystems.VirtualPath { .GetDirectory(path) .Files .OfType() - .Select(f => ToAppRelative(f.VirtualPath)); + .Select(f => VirtualPathUtility.ToAppRelative(f.VirtualPath)); } public virtual IEnumerable ListDirectories(string path) { @@ -26,7 +33,7 @@ namespace Orchard.FileSystems.VirtualPath { .GetDirectory(path) .Directories .OfType() - .Select(d => ToAppRelative(d.VirtualPath)); + .Select(d => VirtualPathUtility.ToAppRelative(d.VirtualPath)); } public virtual string Combine(params string[] paths) { @@ -34,7 +41,65 @@ namespace Orchard.FileSystems.VirtualPath { } public virtual string ToAppRelative(string virtualPath) { - return VirtualPathUtility.ToAppRelative(virtualPath); + if (RejectMalformedVirtualPath(virtualPath)) + return null; + + try { + string result = VirtualPathUtility.ToAppRelative(virtualPath); + + // In some cases, ToAppRelative doesn't normalize the path. In those cases, + // the path is invalid. + // Example: + // ApplicationPath: /Foo + // VirtualPath : ~/Bar/../Blah/Blah2 + // Result : /Blah/Blah2 <= that is not an app relative path! + if (!result.StartsWith("~/")) { + Logger.Information("Path '{0}' cannot be made app relative: Path returned ('{1}') is not app relative.", virtualPath, result); + return null; + } + return result; + } + catch (Exception e) { + // The initial path might have been invalid (e.g. path indicates a path outside the application root) + Logger.Information(e, "Path '{0}' cannot be made app relative", virtualPath); + return null; + } + } + + /// + /// We want to reject path that contains ".." going outside of the application root. + /// ToAppRelative does that already, but we want to do the same while avoiding exceptions. + /// + /// Note: This method doesn't detect all cases of malformed paths, it merely checks + /// for *some* cases of malformed paths, so this is not a replacement for full virtual path + /// verification through VirtualPathUtilty methods. + /// + public bool RejectMalformedVirtualPath(string virtualPath) { + if (string.IsNullOrEmpty(virtualPath)) + return true; + + if (virtualPath.IndexOf("..") >= 0) { + virtualPath = virtualPath.Replace(Path.DirectorySeparatorChar, '/'); + string rootPrefix = virtualPath.StartsWith("~/") ? "~/" : virtualPath.StartsWith("/") ? "/" : ""; + if (!string.IsNullOrEmpty(rootPrefix)) { + string[] terms = virtualPath.Substring(rootPrefix.Length).Split('/'); + int depth = 0; + foreach (var term in terms) { + if (term == "..") { + if (depth == 0) { + Logger.Information("Path '{0}' cannot be made app relative: Too many '..'", virtualPath); + return true; + } + depth--; + } + else { + depth++; + } + } + } + } + + return false; } public virtual Stream OpenFile(string virtualPath) { @@ -62,29 +127,14 @@ namespace Orchard.FileSystems.VirtualPath { } public virtual bool TryFileExists(string virtualPath) { + if (RejectMalformedVirtualPath(virtualPath)) + return false; + try { - // Check if the path falls outside the root directory of the app - string directoryName = Path.GetDirectoryName(virtualPath); - - int level = 0; - int stringLength = directoryName.Count(); - - for(int i = 0 ; i < stringLength ; i++) { - if (directoryName[i] == '\\') { - if (i < (stringLength - 2) && directoryName[i + 1] == '.' && directoryName[i + 2] == '.') { - level--; - i += 2; - } else level++; - } - - if (level < 0) { - return false; - } - } - return FileExists(virtualPath); } - catch { + catch (Exception e) { + Logger.Information(e, "File '{0}' can not be checked for exitence. Assuming doesn't exist.", virtualPath); return false; } }