Over the last week a few stories have moved through the Ruby On Rails and wider development community as one of their shining stars, GitHub was hacked to draw attention to some of the weaknesses that can come about from ROR’s convention-based model binding. The interesting thing about the security hole found at GitHub is that it is not necessarily limited to Ruby On Rails, but often comes from using a framework that supports model binding out of the box without understanding the security limitations up front. It also brings a question to the fore: Is it the role of framework developers to force any security configuration to be the default instead of being explicitly applied?
A ROR developer Egor Homakov, found a configuration vulnerability in the ROR framework’s scaffolding. This vulnerability affected all ROR sites that had been developed using the basic scaffolding that comes out of the box without consideration for security issues, and its implementation of relationship links between data access classes.
Initially Egor, tried to bring attention to this by posting an Issue to the Ruby On Rails Github site. After not receiving the response he thought the Rails community should have been giving to such a wide affecting security configuration bug, posted a message with a timestamp 1,000 years in the future and then added his private key to the Rails group and uploaded a (completely innocuous) file to the Rails Git repository – all to prove his point.
What I'll show you below, is the problem that GitHub had, how this can also occur in a hypothetical situation in ASP.Net MVC, and what you can do about it.
Let’s Run Over The Problem
Ruby On Rails is a pretty slick Ruby framework for building websites fast, and one of the main ways it does this is by taking a database and scaffolding a site from it, with everything required to get up and running.
In Ruby On rails, If you have a data model that looks like this (the foreign key would actually not be included in your model object, ROR injects this for you):
When you scaffold the tables you get code that looks like this (I've removed any code we aren’t going to talk about):
# == Schema Information # Table name: users # # id :integer(11) not null, primary key # email :string(255) # username :string(255) # password :string(255) class User < ActiveRecord::Base belongs_to :securityroles validates_presence_of :email, :password validates_uniqueness_of :email ... #class scaffolding code ... end # == Schema Information # Table name: securityroles # # id :integer(11) not null, primary key # roleName :string(255) not null class SecurityRole < ActiveRecord::Base has_many :users validates_presence_of :roleName ... #class scaffolding code ... def update @user = User.find(params[:id]) respond_to do |format| if @user.update_attributes(params[:user]) flash[:notice] = 'User was successfully updated.' format.html { redirect_to(@user) } else format.html { render :action => "edit" } end end end end
You’ll notice above that the main thing to be concerned about with this scaffolded code is the part where it says this:
def update @user = User.find(params[:id]) respond_to do |format| if @user.update_attributes(params[:user]) flash[:notice] = 'User was successfully updated.' format.html { redirect_to(@user) } else format.html { render :action => "edit" } end end end
What ROR is doing here is a feature they call “Mass assignment”.
What the above code is doing:
- Pulling a User record from the database using the incoming parameter id to find it.
- Binding all the other incoming Request parameters to the data model loaded from the database and updating them.
- Saving the new model back to the database.
Egor’s attack on GitHub, was dead simple in that he simply added a new field to the page that submitted new SSH keys for a user that had the field name of id, and he then proceeded to make this field’s value the id for the Ruby On Rails project owner. This caused the above default scaffolding code to save his GitHub SSH Private key not to his own GitHub account, but to the Ruby On Rails GitHub Account. Sneaky. Also a very basic oversight by GitHub.
To be fair to Ruby On Rails, there is a way to stop this kind of attack by marking your class with the property attr_accessible nil to turn off mass assignment support for a class unless specified so you can white list only the fields you want to bind by hand – the problem with this feature is that it is not the default. You have have to do this manually – and we all know how easy it to skip or forget about these things in the rush to meet a deadline.
In ASP.net MVC this ROR functionality would be similar to our model binding. Obviously the code that the ROR scaffolding tool creates is a lot more functional than we get out of the box with ASP.Net MVC, but the CRUD functionality they are creating is very similar across all web platforms. In ASP.Net MVC you have to write your own CRUD code to add, edit and update records and ASP.Net MVC tries to make this simpler by adding Model Binding into the mix.
In some ways the security concerns this raises are greater because as if you are a junior developer you might not be clear on any potential nasties that can come about if when you write insecure code, or what this even includes.
What This Behaviour Look Like In Practice
To take a look at what this would entail in ASP.Net MVC let’s think about how the above Ruby on Rails data model application would actually play out in real life using ASP.Net MVC.
You would most probably have a membership based site where users log in, have different levels of access to things, and can change their settings.
If you weren’t switched on and you were new to ASP.Net MVC and web development it would be easy to write a user settings page that looked like this:
@using (Html.BeginForm()) { @Html.ValidationSummary() <div> <fieldset> <legend>Account Information</legend> <div class="editor-label"> @Html.LabelFor(m => m.Name) </div> <div class="editor-field"> @Html.TextBoxFor(m => m.Name) @Html.ValidationMessageFor(m => m.Name) </div> <div class="editor-label"> @Html.LabelFor(m => m.Email) </div> <div class="editor-field"> @Html.TextBoxFor(m => m.Email) @Html.ValidationMessageFor(m => m.Email) </div> <p> <input type="submit" value="Change Account Details" /> </p> </fieldset> </div> }
And your settings controller could look like:
[Authorize] public ActionResult ChangeAccountDetails() { var dbUser = DAL.UserRepository.Load(x => x.Username = User.Identity.Name); var model = new UserModel(dbUser); return View(model); } [Authorize] [HttpPost] public ActionResult ChangeAccountDetails(UserModel model) { DAL.User dbUser = DAL.UserRepository.Load(x => x.Username = User.Identity.Name); if (TryUpdateModel(dbUser)) { DAL.User.Save(dbUser); return RedirectToAction("ChangeDetailsSuccess"); } ModelState.AddModelError("", "Something went wrong"); return View(model); }
What’s Wrong With This Code?
If you’re stumped looking at the above code to find any issues, it’s time to start thinking about one of the golden rules of web development security; White Listing.
The exact same problem that affects the Ruby On Rails community in the GitHub hack with their mass assignment “bug” (if we even call it that), is that the above ASP.Net MVC code blindly binds any incoming request parameters to our database model.
This is exactly the same problem that allowed Egor to be able to hack GitHub:
We don’t specify what incoming parameters we actually care about.
This can lead to all manner of implicit security or data quality issues on your sites, because users can potentially hijack your own code for other purposes – it’s kind of like the modern equivalent of SQL Injection, but through binding code!
How could we hack the above piece of MVC code?
The problem that could affect any ASP.Net MVC site that isn’t verifying all incoming user submitted data, usually comes about not when the site is used as intended – but when new parts are added to the request.
- The site gives us data.
- We send the updated data back.
- But we also added something sneaky along for the ride.
Lets change our Account Settings page, and add a new field.
@using (Html.BeginForm()) { @Html.ValidationSummary() <div> <fieldset> <legend>Account Information</legend> <div class="editor-label"> @Html.LabelFor(m => m.Name) </div> <div class="editor-field"> @Html.TextBoxFor(m => m.Name) @Html.ValidationMessageFor(m => m.Name) </div> <div class="editor-label"> @Html.LabelFor(m => m.Email) </div> <div class="editor-field"> @Html.TextBoxFor(m => m.Email) @Html.ValidationMessageFor(m => m.Email) </div> <!-- #HACK ATTACK# THIS FIELD WILL SET OUR ROLE ID TO "1" --> <input type="text" name="SecurityRoles_Id" value="1"/> <!-- --> <p> <input type="submit" value="Change Account Details" /> </p> </fieldset> </div> }
- The site gives us our page.
- We add a new field, named SecurityRoles_Id to the page using Firebug etc.
- When we submit the page back the site would set our security role foreign key to 1.
You can see from my above view code that if I added this field in Firebug or developer tools and submitted it back to the site, I would have changed my role in the site’s database to whatever role was stored in the database’s roles tables with the Id of 1.
Think back to when you first set up your database – What was the first user (record number 1) you added to your database, an Admin? What was the first role (record number 1) you added to your Security Roles table, Admin?
This problem is ever simpler if you have a user table that simply has a Boolean field called something like “IsAdmin” to define the users permissions as an attacker wouldn’t have to guess what the id of a nice juicy role is set to.
I understand that looking at the above attack you have to know that my database field was called securityroles_id but if you ask anyone working in web security they’ll tell you have unless a site has also locked down all its error messaging or tracing this can be sometimes easy enough to find out using other attacks first to probe your data model and review the errors thrown.
Batten Down The Hatches
Whitelist your submitted data
When visitors to your site submit data back to you, always white list the fields you are saving to the database. This means manually bind or copy only the fields you want to save from the request – don’t just bind the whole model to a database record.
Use model binding carefully with sensitive data to avoid any accidental oversights while developing your application.
ASP.Net MVC supports protection against this in a similar way to Ruby On Rails’ attr_accessible nil by enabling your controller action to use the Bind Attribute [Bind(Include=*explicit properties here*)] when defining your incoming model binding. Just like ROR however, this is not turned on by default…
public ActionResult ChangeAccountDetails([Bind(Include = "Name,Email")] User model) { ... }
If your model class has many fields you can work backwards, and exclude properties from binding instead;
public ActionResult ChangeAccountDetails([Bind(Exclude = "SecurityRoles_Id")] User model) { ... }
You can also use the additional overloads of the TryUpdateModel method to define what objects to bind, or not bind from the FormCollection if you are using this method of model binding/validation.
Include;
[Authorize] [HttpPost] public ActionResult ChangeAccountDetails(UserModel model) { DAL.User dbUser = DAL.UserRepository.Load(x => x.Username = User.Identity.Name); string[] includeInModel = new[] { "EmailAddress" }; if (TryUpdateModel(dbUser, includeInModel)) { DAL.User.Save(dbUser); return RedirectToAction("ChangeDetailsSuccess"); } ModelState.AddModelError("", "Something went wrong"); return View(model); }
Exclude;
[Authorize] [HttpPost] public ActionResult ChangeAccountDetails(UserModel model) { DAL.User dbUser = DAL.UserRepository.Load(x => x.Username = User.Identity.Name); string[] excludeInModel = new[] { "SecurityRoles_Id" }; if (TryUpdateModel(dbUser, string.Empty, null, excludeInModel)) { DAL.User.Save(dbUser); return RedirectToAction("ChangeDetailsSuccess"); } ModelState.AddModelError("", "Something went wrong"); return View(model); }
Also, if you have complex types in your view model (Entity Framework objects etc.) you should abstract your data layer away into a separate project. Other than the fact that you should never have complex types of dependencies in your models, doing this ensures that even if you do step on a land mine by accident, you aren’t talking directly to the database and can have a little more control. If you are only new to this additional separation and are cringing at the thought of all the additional code you’ll have to write, take a look at something like AutoMapper to do this for you (ASP.Net MVC 3 has the TryUpdateModel method that I used in my first example that does this for you as well – albeit with less control).
public ActionResult ChangeAccountDetails(UserModel model) { //create a new User object DAL.User u = new DAL.User(); // map the incoming model to the user object AutoMapper.Mapper.Map(u, model); // // Some code that attaches our object // to the current record in the DB that // matches User.Identity.Name // DAL.User.Save(u); return RedirectToAction("ChangeDetailsSuccess"); }
Turn Off Any Exception Signalling
Ensure that all tracing and exceptions are turned off on your site to avoid probing. This means turning friendly error messages on to avoid any exceptions being handed out to attackers wanting to test the waters, ensuring tracing is turned off, and take a read of Troy Hunt series on OWASP security vulnerabilities that can often occur during application development without you being aware.
Machine.config
<configuration> <system.web> <!-- turn on retail mode --> <deployment retail="true" /> </system.web> </configuration>
Web.config
<configuration> <system.web> <!-- turn debug mode off --> <compilation debug="false"/> <!— turn On custom errors --> <customErrors mode="RemoteOnly"/> <!-- turn tracing off (this is set to false by default) --> <trace enabled="false"/> </system.web> </configuration>
Summary
While more senior web developers working on the ASP.Net MVC stack may think the issues I've raised are too hypothetical and easy to overcome, I can assure you from much experience working with developers of all sorts of skill levels over the years that these types of inherent “you’d never do that, it’s silly” problems are usually the ones I’ve see most.
GitHub’s problems were more related to the way ROR does it’s scaffolding, but it just goes to show that development techniques that save time by writing the code for you (such as ASP.Net MVC’s Model Binding) can often lead to a disconnect between the developer and his application’s code – if a large company like GitHub can miss these problems, its proof that these problems are more of a commonplace occurrence than we’d all like to think.
When saving any user generated data, be sure to only save what you intend to for a given action, and ensure they have permission to do so before putting saving it to your database.