2/21/2012 ASP.Net MVC Unobtrusive Validation Bug

If you use the ASP.Net MVC 3 [Compare] validation attribute on a model property, then include that model as a property in a parent model (so that the field name becomes Parent.ChildProperty), the built-in unobtrusive client validation will choke, and will always report the field as having an error.

This is due to a bug on line 288 of jquery.validate.unobtrusive.js:

adapters.add("equalto", ["other"], function (options) {
    var prefix = getModelPrefix(options.element.name),
        other = options.params.other,
        fullOtherName = appendModelPrefix(other, prefix),
        element = $(options.form).find(":input[name=" + fullOtherName + "]")[0];

    setValidationValues(options, "equalTo", element);
});

Because the value of the name attribute selector is not quoted, this fails if the name contains a ..

The simplest fix is to add quotes around the concatenated value.  However, the jQuery selector there is overkill.  HTML form elements have properties for each named input, so you can do this instead:

adapters.add("equalto", ["other"], function (options) {
    var prefix = getModelPrefix(options.element.name),
        other = options.params.other,
        fullOtherName = appendModelPrefix(other, prefix),
        element = options.form[fullOtherName];
    if (!element)
        throw new Error(fullOtherName + " not found");
    //If there are multiple inputs with that name, get the first one
    if (element.length && element[0])        
        element = element[0];
    setValidationValues(options, "equalTo", element);
});

1/25/2012 Protecting against CSRF attacks in ASP.Net MVC

CSRF attacks are one of the many security issues that web developers must defend against.  Fortunately, ASP.Net MVC makes it easy to defend against CSRF attacks.  Simply slap on [ValidateAntiForgeryToken] to every POST action and include @Html.AntiForgeryToken() in every form, and your forms will be secure against CSRF.

However, it is easy to forget to apply [ValidateAntiForgeryToken] to every action.  To prevent such mistakes, you can create a unit test that loops through all of your controller actions and makes sure that every [HttpPost] action also has [ValidateAntiForgeryToken]. 

Since there may be some POST actions that should not be protected against CSRF, you’ll probably also want a marker attribute to tell the test to ignore some actions.

This can be implemented like this:

First, define the marker attribute in the MVC web project.  This attribute can be applied to a single action, or to a controller to allow every action in the controller.

///<summary>Indicates that an action or controller deliberately 
/// allows CSRF attacks.</summary>
///<remarks>All [HttpPost] actions must have 
/// [ValidateAntiForgeryToken]; any deliberately unprotected 
/// actions must be marked with this attribute.
/// This rule is enforced by a unit test.</remarks>
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)]
public sealed class AllowCsrfAttacksAttribute : Attribute { }

Then, add the following unit test:

[TestMethod]
public void CheckForCsrfProtection() {
    var controllers = typeof(MvcApplication).Assembly.GetTypes().Where(typeof(IController).IsAssignableFrom);
    foreach (var type in controllers.Where(t => !t.IsDefined(typeof(AllowCsrfAttacksAttribute), true))) {
        var postActions = type.GetMethods()
                                .Where(m => !m.ContainsGenericParameters)
                                .Where(m => !m.IsDefined(typeof(ChildActionOnlyAttribute), true))
                                .Where(m => !m.IsDefined(typeof(NonActionAttribute), true))
                                .Where(m => !m.GetParameters().Any(p => p.IsOut || p.ParameterType.IsByRef))
                                .Where(m => m.IsDefined(typeof(HttpPostAttribute), true));

        foreach (var action in postActions) {
            //CSRF XOR AntiForgery
            Assert.IsTrue(action.IsDefined(typeof(AllowCsrfAttacksAttribute), true) != action.IsDefined(typeof(ValidateAntiForgeryTokenAttribute), true),
                            action.Name + " is [HttpPost] but not [ValidateAntiForgeryToken]");
        }
    }
}

typeof(MvcApplication) must be any type in the assembly that contains your controllers.  If your controllers are defined in multiple assemblies, you’ll need to include those assemblies too.

12/14/2011 The Dark Side of Covariance

What’s wrong with the following code?

