From 1efa75c891c16b235492ba0bc5b01ad44b840931 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 4 Dec 2013 12:42:40 +1100 Subject: [PATCH 1/2] Fixes potential xss --- src/Umbraco.Core/StringExtensions.cs | 13 +++++++++++ .../config/umbracoSettings.config | 6 ++--- src/Umbraco.Web/HttpRequestExtensions.cs | 5 +---- .../Modules/SkinModule/ImageUploader.aspx.cs | 22 ++++++++++++------- 4 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/Umbraco.Core/StringExtensions.cs b/src/Umbraco.Core/StringExtensions.cs index abdae894c4..4c3cfbeba5 100644 --- a/src/Umbraco.Core/StringExtensions.cs +++ b/src/Umbraco.Core/StringExtensions.cs @@ -50,6 +50,19 @@ namespace Umbraco.Core return mName; } + /// + /// Cleans string to aid in preventing xss attacks. + /// + /// + /// + internal static string CleanForXss(this string input) + { + //remove any html + input = input.StripHtml(); + //strip out any potential chars involved with XSS + return input.ExceptChars(new HashSet("*?(){}[];:%<>/\\|&'\"".ToCharArray())); + } + public static string ExceptChars(this string str, HashSet toExclude) { var sb = new StringBuilder(str.Length); diff --git a/src/Umbraco.Web.UI/config/umbracoSettings.config b/src/Umbraco.Web.UI/config/umbracoSettings.config index 4510e6ae2f..ff21ef8249 100644 --- a/src/Umbraco.Web.UI/config/umbracoSettings.config +++ b/src/Umbraco.Web.UI/config/umbracoSettings.config @@ -177,15 +177,15 @@ - + 0 - umblb1.dev - localhost + umb1.dev + localhost diff --git a/src/Umbraco.Web/HttpRequestExtensions.cs b/src/Umbraco.Web/HttpRequestExtensions.cs index b9ab1c6993..4071cf4d72 100644 --- a/src/Umbraco.Web/HttpRequestExtensions.cs +++ b/src/Umbraco.Web/HttpRequestExtensions.cs @@ -20,10 +20,7 @@ namespace Umbraco.Web public static string GetCleanedItem(this HttpRequest request, string key) { var item = request.GetItemAsString(key); - //remove any html - item = item.StripHtml(); - //strip out any potential chars involved with XSS - return item.ExceptChars(new HashSet("(){}[];:%<>/\\|&'\"".ToCharArray())); + return item.CleanForXss(); } /// diff --git a/src/Umbraco.Web/umbraco.presentation/umbraco/LiveEditing/Modules/SkinModule/ImageUploader.aspx.cs b/src/Umbraco.Web/umbraco.presentation/umbraco/LiveEditing/Modules/SkinModule/ImageUploader.aspx.cs index 245aabbd7c..4de9be335d 100644 --- a/src/Umbraco.Web/umbraco.presentation/umbraco/LiveEditing/Modules/SkinModule/ImageUploader.aspx.cs +++ b/src/Umbraco.Web/umbraco.presentation/umbraco/LiveEditing/Modules/SkinModule/ImageUploader.aspx.cs @@ -10,9 +10,13 @@ using System.Drawing.Drawing2D; using System.Drawing.Imaging; using Umbraco.Core.IO; using umbraco.BusinessLogic; +using Umbraco.Core; namespace umbraco.presentation.umbraco.LiveEditing.Modules.SkinModule { + + //TODO: This doesn't really handle image uploading appropriately (sizing, naming, etc... )- yet another reason live editing needs to be removed. + public partial class ImageUploader : BasePages.UmbracoEnsuredPage { //max width and height is used to make sure the cropper doesn't grow bigger then the modal window @@ -35,23 +39,25 @@ namespace umbraco.presentation.umbraco.LiveEditing.Modules.SkinModule { if (FileUpload1.HasFile) { + //clean the name from XSS + var uploadedFileName = FileUpload1.FileName.CleanForXss(); - Guid g = Guid.NewGuid(); - DirectoryInfo updir = new DirectoryInfo(IOHelper.MapPath("~/media/upload/" + g)); + var g = Guid.NewGuid(); + var updir = new DirectoryInfo(IOHelper.MapPath("~/media/upload/" + g)); - if (!updir.Exists) + if (updir.Exists == false) updir.Create(); - FileName.Value = FileUpload1.FileName; + FileName.Value = uploadedFileName; - FileUpload1.SaveAs(updir.FullName + "/" + FileUpload1.FileName); + FileUpload1.SaveAs(updir.FullName + "/" + uploadedFileName); - if (IsValidImage(updir.FullName + "/" + FileUpload1.FileName)) + if (IsValidImage(updir.FullName + "/" + uploadedFileName)) { - Image1.ImageUrl = this.ResolveUrl("~/media/upload/" + g) + "/" + FileUpload1.FileName; + Image1.ImageUrl = this.ResolveUrl("~/media/upload/" + g) + "/" + uploadedFileName; Image.Value = Image1.ImageUrl; - if ((!string.IsNullOrEmpty(Request["w"]) && Request["w"].ToString() != "0") && (!string.IsNullOrEmpty(Request["h"]) && Request["h"].ToString() != "0")) + if ((string.IsNullOrEmpty(Request["w"]) == false && Request["w"] != "0") && (string.IsNullOrEmpty(Request["h"]) == false && Request["h"] != "0")) { if (Convert.ToInt32(Request["w"]) > MaxWidth || Convert.ToInt32(Request["h"]) > MaxHeight) From 33aa4e20621c039eb3df1e43f8a4d8bf8e11f2a2 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 4 Dec 2013 13:33:24 +1100 Subject: [PATCH 2/2] Fixes potential xss --- .../LiveEditing/Modules/SkinModule/ModuleInjector.aspx | 2 +- src/Umbraco.Web.UI/umbraco/Umbraco.aspx.cs | 4 ++-- .../umbraco/developer/Xslt/xsltChooseExtension.aspx | 2 +- .../umbraco/developer/Xslt/xsltInsertValueOf.aspx | 4 ++-- src/Umbraco.Web.UI/umbraco/dialogs/create.aspx | 4 ++-- src/Umbraco.Web.UI/umbraco/dialogs/insertMacro.aspx | 4 ++-- src/Umbraco.Web.UI/umbraco/dialogs/moveOrCopy.aspx | 6 +++--- src/Umbraco.Web.UI/umbraco/dialogs/sort.aspx | 4 ++-- src/Umbraco.Web.UI/umbraco/dialogs/umbracoField.aspx | 2 +- .../umbraco/plugins/tinymce3/insertMacro.aspx | 4 ++-- src/Umbraco.Web/HttpRequestExtensions.cs | 2 +- .../umbraco.presentation/umbraco/dashboard.aspx.cs | 2 +- .../umbraco/dashboard/FeedProxy.aspx.cs | 5 +++-- .../umbraco/developer/Cache/viewCacheItem.aspx.cs | 3 ++- .../umbraco/developer/Macros/assemblyBrowser.aspx.cs | 3 ++- .../umbraco.presentation/umbraco/dialogs/create.aspx.cs | 2 +- 16 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/Umbraco.Web.UI/umbraco/LiveEditing/Modules/SkinModule/ModuleInjector.aspx b/src/Umbraco.Web.UI/umbraco/LiveEditing/Modules/SkinModule/ModuleInjector.aspx index 9c9e7cc631..54f71d7368 100644 --- a/src/Umbraco.Web.UI/umbraco/LiveEditing/Modules/SkinModule/ModuleInjector.aspx +++ b/src/Umbraco.Web.UI/umbraco/LiveEditing/Modules/SkinModule/ModuleInjector.aspx @@ -109,7 +109,7 @@ top.jQuery('.umbModalBoxIframe').closest(".umbModalBox").ModalWindowAPI().close(); - top.umbInsertModule('<%=Request.GetCleanedItem("target")%>',macroString,'<%=Request.GetCleanedItem("type")%>'); + top.umbInsertModule('<%=Request.CleanForXss("target")%>',macroString,'<%=Request.CleanForXss("type")%>'); } function pseudoHtmlEncode(text) { diff --git a/src/Umbraco.Web.UI/umbraco/Umbraco.aspx.cs b/src/Umbraco.Web.UI/umbraco/Umbraco.aspx.cs index 1d41199625..1e72b6f347 100644 --- a/src/Umbraco.Web.UI/umbraco/Umbraco.aspx.cs +++ b/src/Umbraco.Web.UI/umbraco/Umbraco.aspx.cs @@ -23,7 +23,7 @@ namespace Umbraco.Web.UI.Umbraco { get { - var app = Request.GetCleanedItem("app"); + var app = Request.CleanForXss("app"); //validate the app if (global::umbraco.BusinessLogic.Application.getAll().Any(x => x.alias.InvariantEquals(app)) == false) { @@ -45,7 +45,7 @@ namespace Umbraco.Web.UI.Umbraco protected string RightActionId { - get { return Request.GetCleanedItem("id").ReplaceNonAlphanumericChars('-'); } + get { return Request.CleanForXss("id").ReplaceNonAlphanumericChars('-'); } } protected void Page_Load(object sender, EventArgs e) diff --git a/src/Umbraco.Web.UI/umbraco/developer/Xslt/xsltChooseExtension.aspx b/src/Umbraco.Web.UI/umbraco/developer/Xslt/xsltChooseExtension.aspx index 2540c5290a..0700ee1c91 100644 --- a/src/Umbraco.Web.UI/umbraco/developer/Xslt/xsltChooseExtension.aspx +++ b/src/Umbraco.Web.UI/umbraco/developer/Xslt/xsltChooseExtension.aspx @@ -14,7 +14,7 @@ result = result.substring(0, result.length - 2); result = result + ")"; - document.location = 'xsltInsertValueOf.aspx?objectId=<%=Request.GetCleanedItem("objectId")%>&value=' + result; + document.location = 'xsltInsertValueOf.aspx?objectId=<%=Request.CleanForXss("objectId")%>&value=' + result; } diff --git a/src/Umbraco.Web.UI/umbraco/developer/Xslt/xsltInsertValueOf.aspx b/src/Umbraco.Web.UI/umbraco/developer/Xslt/xsltInsertValueOf.aspx index 4a206a04b2..576cd6dff2 100644 --- a/src/Umbraco.Web.UI/umbraco/developer/Xslt/xsltInsertValueOf.aspx +++ b/src/Umbraco.Web.UI/umbraco/developer/Xslt/xsltInsertValueOf.aspx @@ -14,13 +14,13 @@ result = ''; UmbClientMgr.contentFrame().focus(); - UmbClientMgr.contentFrame().UmbEditor.Insert(result, '', '<%=Request.GetCleanedItem("objectId")%>'); + UmbClientMgr.contentFrame().UmbEditor.Insert(result, '', '<%=Request.CleanForXss("objectId")%>'); UmbClientMgr.closeModalWindow(); } function getExtensionMethod() { - document.location = 'xsltChooseExtension.aspx?objectId=<%=Request.GetCleanedItem("objectId")%>'; + document.location = 'xsltChooseExtension.aspx?objectId=<%=Request.CleanForXss("objectId")%>'; } function recieveExtensionMethod(theValue) { diff --git a/src/Umbraco.Web.UI/umbraco/dialogs/create.aspx b/src/Umbraco.Web.UI/umbraco/dialogs/create.aspx index 17b231f923..686c781386 100644 --- a/src/Umbraco.Web.UI/umbraco/dialogs/create.aspx +++ b/src/Umbraco.Web.UI/umbraco/dialogs/create.aspx @@ -34,14 +34,14 @@ } function onNodeSelectionConfirmed() { - document.location.href = 'create.aspx?nodeType=<%=Request.GetCleanedItem("nodeType")%>&app=<%=App%>&nodeId=' + document.getElementById('nodeId').value + document.location.href = 'create.aspx?nodeType=<%=Request.CleanForXss("nodeType")%>&app=<%=App%>&nodeId=' + document.getElementById('nodeId').value } - " /> + " /> - " /> - " /> + " /> + " />
diff --git a/src/Umbraco.Web.UI/umbraco/dialogs/moveOrCopy.aspx b/src/Umbraco.Web.UI/umbraco/dialogs/moveOrCopy.aspx index 683b940f99..43f17f096b 100644 --- a/src/Umbraco.Web.UI/umbraco/dialogs/moveOrCopy.aspx +++ b/src/Umbraco.Web.UI/umbraco/dialogs/moveOrCopy.aspx @@ -16,9 +16,9 @@ if (id > 0) umbraco.presentation.webservices.CMSNode.GetNodeName('<%=umbracoUserContextID%>', id, updateName); else{ - //document.getElementById("pageNameContent").innerHTML = "'<%=umbraco.ui.Text(Request.GetCleanedItem("app"))%>' <%= umbraco.ui.Text("moveOrCopy","nodeSelected") %>"; + //document.getElementById("pageNameContent").innerHTML = "'<%=umbraco.ui.Text(Request.CleanForXss("app"))%>' <%= umbraco.ui.Text("moveOrCopy","nodeSelected") %>"; - jQuery("#pageNameContent").html("<%=umbraco.ui.Text(Request.GetCleanedItem("app"))%> <%= umbraco.ui.Text("moveOrCopy","nodeSelected") %>"); + jQuery("#pageNameContent").html("<%=umbraco.ui.Text(Request.CleanForXss("app"))%> <%= umbraco.ui.Text("moveOrCopy","nodeSelected") %>"); jQuery("#pageNameHolder").attr("class","success"); } } @@ -59,7 +59,7 @@ - diff --git a/src/Umbraco.Web.UI/umbraco/dialogs/sort.aspx b/src/Umbraco.Web.UI/umbraco/dialogs/sort.aspx index 76db9b819f..ca2a0d5f49 100644 --- a/src/Umbraco.Web.UI/umbraco/dialogs/sort.aspx +++ b/src/Umbraco.Web.UI/umbraco/dialogs/sort.aspx @@ -70,8 +70,8 @@ submitButton: jQuery("#submitButton"), closeWindowButton : jQuery("#closeWindowButton"), dateTimeFormat: "<%=CultureInfo.CurrentCulture.DateTimeFormat.ShortDatePattern%> <%=CultureInfo.CurrentCulture.DateTimeFormat.ShortTimePattern%>", - currentId: "<%=Request.GetCleanedItem("ID")%>", - serviceUrl: "<%= IOHelper.ResolveUrl(SystemDirectories.Umbraco)%>/WebServices/NodeSorter.asmx/UpdateSortOrder?app=<%=Request.GetCleanedItem("app")%>" + currentId: "<%=Request.CleanForXss("ID")%>", + serviceUrl: "<%= IOHelper.ResolveUrl(SystemDirectories.Umbraco)%>/WebServices/NodeSorter.asmx/UpdateSortOrder?app=<%=Request.CleanForXss("app")%>" }); sortDialog.init(); diff --git a/src/Umbraco.Web.UI/umbraco/dialogs/umbracoField.aspx b/src/Umbraco.Web.UI/umbraco/dialogs/umbracoField.aspx index 9df80dd370..45b01fc54a 100644 --- a/src/Umbraco.Web.UI/umbraco/dialogs/umbracoField.aspx +++ b/src/Umbraco.Web.UI/umbraco/dialogs/umbracoField.aspx @@ -25,7 +25,7 @@ submitButton: $("#submitButton"), form: document.forms[0], tagName: document.forms[0].<%= tagName.ClientID %>.value, - objectId: '<%=Request.GetCleanedItem("objectId")%>' + objectId: '<%=Request.CleanForXss("objectId")%>' }); umbracoField.init(); }); diff --git a/src/Umbraco.Web.UI/umbraco/plugins/tinymce3/insertMacro.aspx b/src/Umbraco.Web.UI/umbraco/plugins/tinymce3/insertMacro.aspx index 0e935b4ed5..b6017824f4 100644 --- a/src/Umbraco.Web.UI/umbraco/plugins/tinymce3/insertMacro.aspx +++ b/src/Umbraco.Web.UI/umbraco/plugins/tinymce3/insertMacro.aspx @@ -106,8 +106,8 @@ " /> <%if (Request["umb_macroID"] != null || Request["umb_macroAlias"] != null) {%> - " /> - " /> + " /> + " /> <% }%>
diff --git a/src/Umbraco.Web/HttpRequestExtensions.cs b/src/Umbraco.Web/HttpRequestExtensions.cs index 4071cf4d72..7e12a55ae9 100644 --- a/src/Umbraco.Web/HttpRequestExtensions.cs +++ b/src/Umbraco.Web/HttpRequestExtensions.cs @@ -17,7 +17,7 @@ namespace Umbraco.Web /// /// /// - public static string GetCleanedItem(this HttpRequest request, string key) + public static string CleanForXss(this HttpRequest request, string key) { var item = request.GetItemAsString(key); return item.CleanForXss(); diff --git a/src/Umbraco.Web/umbraco.presentation/umbraco/dashboard.aspx.cs b/src/Umbraco.Web/umbraco.presentation/umbraco/dashboard.aspx.cs index f5784c16f1..09e3b1ee88 100644 --- a/src/Umbraco.Web/umbraco.presentation/umbraco/dashboard.aspx.cs +++ b/src/Umbraco.Web/umbraco.presentation/umbraco/dashboard.aspx.cs @@ -36,7 +36,7 @@ namespace umbraco.cms.presentation { if (_section == null) { - var qry = Request.GetCleanedItem("app"); + var qry = Request.CleanForXss("app"); // Load dashboard content if (qry.IsNullOrWhiteSpace() == false) { diff --git a/src/Umbraco.Web/umbraco.presentation/umbraco/dashboard/FeedProxy.aspx.cs b/src/Umbraco.Web/umbraco.presentation/umbraco/dashboard/FeedProxy.aspx.cs index 8317f8ab5d..a938935d2b 100644 --- a/src/Umbraco.Web/umbraco.presentation/umbraco/dashboard/FeedProxy.aspx.cs +++ b/src/Umbraco.Web/umbraco.presentation/umbraco/dashboard/FeedProxy.aspx.cs @@ -1,4 +1,5 @@ using Umbraco.Core.Logging; +using Umbraco.Web; namespace dashboardUtilities { @@ -32,10 +33,10 @@ namespace dashboardUtilities { var response = client.DownloadString(requestUri); - if (!string.IsNullOrEmpty(response)) + if (string.IsNullOrEmpty(response) == false) { Response.Clear(); - Response.ContentType = Request.QueryString["type"] ?? MediaTypeNames.Text.Xml; + Response.ContentType = Request.CleanForXss("type") ?? MediaTypeNames.Text.Xml; Response.Write(response); } } diff --git a/src/Umbraco.Web/umbraco.presentation/umbraco/developer/Cache/viewCacheItem.aspx.cs b/src/Umbraco.Web/umbraco.presentation/umbraco/developer/Cache/viewCacheItem.aspx.cs index a78a8d24da..aafc9000b1 100644 --- a/src/Umbraco.Web/umbraco.presentation/umbraco/developer/Cache/viewCacheItem.aspx.cs +++ b/src/Umbraco.Web/umbraco.presentation/umbraco/developer/Cache/viewCacheItem.aspx.cs @@ -1,5 +1,6 @@ using System; using System.Web; +using Umbraco.Web; using umbraco.BasePages; namespace umbraco.cms.presentation.developer @@ -17,7 +18,7 @@ namespace umbraco.cms.presentation.developer protected void Page_Load(object sender, EventArgs e) { Panel1.Text = ui.Text("viewCacheItem"); - var cacheKey = Request.QueryString["key"]; + var cacheKey = Request.CleanForXss("key"); LabelCacheAlias.Text = cacheKey; var cacheItem = ApplicationContext.ApplicationCache.GetCacheItem(cacheKey); LabelCacheValue.Text = cacheItem != null ? cacheItem.ToString() : "Cache item isn't in cache anymore!"; diff --git a/src/Umbraco.Web/umbraco.presentation/umbraco/developer/Macros/assemblyBrowser.aspx.cs b/src/Umbraco.Web/umbraco.presentation/umbraco/developer/Macros/assemblyBrowser.aspx.cs index a124a97fa4..c81e1cf8ad 100644 --- a/src/Umbraco.Web/umbraco.presentation/umbraco/developer/Macros/assemblyBrowser.aspx.cs +++ b/src/Umbraco.Web/umbraco.presentation/umbraco/developer/Macros/assemblyBrowser.aspx.cs @@ -12,6 +12,7 @@ using System.Web.UI.HtmlControls; using System.Reflection; using System.Collections.Specialized; using Umbraco.Core.IO; +using Umbraco.Web; using umbraco.BusinessLogic; using umbraco.cms.businesslogic.macro; using System.Collections.Generic; @@ -40,7 +41,7 @@ namespace umbraco.developer if (Request.QueryString["type"] == null) { isUserControl = true; - var fileName = Request.QueryString["fileName"]; + var fileName = Request.CleanForXss("fileName"); if (!fileName.StartsWith("~")) { if (fileName.StartsWith("/")) diff --git a/src/Umbraco.Web/umbraco.presentation/umbraco/dialogs/create.aspx.cs b/src/Umbraco.Web/umbraco.presentation/umbraco/dialogs/create.aspx.cs index 761c9cbe2c..a3da4846b3 100644 --- a/src/Umbraco.Web/umbraco.presentation/umbraco/dialogs/create.aspx.cs +++ b/src/Umbraco.Web/umbraco.presentation/umbraco/dialogs/create.aspx.cs @@ -38,7 +38,7 @@ namespace umbraco.dialogs { if (_app == null) { - _app = Request.GetCleanedItem("app"); + _app = Request.CleanForXss("app"); //validate the app if (BusinessLogic.Application.getAll().Any(x => x.alias.InvariantEquals(_app)) == false) {