From 8bb069e996a452b2dd4fa016bca32c3ec6d933cc Mon Sep 17 00:00:00 2001 From: Claus Date: Tue, 8 Nov 2016 09:55:24 +0100 Subject: [PATCH] U4-9134 XSS security issue in the grid exposing xss clean method on templateutilities. making the clean xss string extensions public instead of internal. ensuring the included grid renderers clean for xss. ensuring the included grid editors using html.raw with value directly, cleans for xss. --- src/Umbraco.Core/StringExtensions.cs | 2 +- src/Umbraco.Web.UI/Umbraco.Web.UI.csproj | 2 + .../Partials/Grid/Bootstrap2-Fluid.cshtml | 31 ++-- .../Views/Partials/Grid/Bootstrap2.cshtml | 31 ++-- .../Partials/Grid/Bootstrap3-Fluid.cshtml | 32 ++-- .../Views/Partials/Grid/Bootstrap3.cshtml | 31 ++-- .../Views/Partials/Grid/Editors/Base.cshtml | 1 - .../Views/Partials/Grid/Editors/Embed.cshtml | 1 - .../Views/Partials/Grid/Editors/Macro.cshtml | 2 - .../Views/Partials/Grid/Editors/Media.cshtml | 1 - .../Partials/Grid/Editors/TextString.cshtml | 5 +- .../Templates/TemplateUtilities.cs | 143 +++++++++--------- 12 files changed, 164 insertions(+), 118 deletions(-) diff --git a/src/Umbraco.Core/StringExtensions.cs b/src/Umbraco.Core/StringExtensions.cs index 036b5b979f..27de06c371 100644 --- a/src/Umbraco.Core/StringExtensions.cs +++ b/src/Umbraco.Core/StringExtensions.cs @@ -184,7 +184,7 @@ namespace Umbraco.Core /// /// /// - internal static string CleanForXss(this string input, params char[] ignoreFromClean) + public static string CleanForXss(this string input, params char[] ignoreFromClean) { //remove any html input = input.StripHtml(); diff --git a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj index 6f2728a241..512c8177a9 100644 --- a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj +++ b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj @@ -1973,6 +1973,8 @@ + + Web.Template.config diff --git a/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap2-Fluid.cshtml b/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap2-Fluid.cshtml index 446a82f510..65b9b8abc7 100644 --- a/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap2-Fluid.cshtml +++ b/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap2-Fluid.cshtml @@ -64,21 +64,32 @@ JObject cfg = contentItem.config; if(cfg != null) - foreach (JProperty property in cfg.Properties()) { - attrs.Add(property.Name + "='" + property.Value.ToString() + "'"); + foreach (JProperty property in cfg.Properties()) + { + var propertyValue = TemplateUtilities.CleanForXss(property.Value.ToString()); + if (string.IsNullOrWhiteSpace(propertyValue) == false) + { + attrs.Add(property.Name + "='" + propertyValue + "'"); + } } - + JObject style = contentItem.styles; - if (style != null) { - var cssVals = new List(); - foreach (JProperty property in style.Properties()) - cssVals.Add(property.Name + ":" + property.Value.ToString() + ";"); + if (style != null) { + var cssVals = new List(); + foreach (JProperty property in style.Properties()) + { + var propertyValue = TemplateUtilities.CleanForXss(property.Value.ToString()); + if (string.IsNullOrWhiteSpace(propertyValue) == false) + { + cssVals.Add(property.Name + ":" + propertyValue + ";"); + } + } - if (cssVals.Any()) - attrs.Add("style='" + string.Join(" ", cssVals) + "'"); + if (cssVals.Any()) + attrs.Add("style='" + string.Join(" ", cssVals) + "'"); } - + return new MvcHtmlString(string.Join(" ", attrs)); } } \ No newline at end of file diff --git a/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap2.cshtml b/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap2.cshtml index 6bc730e1f8..37e3c84dad 100644 --- a/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap2.cshtml +++ b/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap2.cshtml @@ -64,21 +64,32 @@ JObject cfg = contentItem.config; if(cfg != null) - foreach (JProperty property in cfg.Properties()) { - attrs.Add(property.Name + "=\"" + property.Value.ToString() + "\""); + foreach (JProperty property in cfg.Properties()) + { + var propertyValue = TemplateUtilities.CleanForXss(property.Value.ToString()); + if (string.IsNullOrWhiteSpace(propertyValue) == false) + { + attrs.Add(property.Name + "=\"" + propertyValue + "\""); + } } - + JObject style = contentItem.styles; - if (style != null) { - var cssVals = new List(); - foreach (JProperty property in style.Properties()) - cssVals.Add(property.Name + ":" + property.Value.ToString() + ";"); + if (style != null) { + var cssVals = new List(); + foreach (JProperty property in style.Properties()) + { + var propertyValue = TemplateUtilities.CleanForXss(property.Value.ToString()); + if (string.IsNullOrWhiteSpace(propertyValue) == false) + { + cssVals.Add(property.Name + ":" + propertyValue + ";"); + } + } - if (cssVals.Any()) - attrs.Add("style=\"" + string.Join(" ", cssVals) + "\""); + if (cssVals.Any()) + attrs.Add("style=\"" + string.Join(" ", cssVals) + "\""); } - + return new MvcHtmlString(string.Join(" ", attrs)); } } \ No newline at end of file diff --git a/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap3-Fluid.cshtml b/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap3-Fluid.cshtml index 1244821d7e..45be239245 100644 --- a/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap3-Fluid.cshtml +++ b/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap3-Fluid.cshtml @@ -5,6 +5,7 @@ @* Razor helpers located at the bottom of this file *@ + @if (Model != null && Model.sections != null) { var oneColumn = ((System.Collections.ICollection)Model.sections).Count == 1; @@ -59,21 +60,32 @@ JObject cfg = contentItem.config; if(cfg != null) - foreach (JProperty property in cfg.Properties()) { - attrs.Add(property.Name + "='" + property.Value.ToString() + "'"); + foreach (JProperty property in cfg.Properties()) + { + var propertyValue = TemplateUtilities.CleanForXss(property.Value.ToString()); + if (string.IsNullOrWhiteSpace(propertyValue) == false) + { + attrs.Add(property.Name + "='" + propertyValue + "'"); + } } - + JObject style = contentItem.styles; - if (style != null) { - var cssVals = new List(); - foreach (JProperty property in style.Properties()) - cssVals.Add(property.Name + ":" + property.Value.ToString() + ";"); + if (style != null) { + var cssVals = new List(); + foreach (JProperty property in style.Properties()) + { + var propertyValue = TemplateUtilities.CleanForXss(property.Value.ToString()); + if (string.IsNullOrWhiteSpace(propertyValue) == false) + { + cssVals.Add(property.Name + ":" + propertyValue + ";"); + } + } - if (cssVals.Any()) - attrs.Add("style='" + string.Join(" ", cssVals) + "'"); + if (cssVals.Any()) + attrs.Add("style='" + string.Join(" ", cssVals) + "'"); } - + return new MvcHtmlString(string.Join(" ", attrs)); } } \ No newline at end of file diff --git a/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap3.cshtml b/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap3.cshtml index f76028d296..afadd3d93e 100644 --- a/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap3.cshtml +++ b/src/Umbraco.Web.UI/Views/Partials/Grid/Bootstrap3.cshtml @@ -64,21 +64,32 @@ JObject cfg = contentItem.config; if(cfg != null) - foreach (JProperty property in cfg.Properties()) { - attrs.Add(property.Name + "=\"" + property.Value.ToString() + "\""); + foreach (JProperty property in cfg.Properties()) + { + var propertyValue = TemplateUtilities.CleanForXss(property.Value.ToString()); + if (string.IsNullOrWhiteSpace(propertyValue) == false) + { + attrs.Add(property.Name + "=\"" + propertyValue +"\""); + } } - + JObject style = contentItem.styles; - if (style != null) { - var cssVals = new List(); - foreach (JProperty property in style.Properties()) - cssVals.Add(property.Name + ":" + property.Value.ToString() + ";"); + if (style != null) { + var cssVals = new List(); + foreach (JProperty property in style.Properties()) + { + var propertyValue = TemplateUtilities.CleanForXss(property.Value.ToString()); + if (string.IsNullOrWhiteSpace(propertyValue) == false) + { + cssVals.Add(property.Name + ":" + propertyValue + ";"); + } + } - if (cssVals.Any()) - attrs.Add("style=\"" + string.Join(" ", cssVals) + "\""); + if (cssVals.Any()) + attrs.Add("style=\"" + string.Join(" ", cssVals) + "\""); } - + return new MvcHtmlString(string.Join(" ", attrs)); } } \ No newline at end of file diff --git a/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Base.cshtml b/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Base.cshtml index a86c04819a..ffb7603048 100644 --- a/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Base.cshtml +++ b/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Base.cshtml @@ -1,5 +1,4 @@ @model dynamic -@using Umbraco.Web.Templates @functions { public static string EditorView(dynamic contentItem) diff --git a/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Embed.cshtml b/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Embed.cshtml index 4fd66ddb90..c27be6bcdf 100644 --- a/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Embed.cshtml +++ b/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Embed.cshtml @@ -1,3 +1,2 @@ @model dynamic -@using Umbraco.Web.Templates @Html.Raw(Model.value) diff --git a/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Macro.cshtml b/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Macro.cshtml index e0822808d8..ed08bb2484 100644 --- a/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Macro.cshtml +++ b/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Macro.cshtml @@ -1,6 +1,4 @@ @inherits UmbracoViewPage -@using Umbraco.Web.Templates - @if (Model.value != null) { diff --git a/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Media.cshtml b/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Media.cshtml index f5dfc6459c..5b5adbdc7d 100644 --- a/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Media.cshtml +++ b/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/Media.cshtml @@ -1,5 +1,4 @@ @model dynamic -@using Umbraco.Web.Templates @if (Model.value != null) { diff --git a/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/TextString.cshtml b/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/TextString.cshtml index a031c658a9..4a15201997 100644 --- a/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/TextString.cshtml +++ b/src/Umbraco.Web.UI/Views/Partials/Grid/Editors/TextString.cshtml @@ -4,10 +4,9 @@ @if (Model.editor.config.markup != null) { string markup = Model.editor.config.markup.ToString(); - var UmbracoHelper = new UmbracoHelper(UmbracoContext.Current); - markup = markup.Replace("#value#", UmbracoHelper.ReplaceLineBreaksForHtml(Model.value.ToString())); + markup = markup.Replace("#value#", UmbracoHelper.ReplaceLineBreaksForHtml(TemplateUtilities.CleanForXss(Model.value.ToString()))); markup = markup.Replace("#style#", Model.editor.config.style.ToString()); @@ -17,6 +16,6 @@ else { -
@Model.value
+
@TemplateUtilities.CleanForXss(Model.value.ToString())
 } diff --git a/src/Umbraco.Web/Templates/TemplateUtilities.cs b/src/Umbraco.Web/Templates/TemplateUtilities.cs index 881cb563c5..c56d7b5b8a 100644 --- a/src/Umbraco.Web/Templates/TemplateUtilities.cs +++ b/src/Umbraco.Web/Templates/TemplateUtilities.cs @@ -7,17 +7,17 @@ using Umbraco.Core.Logging; namespace Umbraco.Web.Templates { - //NOTE: I realize there is only one class in this namespace but I'm pretty positive that there will be more classes in - //this namespace once we start migrating and cleaning up more code. + //NOTE: I realize there is only one class in this namespace but I'm pretty positive that there will be more classes in + //this namespace once we start migrating and cleaning up more code. - /// - /// Utility class used for templates - /// - public static class TemplateUtilities - { + /// + /// Utility class used for templates + /// + public static class TemplateUtilities + { //TODO: Pass in an Umbraco context!!!!!!!! Don't rely on the singleton so things are more testable internal static string ParseInternalLinks(string text, bool preview) - { + { // save and set for url provider var inPreviewMode = UmbracoContext.Current.InPreviewMode; UmbracoContext.Current.InPreviewMode = preview; @@ -33,79 +33,84 @@ namespace Umbraco.Web.Templates } return text; - } + } - /// - /// Parses the string looking for the {localLink} syntax and updates them to their correct links. - /// - /// - /// - public static string ParseInternalLinks(string text) - { + /// + /// Parses the string looking for the {localLink} syntax and updates them to their correct links. + /// + /// + /// + public static string ParseInternalLinks(string text) + { //TODO: Pass in an Umbraco context!!!!!!!! Don't rely on the singleton so things are more testable, better yet, pass in urlprovider, routing context, separately - //don't attempt to proceed without a context as we cannot lookup urls without one - if (UmbracoContext.Current == null || UmbracoContext.Current.RoutingContext == null) - { - return text; - } + //don't attempt to proceed without a context as we cannot lookup urls without one + if (UmbracoContext.Current == null || UmbracoContext.Current.RoutingContext == null) + { + return text; + } - var urlProvider = UmbracoContext.Current.UrlProvider; + var urlProvider = UmbracoContext.Current.UrlProvider; - // Parse internal links - var tags = Regex.Matches(text, @"href=""[/]?(?:\{|\%7B)localLink:([0-9]+)(?:\}|\%7D)", RegexOptions.IgnoreCase | RegexOptions.IgnorePatternWhitespace); - foreach (Match tag in tags) - if (tag.Groups.Count > 0) - { - var id = tag.Groups[1].Value; //.Remove(tag.Groups[1].Value.Length - 1, 1); - var newLink = urlProvider.GetUrl(int.Parse(id)); - text = text.Replace(tag.Value, "href=\"" + newLink); - } + // Parse internal links + var tags = Regex.Matches(text, @"href=""[/]?(?:\{|\%7B)localLink:([0-9]+)(?:\}|\%7D)", RegexOptions.IgnoreCase | RegexOptions.IgnorePatternWhitespace); + foreach (Match tag in tags) + if (tag.Groups.Count > 0) + { + var id = tag.Groups[1].Value; //.Remove(tag.Groups[1].Value.Length - 1, 1); + var newLink = urlProvider.GetUrl(int.Parse(id)); + text = text.Replace(tag.Value, "href=\"" + newLink); + } return text; - } + } - // static compiled regex for faster performance - private readonly static Regex ResolveUrlPattern = new Regex("(=[\"\']?)(\\W?\\~(?:.(?![\"\']?\\s+(?:\\S+)=|[>\"\']))+.)[\"\']?", RegexOptions.Compiled | RegexOptions.IgnoreCase | RegexOptions.IgnorePatternWhitespace); + // static compiled regex for faster performance + private readonly static Regex ResolveUrlPattern = new Regex("(=[\"\']?)(\\W?\\~(?:.(?![\"\']?\\s+(?:\\S+)=|[>\"\']))+.)[\"\']?", + RegexOptions.Compiled | RegexOptions.IgnoreCase | RegexOptions.IgnorePatternWhitespace); - /// - /// The RegEx matches any HTML attribute values that start with a tilde (~), those that match are passed to ResolveUrl to replace the tilde with the application path. - /// - /// - /// - /// - /// When used with a Virtual-Directory set-up, this would resolve all URLs correctly. - /// The recommendation is that the "ResolveUrlsFromTextString" option (in umbracoSettings.config) is set to false for non-Virtual-Directory installs. - /// - public static string ResolveUrlsFromTextString(string text) - { + /// + /// The RegEx matches any HTML attribute values that start with a tilde (~), those that match are passed to ResolveUrl to replace the tilde with the application path. + /// + /// + /// + /// + /// When used with a Virtual-Directory set-up, this would resolve all URLs correctly. + /// The recommendation is that the "ResolveUrlsFromTextString" option (in umbracoSettings.config) is set to false for non-Virtual-Directory installs. + /// + public static string ResolveUrlsFromTextString(string text) + { if (UmbracoConfig.For.UmbracoSettings().Content.ResolveUrlsFromTextString == false) return text; - using (var timer = DisposableTimer.DebugDuration(typeof(IOHelper), "ResolveUrlsFromTextString starting", "ResolveUrlsFromTextString complete")) - { - // find all relative urls (ie. urls that contain ~) - var tags = ResolveUrlPattern.Matches(text); - LogHelper.Debug(typeof(IOHelper), "After regex: " + timer.Stopwatch.ElapsedMilliseconds + " matched: " + tags.Count); - foreach (Match tag in tags) - { - var url = ""; - if (tag.Groups[1].Success) - url = tag.Groups[1].Value; + using (var timer = DisposableTimer.DebugDuration(typeof(IOHelper), "ResolveUrlsFromTextString starting", "ResolveUrlsFromTextString complete")) + { + // find all relative urls (ie. urls that contain ~) + var tags = ResolveUrlPattern.Matches(text); + LogHelper.Debug(typeof(IOHelper), "After regex: " + timer.Stopwatch.ElapsedMilliseconds + " matched: " + tags.Count); + foreach (Match tag in tags) + { + var url = ""; + if (tag.Groups[1].Success) + url = tag.Groups[1].Value; - // The richtext editor inserts a slash in front of the url. That's why we need this little fix - // if (url.StartsWith("/")) - // text = text.Replace(url, ResolveUrl(url.Substring(1))); - // else - if (String.IsNullOrEmpty(url) == false) - { - var resolvedUrl = (url.Substring(0, 1) == "/") ? IOHelper.ResolveUrl(url.Substring(1)) : IOHelper.ResolveUrl(url); - text = text.Replace(url, resolvedUrl); - } - } - } + // The richtext editor inserts a slash in front of the url. That's why we need this little fix + // if (url.StartsWith("/")) + // text = text.Replace(url, ResolveUrl(url.Substring(1))); + // else + if (String.IsNullOrEmpty(url) == false) + { + var resolvedUrl = (url.Substring(0, 1) == "/") ? IOHelper.ResolveUrl(url.Substring(1)) : IOHelper.ResolveUrl(url); + text = text.Replace(url, resolvedUrl); + } + } + } - return text; - } + return text; + } - } + public static string CleanForXss(string text, params char[] ignoreFromClean) + { + return text.CleanForXss(ignoreFromClean); + } + } }