Evaluate/Refactor my ASP.NET XSS Security Helper Class

Question

I am maintaining a site with significant security concerns and I wrote a helper class for validating potential XSS attacks. The ValidateRequest method is meant to evaluate the HttpRequest for any potential issues, and if issues are found, redirect the user to the same page, but in a away that is not threatening to the application.

The application is being evaluated by HP WebInspect. Their initial review included two regular expressions:

1. The "Regex for a simple XSS attack"

   /((\%3C) <)((\%2F) \/)*[a-z0-9\%]+((\%3E) >)/ix

2. The "Paranoid regex for XSS attacks"

   /((\%3C) <)[^\n]+((\%3E) >)/I

The regular expression I am using is a more stringent C# adaptation of the paranoid regex. I prefer to err on the side of caution, so I would rather risk denying valid requests than allowing an invalid request to be processed.

Thanks in advance for the help.

using System;
using System.Text.RegularExpressions;
using System.Web;
public static class XssSecurity
{
public const string PotentialXssAttackExpression = "(http(s)*(%3a|:))|(ftp(s)*(%3a|:))|(javascript)|(alert)|(((\\%3C) <)[^\n]+((\\%3E) >))";
private static readonly Regex PotentialXssAttackRegex = new Regex(PotentialXssAttackExpression, RegexOptions.IgnoreCase);
public static bool IsPotentialXssAttack(this HttpRequest request)
{
 if(request != null)
 {
 string query = request.QueryString.ToString();
 if(!string.IsNullOrEmpty(query) && PotentialXssAttackRegex.IsMatch(query))
 return true;
 if(request.HttpMethod.Equals("post", StringComparison.InvariantCultureIgnoreCase))
 {
 string form = request.Form.ToString();
 if (!string.IsNullOrEmpty(form) && PotentialXssAttackRegex.IsMatch(form))
 return true;
 }
 if(request.Cookies.Count > 0)
 {
 foreach(HttpCookie cookie in request.Cookies)
 {
 if(PotentialXssAttackRegex.IsMatch(cookie.Value))
 {
 return true;
 }
 }
 }
 } 
 return false;
}
 public static void ValidateRequest(this HttpContext context, string redirectToPath = null)
 {
 if(context == null || !context.Request.IsPotentialXssAttack()) return;
 // expire all cookies
 foreach(HttpCookie cookie in context.Request.Cookies)
 {
 cookie.Expires = DateTime.Now.Subtract(TimeSpan.FromDays(1));
 context.Response.Cookies.Set(cookie);
 }
 // redirect to safe path
 bool redirected = false;
 if(redirectToPath != null)
 {
 try
 {
 context.Response.Redirect(redirectToPath,true);
 redirected = true;
 }
 catch
 {
 redirected = false;
 }
 }
 if (redirected) 
 return;
 string safeUrl = context.Request.Url.AbsolutePath.Replace(context.Request.Url.Query, string.Empty);
 context.Response.Redirect(safeUrl,true);
 }
}

Answer 1

Some general idea:

1. Use guard statements instead of big if blocks.
2. Extract the !string.IsNullOrEmpty(query) && PotentialXssAttackRegex.IsMatch(query) condition to a method.
3. Extract individual checks to distinct methods. The IsPotentialXssAttack will be quite easy to read and the code of different validations will not be mixed in one big method.
4. I'd create an expireAllCookies helper method too.

The final code of the first method would look like this (I haven't tested nor compiled):

public static bool IsPotentialXssAttack(this HttpRequest request) {
 if (request == null) {
 return false;
 }
 if (isPotentialXssAttackQuery(request)) {
 return true;
 }
 if (isPotentialXssAttackFormOrAnything(request)) {
 return true;
 }
 if (isPotentialXssAttackCookie(request)) {
 return true;
 }
 return false;
}
public static bool isPotentialXssAttackQuery(this HttpRequest request) {
 string query = request.QueryString.ToString();
 if (isMatch(query)) {
 return true;
 }
 return false;
}
public static bool isMatch(string input) {
 if (string.IsNullOrEmpty(query)) {
 return false;
 }
 return PotentialXssAttackRegex.IsMatch(query);
}
// TODO: rename it
public static bool isPotentialXssAttackFormOrAnything(this HttpRequest request) {
 if (!request.HttpMethod.Equals("post", 
 StringComparison.InvariantCultureIgnoreCase)) {
 return false;
 }
 string form = request.Form.ToString();
 if (isMatch(form)) {
 return true;
 }
 return false;
}
public static bool isPotentialXssAttackCookie(this HttpRequest request) {
 if (request.Cookies.Count <= 0) {
 return false;
 }
 foreach (HttpCookie cookie in request.Cookies){
 if (isMatch(cookie.Value)) {
 return true;
 }
 }
 return false;
}

• smartcaveman: +1, @palacsint, clearly all good suggestions. I like that when you apply this kind of refactoring the things being tested become much more explicit to other developers. I noticed we can even go further and write, public static bool IsPotentialXssAttack(this HttpRequest request) { return request != null && (isPotentialXssAttackQuery(request) || isPotentialXssAttackFormOrAnything(request) || isPotentialXssAttackCookie(request)); }. However, did you notice any potential flaws in the actual functionality for mitigating XSS attacks?
• palacsint: I don't prefer long conditions with lots of ||s. I think it's harder to read than single ifs. I didn't notice any flaws in the XSS logic, unfortunately I'm not too familiar with this field.
• smartcaveman: I guess that's personal preference. To me it's easier to read, b/c my brain automatically translates || to " or " and && to " and ", so it sounds like plain english
• smartcaveman: e.g. imagine: public static class Syntax { public static bool Or<T>(this bool value, T candidate, Func<T, bool> predicate) { return value || predicate(candidate); } public static bool And<T>(this bool value, T candidate, Func<T, bool> predicate) { return value && predicate(candidate); } } . Then you can do return (request != null).And(isPotentialXssAttackQuery(request).Or(request,isPotentialXssAttackFormOrAnything).Or(request,isPotentialXssAttackCookie); ... I'd say that's a pretty readable single line