Auditing ASP.NET MVC Actions

aspnetmvc security 12 comments suggest edit

Phil Haack is writing a blog post about ASP.NET MVC? What is this, 2011?

No, do not adjust your calendars. I am indeed writing about ASP.NET MVC in 2017.

It’s been a long time since I’ve had to write C# to put food on the table. My day job these days consists of asking people to put cover sheets on TPS reports. And only one of my teams even uses C# anymore, the rest moving to JavaScript and Electron. On top of that, I’m currently on an eight week leave (more on that another day).

But I’m not completely disconnected from ASP.NET MVC and C#. Every year I spend a little time on a side project I built for a friend. He uses the site to manage and run a yearly soccer tournament.

Every year, it’s the same rigmarole. It starts with updating all of the NuGet packages. Then fixing all the breaking changes from the update. Only then do I actually add any new features. At the moment, the project is on ASP.NET MVC 5.2.3.

I’m not ready to share the full code for that project, but I plan to share some interesting pieces of it. The first piece is a little something I wrote to help make sure I secure controller actions.

The Problem

You care about your users. If not, at least pretend to do so. With that in mind, you want to protect them from potential Cross Site Request Forgery attacks. ASP.NET MVC includes helpers for this purpose, but it’s up to you to apply them.

By way of review, there are two steps to this. The first step is to update the view and add the anti-forgery hidden input to your HTML form via the Html.AntiForgeryToken() method. The second step is to validate that token in the action that receives the form post. Do this by decorating that action method with the [ValidateAntiForgeryToken] attribute.

You also care about your data. If you have actions that modify that data, you may want to ensure that the user is authorized to make that change via the [Authorize] attribute.

This is a lot to track. Especially if you’re in a hurry to build out a site. On this project, I noticed I forgot to apply some of these attributes where they should be placed. When I fixed the few places I happened to notice, I wondered what places did I miss?

It would be tedious to check every action by hand. So I automated it. I wrote a simple controller action that reflects over every controller action. It then displays all the actions that might need one of these attributes.

Here’s a screenshot of it in action.

Screenshot of Site Checker in action

There’s a few important things to note.

Which actions are checked?

The checker looks for all actions that might modify an HTTP resource. In other words, any action that responds to the following HTTP verbs: POST, PUT, PATCH, DELETE. In code, these correspond to action methods decorated with the following attributes: [HttpPost], [HttpPut], [HttpPatch], [HttpDelete] respectively. The presence of these attributes are good indicators that the action method might modify data. Action methods that respond to GET requests should never modify data.

Do all these need to be secured?

No.

For example, it wouldn’t make sense to decorate your LogOn action with [Authorize] as that violates causality. You don’t want to require users to be already authenticated before the log in to your site. That’s just silly sauce.

There’s no way for the checker to understand the semantics of your action method code to determine whether an action should be authorized or not. So it just lists everything it finds. It’s up to you to figure out if there’s any action (no pun intended) required on your part.

How do I deploy it?

All you have to do is copy and paste this SystemController.cs file into your ASP.NET MVC project. It just makes it easier to compile this into the same assembly where your controller actions exist.

Next, make sure there’s a route that’ll hit the Index action of the SystemController. If you have the default route that ASP.NET MVC project templates include present, you would visit this at /system/index.

Be aware that if you accidentally deploy SiteController, it will only responds to local requests (requests from the hosting server itself) and not to public requests. You really don’t want to expose this information to the public. That would be an open invitation to be hacked. You may like being Haacked, it’s no fun to be hacked.

And that’s it.

How’s it work?

I kept all the code in a single file, so it’s a bit ugly, but should be easy to follow.

The key part of the code is how I obtain all the controllers.

var assembly = Assembly.GetExecutingAssembly();

var controllers = assembly.GetTypes()
    .Where(type => typeof(Controller).IsAssignableFrom(type)) //filter controllers
    .Select(type => new ReflectedControllerDescriptor(type));

The first part looks for all types in the currently executing assembly. But notice that I wrap each type with a ReflectedControllerDescriptor. That type contains the useful GetCanonicalActions() method to retrieve all the actions.

It would have been possible for me to get all the action methods without using GetCanonicalActions by calling type.GetMethods(...) and filtering the methods myself. But GetCanonicalActionsis a much better approach since it encapsulates the same logic ASP.NET MVC uses to locate actions.

