About the author

Will Barrett is a Software Engineer, Technical Lead, and Engineering Manager from the San Francisco Bay Area with over 14 years of experience. He’s a Superset PMC Member of The Apache Software Foundation. He’s held staff software engineer and senior engineer roles at Change.org, Entelo, Sqwiggle and Preset.

Will is the author of On Learning to Program, a blog for new Software Engineers entering the industry. Will is also a certified reviewer on PullRequest where he’s caught hundreds of bugs, security issues, and other critical issues for over 40 teams.

One of the most important tasks of a code reviewer is to catch security problems before they make it into production. As an engineer in the PullRequest reviewer network, I frequently discover security vulnerabilities in pull requests submitted through the service from various companies, teams, and types of codebases. Building secure web applications is not a simple task, and there are many ways that it can fail. The OWASP #1 vulnerability, Broken Access Control, contains a myriad of issue types.

The Open Web Application Security Project (OWASP) describes Broken Access Control as:

Access control enforces policy such that users cannot act outside of their intended permissions. Failures typically lead to unauthorized information disclosure, modification, or destruction of all data or performing a business function outside the user’s limits. Common access control vulnerabilities include:

1. Violation of the principle of least privilege or deny by default, where access should only be granted for particular capabilities, roles, or users, but is available to anyone.
2. Bypassing access control checks by modifying the URL (parameter tampering or force browsing), internal application state, or the HTML page, or by using an attack tool modifying API requests.
3. Permitting viewing or editing someone else’s account, by providing its unique identifier (insecure direct object references)
4. Accessing API with missing access controls for POST, PUT and DELETE.
5. Elevation of privilege. Acting as a user without being logged in or acting as an admin when logged in as a user.
6. Metadata manipulation, such as replaying or tampering with a JSON Web Token (JWT) access control token, or a cookie or hidden field manipulated to elevate privileges or abusing JWT invalidation.
7. CORS misconfiguration allows API access from unauthorized/untrusted origins.
8. Force browsing to authenticated pages as an unauthenticated user or to privileged pages as a standard user.

From the OWASP Website - numbers added to the list for easy reference in the rest of the article.

1. Violation of the Principle of Least Privilege

The Least Privilege Principle indicates that every user should be allowed to do the minimum number of things necessary to accomplish their goals. So, when we’re reviewing for violations of the Principle of Least Privilege, you look for situations where a system defaults to open. In a Rails application, you might see the following:

class UserController < ApplicationController
	def show
		if current_user && current_user.id != params[:id]
			render :403
			return
		end
		@user = User.find(user_id)
	end
end

At first glance, this seems OK. The part that may not be right is that the system only denies access if a user is logged in - if current_user is nil then this endpoint will still render. There may be a check somewhere else to ensure that current_user is populated, but from what we can see here there is a possible security issue.

Another way this manifests is through role definitions that are overly broad - let’s assume a standard blog structure with User and Post models. If I were to see a role definition called ReadOnly that had write access to all Post objects, that would be cause for concern. When looking at role definitions, the best way to catch problems is to read the role name and description if there is one and, for each permission, ask if the user granting the role would expect that permission to be granted to the user being assigned the role. In this case “If I gave someone a role called read-only, would I expect them to be able to edit posts?” - obviously not.

2. Bypassing Access Control Checks

Catching this security issue in a code review primarily comes down to looking for missing or weak access control checks. There are three places where I most often see this:

• A difference in access control between endpoints protecting the same resource.
• Situations where a missing value will change a query to allow greater access.
• And places where access control parameters are passed up via the client rather than loaded from internal state.

Take this example of access control failure:

class ProductController < ApplicationController
	def index
		@products = Product.where(created_by: current_user.id)
	end
	
	def show
		@product = Product.find(params[:id])
	end
end

Notice that the products loaded in the index action are scoped to those created by the current user - let’s assume for the moment that the access control on index is correct, and users should only be able to access products that they created via this controller. The second endpoint, the show action, then allows a user to access any product in the system by modifying the id sent as part of the request. This would allow a user to modify the id value sent as part of the request and circumvent the access control that’s in place.

