Ruby: Add query for insecure mass assignment

[joefarebrother]

This query looks for instances of request parameters having permit! called on them, which allows arbitrary parameters to be specified by the request, and then being used for a mass assignment operation.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp

Insecure Mass Assignment

Operations that allow for mass assignment (setting multiple attributes of an object using a hash), such as ActiveRecord::Base.new, should take care not to allow arbitrary parameters to be set by the user. Otherwise, unintended attributes may be set, such as an isAdmin feild for a User object.

Recommendation

When using a mass assignment operation from user supplied parameters, use ActionController::Parameters#permit to restrict the possible parameters a user can supply, rather than ActionController::Parameters#permit!, which permits arbitrary parameters to be used for mass assignment.

Example

In the following example, permit! is used which allows arbitrary parameters to be supplied by the user.

class UserController < ActionController::Base
    def create
        # BAD: arbitrary params are permitted to be used for this assignment
        User.new(user_params).save!
    end

    def user_params
        params.require(:user).permit!
    end
end

In the following example, only specific parameters are permitted, so the mass assignment is safe.

class UserController < ActionController::Base
    def create
        # GOOD: the permitted parameters are explicitly specified
        User.new(user_params).save!
    end

    def user_params
        params.require(:user).permit(:name, :email)
    end
end

References

• Common Weakness Enumeration: CWE-915.

[hmac]

This looks great. I have only some minor comments. DCA shows a significant slowdown for one big Rails repo. I think it's worth running against the rails-projects source suite to see if there's a pattern there that we need to look into.

I also noticed in the Rails guide that you can do

params.permit(preferences: {})

which allows arbitrary data inside preferences. We should probably extend the query to cover that case, but that can be a follow-up and doesn't need to block this PR.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp

errors/warnings:

/home/runner/work/codeql/codeql/ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp:32:29: Open quote is expected for attribute "href" associated with an  element type  "a".
A fatal error occurred: 1 qhelp files could not be processed.

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp

Insecure Mass Assignment

Operations that allow for mass assignment (setting multiple attributes of an object using a hash), such as ActiveRecord::Base.new, should take care not to allow arbitrary parameters to be set by the user. Otherwise, unintended attributes may be set, such as an is_admin field for a User object.

Recommendation

When using a mass assignment operation from user supplied parameters, use ActionController::Parameters#permit to restrict the possible parameters a user can supply, rather than ActionController::Parameters#permit!, which permits arbitrary parameters to be used for mass assignment.

Example

In the following example, permit! is used which allows arbitrary parameters to be supplied by the user.

class UserController < ActionController::Base
    def create
        # BAD: arbitrary params are permitted to be used for this assignment
        User.new(user_params).save!
    end

    def user_params
        params.require(:user).permit!
    end
end

In the following example, only specific parameters are permitted, so the mass assignment is safe.

class UserController < ActionController::Base
    def create
        # GOOD: the permitted parameters are explicitly specified
        User.new(user_params).save!
    end

    def user_params
        params.require(:user).permit(:name, :email)
    end
end

References

• Rails guides: Strong Parameters.
• Common Weakness Enumeration: CWE-915.

[hmac]

👌

[asgerf]

I came looking out of curiosity, looks great, but I'll defer to @hmac's review. Just a few drive-by comments

[hvitved]

It's better to use CFG nodes instead of AST nodes here. I.e., use HashLiteralCfgNode instead of HashLiteral, etc.