From 89f92accf49b7f73360bfdbd9403656a655f682f Mon Sep 17 00:00:00 2001 From: James South Date: Tue, 30 Sep 2014 18:55:19 +0100 Subject: [PATCH] Tightening security in processing module. Former-commit-id: fba04153d99c8f005f17b4244288b642c88eec85 --- .../Helpers/ImageHelpers.cs | 2 +- .../HttpModules/ImageProcessingModule.cs | 283 +++++++++--------- src/ImageProcessorConsole/Program.cs | 0 src/Images/Penguins-8.png.REMOVED.git-id | 2 +- 4 files changed, 148 insertions(+), 139 deletions(-) delete mode 100644 src/ImageProcessorConsole/Program.cs diff --git a/src/ImageProcessor.Web/Helpers/ImageHelpers.cs b/src/ImageProcessor.Web/Helpers/ImageHelpers.cs index ab93a43a3..2b7946d77 100644 --- a/src/ImageProcessor.Web/Helpers/ImageHelpers.cs +++ b/src/ImageProcessor.Web/Helpers/ImageHelpers.cs @@ -43,7 +43,7 @@ namespace ImageProcessor.Web.Helpers /// True the value contains a valid image extension, otherwise false. public static bool IsValidImageExtension(string fileName) { - return EndFormatRegex.IsMatch(fileName) || string.IsNullOrWhiteSpace(Path.GetExtension(fileName)); + return EndFormatRegex.IsMatch(fileName); } /// diff --git a/src/ImageProcessor.Web/HttpModules/ImageProcessingModule.cs b/src/ImageProcessor.Web/HttpModules/ImageProcessingModule.cs index bf4f2e754..9927e3330 100644 --- a/src/ImageProcessor.Web/HttpModules/ImageProcessingModule.cs +++ b/src/ImageProcessor.Web/HttpModules/ImageProcessingModule.cs @@ -270,183 +270,181 @@ namespace ImageProcessor.Web.HttpModules HttpRequest request = context.Request; IImageService currentService = this.GetImageServiceForRequest(request); - if (currentService == null) + if (currentService != null) { - throw new HttpException(500, "No ImageService found for current request."); - } - - bool isFileLocal = currentService.IsFileLocalService; - string requestPath = string.Empty; - string queryString = string.Empty; - string urlParameters = string.Empty; - - if (!isFileLocal) - { - // We need to split the querystring to get the actual values we want. - string urlDecode = HttpUtility.UrlDecode(request.QueryString.ToString()); + bool isFileLocal = currentService.IsFileLocalService; + string requestPath = string.Empty; + string queryString = string.Empty; + string urlParameters = string.Empty; - if (!string.IsNullOrWhiteSpace(urlDecode)) + if (!isFileLocal) { - // UrlDecode seems to mess up in some circumstance. - if (urlDecode.IndexOf("://", StringComparison.OrdinalIgnoreCase) == -1) + // We need to split the querystring to get the actual values we want. + string urlDecode = HttpUtility.UrlDecode(request.QueryString.ToString()); + + if (!string.IsNullOrWhiteSpace(urlDecode)) { - urlDecode = urlDecode.Replace(":/", "://"); - } + // UrlDecode seems to mess up in some circumstance. + if (urlDecode.IndexOf("://", StringComparison.OrdinalIgnoreCase) == -1) + { + urlDecode = urlDecode.Replace(":/", "://"); + } - string[] paths = urlDecode.Split('?'); + string[] paths = urlDecode.Split('?'); - requestPath = paths[0]; + requestPath = paths[0]; - // Handle extension-less urls. - if (paths.Length > 2) - { - queryString = paths[2]; - urlParameters = paths[1]; - } - else if (paths.Length > 1) - { - queryString = paths[1]; + // Handle extension-less urls. + if (paths.Length > 2) + { + queryString = paths[2]; + urlParameters = paths[1]; + } + else if (paths.Length > 1) + { + queryString = paths[1]; + } } } - } - else - { - requestPath = HostingEnvironment.MapPath(request.Path); - queryString = HttpUtility.UrlDecode(request.QueryString.ToString()); - } - - // Only process requests that pass our sanitizing filter. - if (ImageHelpers.IsValidImageExtension(requestPath) && !string.IsNullOrWhiteSpace(queryString)) - { - // Replace any presets in the querystring with the actual value. - queryString = this.ReplacePresetsInQueryString(queryString); + else + { + requestPath = HostingEnvironment.MapPath(request.Path); + queryString = HttpUtility.UrlDecode(request.QueryString.ToString()); + } - string parts = !string.IsNullOrWhiteSpace(urlParameters) ? "?" + urlParameters : string.Empty; - string fullPath = string.Format("{0}{1}?{2}", requestPath, parts, queryString); + // Only process requests that pass our sanitizing filter. + if (!string.IsNullOrWhiteSpace(queryString)) + { + // Replace any presets in the querystring with the actual value. + queryString = this.ReplacePresetsInQueryString(queryString); - // Create a new cache to help process and cache the request. - DiskCache cache = new DiskCache(requestPath, fullPath); - string cachedPath = cache.CachedPath; + string parts = !string.IsNullOrWhiteSpace(urlParameters) ? "?" + urlParameters : string.Empty; + string fullPath = string.Format("{0}{1}?{2}", requestPath, parts, queryString); - // Since we are now rewriting the path we need to check again that the current user has access - // to the rewritten path. - // Get the user for the current request - // If the user is anonymous or authentication doesn't work for this suffix avoid a NullReferenceException - // in the UrlAuthorizationModule by creating a generic identity. - string virtualCachedPath = cache.VirtualCachedPath; + // Create a new cache to help process and cache the request. + DiskCache cache = new DiskCache(requestPath, fullPath); + string cachedPath = cache.CachedPath; - IPrincipal user = context.User ?? new GenericPrincipal(new GenericIdentity(string.Empty, string.Empty), new string[0]); + // Since we are now rewriting the path we need to check again that the current user has access + // to the rewritten path. + // Get the user for the current request + // If the user is anonymous or authentication doesn't work for this suffix avoid a NullReferenceException + // in the UrlAuthorizationModule by creating a generic identity. + string virtualCachedPath = cache.VirtualCachedPath; - // Do we have permission to call UrlAuthorizationModule.CheckUrlAccessForPrincipal? - PermissionSet permission = new PermissionSet(PermissionState.None); - permission.AddPermission(new AspNetHostingPermission(AspNetHostingPermissionLevel.Unrestricted)); - bool hasPermission = permission.IsSubsetOf(AppDomain.CurrentDomain.PermissionSet); + IPrincipal user = context.User ?? new GenericPrincipal(new GenericIdentity(string.Empty, string.Empty), new string[0]); - bool isAllowed = true; + // Do we have permission to call UrlAuthorizationModule.CheckUrlAccessForPrincipal? + PermissionSet permission = new PermissionSet(PermissionState.None); + permission.AddPermission(new AspNetHostingPermission(AspNetHostingPermissionLevel.Unrestricted)); + bool hasPermission = permission.IsSubsetOf(AppDomain.CurrentDomain.PermissionSet); - // Run the rewritten path past the authorization system again. - // We can then use the result as the default "AllowAccess" value - if (hasPermission && !context.SkipAuthorization) - { - isAllowed = UrlAuthorizationModule.CheckUrlAccessForPrincipal(virtualCachedPath, user, "GET"); - } + bool isAllowed = true; - if (isAllowed) - { - // Is the file new or updated? - bool isNewOrUpdated = cache.IsNewOrUpdatedFile(cachedPath); + // Run the rewritten path past the authorization system again. + // We can then use the result as the default "AllowAccess" value + if (hasPermission && !context.SkipAuthorization) + { + isAllowed = UrlAuthorizationModule.CheckUrlAccessForPrincipal(virtualCachedPath, user, "GET"); + } - // Only process if the file has been updated. - if (isNewOrUpdated) + if (isAllowed) { - // Process the image. - using (ImageFactory imageFactory = new ImageFactory(preserveExifMetaData != null && preserveExifMetaData.Value)) + // Is the file new or updated? + bool isNewOrUpdated = cache.IsNewOrUpdatedFile(cachedPath); + + // Only process if the file has been updated. + if (isNewOrUpdated) { - using (await Locker.LockAsync(cachedPath)) + // Process the image. + using ( + ImageFactory imageFactory = + new ImageFactory(preserveExifMetaData != null && preserveExifMetaData.Value)) { - byte[] imageBuffer; - - if (!isFileLocal) - { - Uri uri = new Uri(requestPath + "?" + urlParameters); - imageBuffer = await currentService.GetImage(uri); - } - else + using (await Locker.LockAsync(cachedPath)) { - imageBuffer = await currentService.GetImage(requestPath); + byte[] imageBuffer; + + if (!isFileLocal) + { + Uri uri = new Uri(requestPath + "?" + urlParameters); + imageBuffer = await currentService.GetImage(uri); + } + else + { + imageBuffer = await currentService.GetImage(requestPath); + } + + using (MemoryStream memoryStream = new MemoryStream(imageBuffer)) + { + // Reset the position of the stream to ensure we're reading the correct part. + memoryStream.Position = 0; + + // Process the Image + imageFactory.Load(memoryStream).AutoProcess(queryString).Save(cachedPath); + + // Add to the cache. + cache.AddImageToCache(cachedPath); + + // Store the cached path, response type, and cache dependency in the context for later retrieval. + context.Items[CachedPathKey] = cachedPath; + context.Items[CachedResponseTypeKey] = imageFactory.CurrentImageFormat.MimeType; + context.Items[CachedResponseFileDependency] = new List { cachedPath }; + } } + } + } - using (MemoryStream memoryStream = new MemoryStream(imageBuffer)) - { - // Reset the position of the stream to ensure we're reading the correct part. - memoryStream.Position = 0; - - // Process the Image - imageFactory.Load(memoryStream) - .AutoProcess(queryString) - .Save(cachedPath); - - // Add to the cache. - cache.AddImageToCache(cachedPath); + // Image is from the cache so the mime-type will need to be set. + if (context.Items[CachedResponseTypeKey] == null) + { + string mimetype = ImageHelpers.GetMimeType(cachedPath); - // Store the cached path, response type, and cache dependency in the context for later retrieval. - context.Items[CachedPathKey] = cachedPath; - context.Items[CachedResponseTypeKey] = imageFactory.CurrentImageFormat.MimeType; - context.Items[CachedResponseFileDependency] = new List { cachedPath }; - } + if (!string.IsNullOrEmpty(mimetype)) + { + context.Items[CachedResponseTypeKey] = mimetype; } } - } - - // Image is from the cache so the mime-type will need to be set. - if (context.Items[CachedResponseTypeKey] == null) - { - string mimetype = ImageHelpers.GetMimeType(cachedPath); - if (!string.IsNullOrEmpty(mimetype)) + if (context.Items[CachedResponseFileDependency] == null) { - context.Items[CachedResponseTypeKey] = mimetype; + context.Items[CachedResponseFileDependency] = new List { cachedPath }; } - } - if (context.Items[CachedResponseFileDependency] == null) - { - context.Items[CachedResponseFileDependency] = new List { cachedPath }; - } + string incomingEtag = context.Request.Headers["If" + "-None-Match"]; - string incomingEtag = context.Request.Headers["If" + "-None-Match"]; + if (incomingEtag != null && !isNewOrUpdated) + { + // Set the Content-Length header so the client doesn't wait for + // content but keeps the connection open for other requests. + context.Response.AddHeader("Content-Length", "0"); + context.Response.StatusCode = (int)HttpStatusCode.NotModified; + context.Response.SuppressContent = true; - if (incomingEtag != null && !isNewOrUpdated) - { - // Set the Content-Length header so the client doesn't wait for - // content but keeps the connection open for other requests. - context.Response.AddHeader("Content-Length", "0"); - context.Response.StatusCode = (int)HttpStatusCode.NotModified; - context.Response.SuppressContent = true; + if (isFileLocal) + { + // Set the headers and quit. + this.SetHeaders(context, (string)context.Items[CachedResponseTypeKey], new List { requestPath, cachedPath }); + return; + } - if (!isFileLocal) - { - // Set the headers and quit. - this.SetHeaders(context, (string)context.Items[CachedResponseTypeKey], new List { requestPath, cachedPath }); - return; + this.SetHeaders(context, (string)context.Items[CachedResponseTypeKey], new List { cachedPath }); } - this.SetHeaders(context, (string)context.Items[CachedResponseTypeKey], new List { cachedPath }); + // The cached file is valid so just rewrite the path. + context.RewritePath(virtualCachedPath, false); + } + else + { + throw new HttpException(403, "Access denied"); } - - // The cached file is valid so just rewrite the path. - context.RewritePath(virtualCachedPath, false); } - else + else if (!isFileLocal) { - throw new HttpException(403, "Access denied"); + // Just re-point to the external url. + HttpContext.Current.Response.Redirect(requestPath); } } - else if (!isFileLocal) - { - // Just re-point to the external url. - HttpContext.Current.Response.Redirect(requestPath); - } } /// @@ -539,7 +537,18 @@ namespace ImageProcessor.Web.HttpModules } } - return imageService ?? services.FirstOrDefault(s => string.IsNullOrWhiteSpace(s.Key)); + if (imageService != null) + { + return imageService; + } + + // Return the file based service + if (ImageHelpers.IsValidImageExtension(path)) + { + return services.FirstOrDefault(s => string.IsNullOrWhiteSpace(s.Key)); + } + + return null; } #endregion } diff --git a/src/ImageProcessorConsole/Program.cs b/src/ImageProcessorConsole/Program.cs deleted file mode 100644 index e69de29bb..000000000 diff --git a/src/Images/Penguins-8.png.REMOVED.git-id b/src/Images/Penguins-8.png.REMOVED.git-id index 377403fa3..912fc9848 100644 --- a/src/Images/Penguins-8.png.REMOVED.git-id +++ b/src/Images/Penguins-8.png.REMOVED.git-id @@ -1 +1 @@ -c433c806c6aba9b23e24ce55b26382e117c7a5d9 \ No newline at end of file +1672192a14349a4c93fe9ae885d57a3f34e6cc7c \ No newline at end of file