Missing parameters expanding query scope is the second common failure I see. In the example below, assume a tree structure of comments, where the Comment model has a parent_id field that points to another comment, like so:

- Comment id: 1, parent_id: NULL
	- Comment id: 3, parent_id: 1
- Comment id: 2, parent_id: NULL
	- Comment id: 4, parent_id: 2
	- Comment id: 5, parent_id: 2
		- Comment id: 6, parent_id: 5
	

Given this controller implementation:

class CommentsController < ApplicationController
	# URL: /comments/:id/children/:parent_id
	def child_comments
		@comment = Comment.find(params[:id])
		if current_user.can?(:view, @comment)
			@child_comments = Comment.where(parent_id: params[:parent_id])
		end
	end
end

The intended use of parent_id is to allow the client to dig down into a tree structure of nested comment threads. However, the access control is only tied to the top-level comment. This works fine in theory, but the parent_id is not validated to be one of the descendants of the top-level comment specified by the id parameter. This means that a user could pass in NULL for the parent_id parameter to get back a list of all top-level comments, and then spider their way to any comment in the system so long as there was a comment they have access to.

3. Permitting viewing or editing someone else’s account via its unique identifier

This is a similar failure to what we’ve seen previously, but given it is account-related, it can be a more severe problem, particularly if payment, sensitive information, or shipment of goods is involved.

In addition to looking for the types of failures I’ve show previously, I also check that:

1. The current logged-in user’s session is stored securely - either server-side, or in a JWT with appropriate encryption.
2. Subdomain or domain-based routing has appropriate access checks for logged in users. All queries should be scoped through the domain object, be that an organization, a user, or similar.
3. Password reset systems are secure and it’s not possible to guess a password reset token for a different account.
4. Looking up the user’s ID in a UsersController#show action will not display sensitive information.
5. In a service based architecture, any service that contains account information will go back to a single secure “source of truth” for user identification, whether it is intended to be publicly exposed or not.

4. Accessing API with missing access controls for POST, PUT and DELETE

The most common source of missing access controls that I see is scaffolded code where one of the endpoints has not been touched. As all of us in Western society generally think from top to bottom and left to right, the endpoint where I most frequently see issues is on the DELETE operation. Here are some tips for reviewing that can help you more easily detect issues:

1. Review controller methods from bottom to top. This puts our eyes on the most likely problem points first.
2. Look out for the same missing access control points we’ve discussed previously, and focus on the “less important” actions, such as edit and delete - most programmers will spend most of their time focused on the index, show, and update actions.
3. Ensure that access controls on the edit, update and delete actions are at least as restrictive as those on the index and show actions.

Conclusions for Part 1

The common threads through all of the problems in this section is inconsistency in access control. When reviewing for the types of issues represented here, make sure to check that the access controls and query scoping is consistent across all introduced endpoints. If it’s different, access controls on endpoints that modify resources will generally be more restrictive than those for display. Also ensure that session handling is secure and correct to avoid problems down the road.

In Part 2, we’ll cover the remaining items on the list including CORS configuration, metadata security, and escalation of privilege. Stay tuned!

Find this useful? Be sure to check out these additional articles by Will Barrett:

• Tips for Migrating to a New Computer for Programmers
• The Top 5 Most Common Security Issues I Discover When Reviewing Code
• Reviewing One’s Own Code
• Handling Code Reviews with Empathy
• What We Can Learn About Code Review From the Apache Superset Project
• What We Can Learn About Code Review From the Apache Spark Project

More from PullRequest on catching security issues in pull request code review:

• The Top 5 Most Common Security Issues I Discover When Reviewing Code
• Top 3 Security Vulnerabilities I Find in iOS and Android Projects
• The Most Overlooked Server Permission Checks
• Pesky Server Auth Issues We’ve Found
• Learning security one code review at a time