Add CI-enforced Secure Code Guideline for use of params

With Add defense-in-depth against mass assignment in... (!149029 - merged) we went and added some defense-in-depth to existing code. Now we should try to prevent issues occurring in the future by updating the Secure Code Guidelines, and enforcing it with a rubocop.

Context

In a Rails controller params[:key] could be a String, an Integer, an Array, a File, all sorts of things. It becomes a problem when a developer doesn't expect this in their code.

For example: attempting to brute force a password by sending many passwords in a single request for a single user should never work. Nor should sending multiple OTP codes.

# POST /vulnerable?email=fake@attacker.com
# params[:email] is a String
> User.find_by(email: params[:email])
# User Load (3.4ms)  SELECT "users".* FROM "users" WHERE "users"."email" = 'fake@attacker.com' LIMIT 1
=> nil

# We expect email to be a string, but what if it's not?
# POST /vulnerable?email[]=fake@attacker.com&email[]=admin@example.com
# params[:email] is an Array of Strings
> User.find_by(email: params[:email])
# User Load (1.6ms)  SELECT "users".* FROM "users" WHERE "users"."email" IN ('fake@attacker.com', 'admin@example.com')
=> #<User id:1 @root>

Real Example

Account Takeover via Password Reset without use... (#436084 - closed) was a severity1 issue that could have been prevented by StrongParams.

1. A user can reset their password by passing an email address in a parameter ?user[email]=VALUE; a Hash with a String parameter
2. An attacker passes ?user[email][]=victim@example.com&user[email][]=attacker@example.com; a Hash with an Array parameter
3. This endpoint calls create in Devise::PasswordsController

   class Devise::PasswordsController < DeviseController
     # POST /resource/password
     def create
       self.resource = resource_class.send_reset_password_instructions(resource_params)

   class DeviseController < Devise.parent_controller.constantize
     def resource_params
       params.fetch(resource_name, {}) # ActionController::Parameters.fetch doesn't care about type / permitting
     end

   # For the victim's normal call
   > params = ActionController::Parameters.new({user: {email: "victim@example.com"}})
   > params.fetch(:user, {})[:email]
   => "victim@example.com"
   
   # For the attacker's call
   > params = ActionController::Parameters.new({user: {email: ["victim@example.com", "attacker@example.com"]}})
   => #<ActionController::Parameters {"user"=>{"email"=>["victim@example.com", "attacker@example.com"]}} permitted: false>
   > params.fetch(:user, {})[:email]
   => ["victim@example.com", "attacker@example.com"]

4. This value, whether it was a String or Array, was then passed to app/models/concerns/recoverable_by_any_email.rb:

     def send_reset_password_instructions(attributes = {})
       email = attributes.delete(:email)
       record = find_by_any_email(email, confirmed: true) || new 
       # This returned the victim. 
       # In `user.rb` it eventually gets passed to a Users#where type call.
       # It returned the victim without error because the first value of the array is the victim's email
   
       recoverable.send_reset_password_instructions(to: email) if recoverable&.persisted? 
       # This sent the email to both the victim and the attacker, 
       # because Mailer accepted (at the time) both a String or Array of email addresses

Solution

Rails has StrongParameters which forces you to permit explicit params. When this gets passed to an ActiveModel it will fail to assign a parameter that hasn't been permitted. In addition, the object returned by params.permit will have extra or incorrectly-typed attributes removed.

Proposal

• Add a rubocop which raises for use of params in controllers
  • Write the rule
  • Generate the long list of TODOs

Then:

• Open an issue to start clearing out the TODO and/or improving the rule
• Open an issue to identify gems where we extend their controllers (e.g. Devise), and work through any areas of risk