var names = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
...
if (names.Contains(sqlCommand.ExecuteScalar())

This  code is intended to check whether the result of a SQL query is contained in a case-insensitive collection of names.  However, if you run this code, the resulting check will be case-sensitive.  Why?

As you may have guessed from the title, this is caused by covariance.  In fact, this code will not compile at all against .Net 3.5. 

The problem is that ExecuteScalar() returns object, not string.  Therefore, it doesn’t call HashSet<string>.Contains(string), which is what it’s intending to call (and which uses the HashSet’s comparer).  Instead, on .Net 4.0, this calls the  Enumerable.Contains<object>(IEnumerable<object>, string) extension method, using the covariant conversion from IEnumerable<string> to IEnumerable<object>.  Covariance allows us to pass object to the Contains method of any strongly-typed collection (of reference types).

Still, why is it case-sensitive?  As Jon Skeet points out, the LINQ Contains() method is supposed to call any built-in Contains() method from ICollection<T>, so it should still use the HashSet’s case-insensitive Contains().

The reason is that although HashSet<String> implements ICollection<string>, it does not implement ICollection<object>.  Since we’re calling Enumerable.Contains<object>, it checks whether the sequence implements ICollection<object>, which it doesn’t.  (ICollection<T> is not covariant, since it allows write access)

Fortunately, there’s a simple fix: just cast the return value back to string (and add a comment explaining the problem).  This allows the compiler to call HashSet<string>.Contains(string), as was originally intended.

//Call HashSet<string>.Contains(string), not the
//covariant Enumerable.Contains(IEnumerable<object>, object)
//http://blog.slaks.net/2011/12/dark-side-of-covariance.html
if (names.Contains((string)sqlCommand.ExecuteScalar())

(I discovered this issue in my StringListConstraint for ASP.Net MVC)

12/07/2011 CAPTCHAs do not mitigate XSS worms

One common misconception about web security is that protecting important actions with CAPTCHAs can prevent XSS attacks from doing real damage.  By preventing malicious code from scripting critical tasks, the idea goes, XSS injections won’t be able to accomplish much.

This idea is dangerously wrong. 

First of all, this should not even be considered except as a defense-in-depth mechanism.  Regardless of whether the actions you care about are protected by CAPTCHAs, XSS attacks can create arbitrary UI on your pages, and can thus make “perfect” phishing attacks.

Also, even with CAPTCHAs, an XSS injection can wait until the user performs the critical action, then change the submitted data to the attacker’s whim.

For example, if Twitter took this approach to prevent XSS injections from sending spammy tweets, the attacker could simply wait until the user sends a real tweet, then silently append advertising to the tweet as the user submits it and fills out the CAPTCHA.

However, there is also a more fundamental issue.  Since the injected Javascript is running in the user’s browser, it simply display the CAPTCHA to the user and block all page functionality until the user solves the CAPTCHA.  The attacker can even put his own text around the CAPTCHA to look like a legitimate security precaution, so that the (typical) user will not realize that the site has been compromised.  (that could be prevented by integrating a description of the action being performed into the CAPTCHA itself in a way that the attacker can’t hide)

I haven’t even mentioned the inconvenience of forcing all legitimate, uncompromised users to fill out CAPTCHAs every time they do anything significant.

In summary, CAPTCHAs should only be used to prevent programs from automatically performing actions (eg, bulk-registering Google accounts), and as a rate-limiter if a user sends too many requests too quickly (eg, getting a password wrong too many times in a row).

XSS can only be stopped by properly encoding all user-generated content that gets concatenated into markup (whether HTML, Javascript, or CSS)

11/20/2011 About Concurrent Collections

One of the most useful additions to the .Net 4.0 base class library is the System.Collections.Concurrent namespace, which contains an all-new set of lock-free thread.

However, these collections are noticeably different from their classical counterparts.  There is no simple ConcurrentList<T> that you can drop into your code so that it will become thread-safe.  Instead, the new namespace has a queue, a stack, and some new thing called a bag, as well as ConcurrentDictionary<TKey, TValue> that largely resembles classical dictionaries.  It also has a BlockingCollection<T> class that wraps a concurrent collection and blocks until operations can succeed.

Many people have complained that Microsoft chose to provide an entirely new set of classes, rather than adding synchronized versions of the existing collections. 

In truth, however, creating this new set of classes is the correct – and, in fact, only – choice.  Ordinary synchronized are rarely useful and will not make a program thread-safe.  In general, slapping locks everywhere does not make a program thread-safe!  Rather, that will either not help (if there aren’t enough locks) or, if there are enough locks, result in deadlocks or a program that never runs more than one thread at a time.  (depending on how many different objects get locked)

Collections in particular have two fundamental issues when used on multiple threads.

The first, and simpler, issue is that the collection classes themselves are not thread-safe.  If two threads add to a List<T> at the same exact time, one thread is likely to overwrite the other thread’s value.  A synchronized version of List<T> with a lock around every method would solve this problem.

The bigger issue is that any code that uses a List<T> is unlikely to be thread-safe, even if the list itself is thread-safe.  For example. you can never enumerate over a multi-threaded list, because another thread may change the list at any time, invalidating the enumerator.  This issue could be solved by taking a read lock for the lifetime of the enumerator.  However, that is also a bad idea, since if any client code forgets to dispose the enumerator, the collection will deadlock when written to.

You also cannot use indices.  It is never safe to get, set, or remove an item at an index, because another thread might remove that item or clear the entire collection between your index check and the operation.

To solve all of these problems, you need thread-safe collections that provide atomic operations.  This is why all of the concurrent collections have such strange methods, including TryPop, AddOrUpdate, and TryTake.  These methods perform their operations atomically, and return false if the collection was empty (as appropriate; consult the documentation for actual detail).  Thus, the new concurrent collections can be used reliably in actual multi-threaded code without a separate layer of locks.

11/08/2011 Beware of Response.RedirectToRoute in MVC 3.0

ASP.Net MVC uses the new (to ASP.Net 3.5) Http*Base wrapper classes (HttpContextBase, HttpRequestBase, HttpResponseBase, etc) instead of the original Http* classes.  This allows you to create mock implementations that inherit the Http*Base classes without an actual HTTP request.  This is useful for unit testing, and for overriding standard behaviors (such as route checking).

In ordinary MVC code, the HttpContext, Request, and Response properties will return Http*Wrapper instances that directly wrap the original Http* classes (eg, HttpContextWrapper, which wraps HttpContext).  Most MVC developers use the HttpContext and related properties without being aware of any of this redirection.

Until you call Response.RedirectToRoute.  This method, which is new to .Net 4.0, redirects the browser to a URL for a route in the new ASP.Net routing engine.  Like other HttpResponse methods, HttpResponseBase has its own version of this method for derived classes to override.

However, in .Net 4.0, Microsoft forgot to override this method in the standard HttpResponseWrapper.  Therefore, if you call Response.RedirectToRoute in an MVC application (where Response is actually an HttpResponseWrapper), you’ll get a NotImplementedException.

You can see this oversight in the methods list for HttpResponseWrapper.  Every method except for RedirectToRoute and RedirectToRoutePermanent are list as (Overrides HttpResponseBase.MethodName().); these methods are listed as (Inherited from HttpResponseBase.MethodName().)

To work around this issue, you can either use the original HttpResponse by writing HttpContext.Current.Response.RedirectToRoute(…) or by calling Response.Redirect instead.

Note that most MVC applications should not call Response.Redirect or Response.RedirectToRoute at all; instead, they should return ActionResults by calling helper methods like return Redirect(…); or return RedirectToAction(…);

In the upcoming ASP.Net 4.5 release, these methods have been properly overridden.

10/23/2011 Caller Info Attributes vs. Stack Walking

People sometimes wonder why C# 5 needs to add caller info attributes, when this information is already available by using the StackTrace class.  In reality, caller info attributes behave rather differently from the StackTrace class, for a number of reasons.

Advantages to Caller Info Attributes

The primary reason to use caller info attributes is that they’re much faster.  Stack walking is one of the slowest internal (as opposed to network IO) things you can do in .Net (disclaimer: I haven’t measured).  By contrast, caller info attributes have exactly 0 performance penalty.  Caller info is resolved at compile-time; the callsite is compiled to pass a string or int literal that was determined by the compiler.  Incidentally, this is why C# 5 doesn’t have [CallerType] or [CallerMemberInfo] attributes; the compiler team wasn’t happy with the performance of the result IL and didn’t have time to implement proper caching to make it faster.

Caller info attributes are also more reliable than stack walking.  Stack walking will give incorrect results if the JITter decides to inline either your method or its caller.  Since caller info attributes are resolved at compile-time, they are immune to inlining.  Also, stack walking can only give line number and filename info if the calling assembly’s PDB file is present at runtime, whereas [CallerFileName] and [CallerLineNumber] will always work. 

Caller info attributes are also unaffected by compiler transformations.  If you do a stack walk from inside a lambda expression, iterator, or async method, you’ll see the compiler-generated method; there is no good way to retrieve the name of the original method.  Since caller info attributes are processed by the compiler, [CallerMemberName] should correctly pass the name of the containing method (although I cannot verify this).  Note that stack walking will retrieve the correct line number even inside lambda expressions (assuming there is a PDB file)

Advantages To Stack Walking

There are still a couple of reasons to use the StackTrace class.  Obviously, if you want to see more than one frame (if you want your caller’s caller), you need to use StackTrace.  Also, if you want to support clients in languages that don’t feature caller info attributes, such as C# 4 or F#, you should walk the stack instead.

Stack walking is the only way to find the caller’s type or assembly (or a MemberInfo object), since caller info attributes do not provide this information.

Stack walking is also the only choice for security-related scenarios.  It should be obvious that caller info attributes can trivially be spoofed; nothing prevents the caller from passing arbitrary values as the optional parameter.  Stack walking, by contrast, happens within the CLR and cannot be spoofed. 

Note that there are some other concerns with stack walking for security purposes.

• Filename and line number information can be spoofed by providing a modified PDB file
• Class and function names are meaningless; your enemy can name his function whatever he wants to.  All you should rely on is the assembly name.
• There are various ways for an attacker to cause your code to be called indirectly (eg, DynamicInvoking a delegate) so that his assembly isn’t the direct caller.  In fact, if the attacker invokes your delegate on a UI thread (by calling BeginInvoke), his assembly won’t show up on the callstack at all.  The attacker can also compile a dynamic assembly that calls your function and call into that assembly.
• As always, beware of inlining