As such, it handles cases such as when an action method is named differently from the underlying class method via the [ActionName("SomeOtherMethod")] attribute.

What’s Next?

There’s so many improvements we could make (notice how I’m using “we” in a bald attempt to pull you into this?) to this. For example, the code only looks at the HTTP* attributes. But to be completely correct, it should also check the [AcceptVerbs] attribute. I didn’t bother because I never use that attribute, but maybe you have some legacy code that does.

Also, there might be other things you want to check. For example, what about mass assignment attacks? I didn’t bother because I tend to use input models for my action methods. But if you use the [Bind] attribute, you might want this checker to look for issues there.

Well that’s great. I don’t plan to spend a lot of time on this, but I’d be happy to accept your contributions! The source is on GitHub.

Let me know if this is useful to you or if you use something better.

Found a typo or error? Suggest an edit! If accepted, your contribution is listed automatically here.

Comments

avatar

12 responses

  1. Avatar for johnkors
    johnkors August 10th, 2017

    Another option is a global Auth filter + a "NoAuthorize"-attribute on the logon-action. So auth by default, then opt out explicitly on the few that don't need it.

  2. Avatar for Alexandre Konioukhov
    Alexandre Konioukhov August 10th, 2017

    why not ASP.NET Core?

  3. Avatar for Vahid N.
    Vahid N. August 10th, 2017

    You can try the roslyn-security-guard extension too.
    https://github.com/dotnet-s...

  4. Avatar for Tim
    Tim August 11th, 2017

    I've done something very similar with a recent project. One difference is that I wrapped the checks in a reusable library and then execute them as unit tests in each solution on every build -- failing the build if they fail. (I pass in a reference to the main site assembly to do the checks.) This helps as the analysis is not in deployable code.

    The unit tests looks for some form of authorize on all actions (ignoring actions when AllowAnonymous or ChildActionOnly attributes are used -- the former helping out in being declarative in intent for a login page not needing authentication.) Additionally, like in your example, the tests look for non-get actions and ensure that the anti-forgery validation attribute is present. Another situation we also check for (since we are using OAuth) is to ensure that no non-get action is decorated to be invoked by a user with 'read-only' access -- This is a little more solution specific...

    To make the tests repeatable in a build environment, the tests allow a pre-configured list (in the unit test definition) of ignore patterns to be passed in to the library methods so that if there is a reason to break convention, the method signature can be ignored on a case-by-case basis allowing the tests to pass once reviewed.

    One might argue that these 'unit tests' are more of code convention policies and/or code analysis rules, but we've found that forcing them on the build server ensures lazy developers are held accountable...

  5. Avatar for haacked
    haacked August 11th, 2017

    What you've done is how I'd like this to end up. It's much better than my ad-hoc solution.

  6. Avatar for haacked
    haacked August 11th, 2017

    I'm waiting for 2.0. :)

  7. Avatar for Alexandre Konioukhov
    Alexandre Konioukhov August 14th, 2017

    released today

  8. Avatar for Robert Armour
    Robert Armour August 25th, 2017

    Phil,

    As you didn't put an [Authorize] attribute on your SystemController class, anybody who read this article will now be trying to activate the 'system/index' endpoint on your sites, in order to obtain a handy list of potential vulnerabilities.

    Just saying ;)

  9. Avatar for haacked
    haacked August 25th, 2017

    Ha! Well if you read the post carefully, you'll see I addressed that.

    > Be aware that if you accidentally deploy SiteController, it will only responds to local requests (requests from the hosting server itself) and not to public requests. You really don’t want to expose this information to the public. That would be an open invitation to be hacked. You may like being Haacked, it’s no fun to be hacked.

  10. Avatar for Robert Armour
    Robert Armour August 26th, 2017

    That'll teach me to speed-read ;)

  11. Avatar for 1elrond
    1elrond August 28th, 2017

    Thanks, this is a cool tool. I usually add specific time on my project plan for "security review" to manually go through and verify that I have set the authorization correctly for the controller actions, as you say it is a lot to keep track of and while tedious a little extra testing never hurt anybody.

  12. Avatar for Marvin Haagsma
    Marvin Haagsma February 16th, 2018

    Errors: I see four "Unexpected character '$' errors, followed by many more related to them