CSC/ECE 517 Fall 2017/E1784 Fix mass assignments reported by Brakeman.rb

About Expertiza

Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

Introduction

Background

Mass Assignment is the name Rails gives to the act of constructing your object with a parameters hash. It is "mass assignment" in that you are assigning multiple values to attributes via a single assignment operator.

The following snippets perform mass assignment of the name and topic attribute of the Post model:

• Post.new(:name => "John", :topic => "Something")

• Post.create(:name => "John", :topic => "Something")

• Post.update_attributes(:name => "John", :topic => "Something")

Without the convenience of mass assignment, we'd have to write an assignment statement for each attribute to achieve the same result. Here's an example:

• attrs = { :name => "John", :topic => "Something" }

• post = Post.new

• post.name = attrs[:name]

• post.topic = attrs[:topic]

Obviously, this can get tedious and painful; so we bow at the feet of laziness and say, yes yes, mass assignment is a good thing.

Problem Statement

With the help of mass assignment, when we create or update certain object, we do not need to write an assignment statement for each attribute. But mass assignment could cause security vulnerabilities. Hackers could add other parameters to do some bad things. For example:

• @post = Post.new(params[:post])

Typically this is used when the user submits a form rendered by a form_for @post. In an ideal world, the params[:post] hash should only contain the fields we displayed on the form. However, it is trivial easy hackers to pass additional fields in their request, so in effect you're allowing a user to set any fields on @post, not just the ones displayed on the form.

How to Deal With Mass Assignment? In models, we need to add "attr_accessible" to implement the whitelist. For example:

class User < ActiveRecord::Base
    attr_accessible :first, :last, :email
end

Here, we're explicitly listing out what can be mass-assigned. Everything else will be disallowed. The advantage here is that if we, say, add an admin flag to the User model, it will automatically be safe from mass-assignment.

For controllers, Rails 4 introduces strong parameters, which is a new approach to protect mass assignment. For example:

class UsersController < ApplicationController
 def update
   user = User.find(params[:id])
   if user.update_attributes(user_params) # see below
     redirect_to home_path
   else
     render :edit
   end
 end

 private

 # Require that :user be a key in the params Hash,
 # and only accept :first, :last, and :email attributes
 def user_params
   params.require(:user).permit(:first, :last, :email)
 end
end

Now, if you attempt something like user.update_attributes(params), you'll get an error in your application. You must first call permit on the params hash with the keys that are allowed for a specific action.

So our group needs to resolve all these "Unprotected mass assignment" issues according to Brakeman report.

Implementation

In models, we need to protect all attributes that we allow to be mass-assigned using "attr_accessible". Previously, we just removed all attributes after "attr_accessible" which fixed "Unprotected mass assignment" but would lead to failure when creating and updating models. So the testes could not pass. Therefore, we added all attributes after "attr_accessible" and tests passed. However, this time, Brakeman would report "Potentially dangerous attribute available for mass assignment". In order to solve "Unprotected mass assignment" and ensure that tests can pass, this is the best we can do.

Problem 1: Mass assignment is not restricted using attr_accessible

If there was no protection at all in the model file. We added "attr_accessible" and all attributes that we allow to be mass-assigned after it.

Problem 2: Unprotected mass assignment

We modified the code in an unobtrusive way, i.e. processing the mass assignment parameters with a function without changing them directly.

Parameters come from requests

Fix unprotected mass assignment in update

Fix unprotected mass assignment in new

Parameters come from other objects

If the parameters come from requests, we can directly use "params.permit()" to implement a whitelist. However, if the parameters come from values of other objects, we cannot directly call that built-in function. Firstly, we need to build a hash with those values and pass the hash into "params". And then we can filter the parameters using permit().

Fix unprotected mass assignment in create

Fix unprotected mass assignment in new

params.permit!

Test

The Expertiza project provides 77 rspec tests under expertiza/spec. After modifying over 140 files, we want to make sure these tests could still pass. It turns out all tests could pass except models/tag_prompt_spec.rb.

In tag_prompt_spec.rb, we will create an instance of Criterion and specify the type of it. However, type is a reserved word for ActiveRecord. When we use type as a reserved word, it specifies the name of a model class.

• (1)If we add :type after attr_accessible in Question(Superclass of Criterion) model

Because you add it after attr_accessible, it will allow you to set the value of type. So, the code in tag_prompt_spec.rb above will try to create an instance of "Criterion" or "Checkbox" or "Text" which should be the subclass of Criterion. However, there are no such subclasses Checkbox and Text. Therefore, the test would fail.

• (2)If we do not add :type after attr_accessible in Question(Superclass of Criterion) model

In this situation, when creating Criterion, the information ("Criterion" or "Checkbox" or "Text") of type will be ignored. All three instances are created with type "Criterion" which is subclass of Criterion(itself). However, in the last test, "expect(tp.html_control(tag_dep, an_long_text)).to eql("")" will check the type these three instances. Because the type of last two instances are actually "Criterion", not "Checkbox" or "Text", the test would fail.

Reference

1. https://github.com/expertiza/expertiza
2. https://codeclimate.com/github/expertiza/expertiza/issues?category=security
3. https://apidock.com/rails/ActiveModel/MassAssignmentSecurity/ClassMethods/attr_accessible
4. https://docs.google.com/document/d/1rdolBAHxVGI9I0N-cT866AqnfORM2L1_m_bo2gRYRrI/edit#
5. https://www.happybearsoftware.com/how-i-avoid-the-rails-mass-assignment-security-mistake
6. https://code.tutsplus.com/tutorials/mass-assignment-rails-and-you--net-31695