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.
Injection vulnerabilities are a family of security vulnerabilities described by the Open Web Application Security Project to include:
In their description of this issue category, they state:
Source code review is the best method of detecting if applications are vulnerable to injections.
Before we can learn how to catch these issues in a code review, let’s get a better working definition.
Understanding Injection Vulnerabilities
Injection vulnerabilities exist when information provided by users of the application is not properly validated or sanitized before it is used. In the case of Cross-site Scripting, this would mean that a field allows the entry of JavaScript which is then re-displayed to the client. For SQL injection, this means a field that is written to the database is not properly validated and a SQL sanitization function is not run against the data before it is sent to the database as part of a command for execution. The last vulnerability they enumerate is “External Control of File Name or Path” this means that a field allows the user to insert a file name or a path that is used to write or read from the underlying operating system on the server. In each case, we have missing data validation. So, for our working definition let’s go with this:
Injection vulnerabilities are missing validations and sanitizations of user input that allow a user to gain access to more data than they should.
Catching Missing Validations and Sanitizations in Code Review
Missing validation and sanitization of inputs are not simple to find through automated testing, and they are not consistently found through click testing of applications before deployment. Most testers are not sufficiently trained in attack vectors to find these problems, and many are hidden away in APIs or may be obscured by client-side data validation. Once you know what to look for, though, these vulnerabilities jump off the screen in a code review.
Catching JavaScript Injection Vulnerabilities
JavaScript injection vulnerabilities, the relevant component of cross-site scripting, happen most frequently when a text input field is written to the database and then displayed later without any sanitization of the displayed information. So, what sanitization do we need here? Primarily we need to turn certain special characters into their entity-encoding before display.
So, if someone inputs this into a text box:
<javascript>alert('You are not secure')</javascript>
When we write it back out to the page we want to turn it into this:
<javascript>alert('You are not secure')</javascript>
The greater-than and less-than symbols have been turned into their HTML entity equivalents, so this will not be interpreted as HTML and the JavaScript function enclosed won’t be executed.
Different languages have different methods for escaping HTML. Reading up on these methods will help prepare you. In Ruby, I know to look for the following telltale signs:
- Using the
.html_safefunction in Rails views. - Not using
CGI::escapeHTMLoutside of view contexts with auto-escaping. - Using raw markdown compiled without protecting against JavaScript tag injection.
Depending on the language, you’ll find many different telltale signs of JavaScript injection vulnerabilities that can result in cross-site scripting. Become familiar with the HTML escaping system in the language you’re using day-to-day, and you’ll find your own list to look for.
Catching SQL Injection Vulnerabilities
SQL injection vulnerabilities occur when strings from the client are inserted directly into a SQL statement without first being sanitized. The most common way that this happens is string interpolation.
In Ruby, for example, the following code is insecure:
ActiveRecord::Base.connection.execute("SELECT * FROM users WHERE id='#{params[:id]}';")
This is because the ID parameter can be replaced with any string the user desires and it will be inserted directly into the SQL statement without any escaping. The secure version would leverage the ORM to perform the same query:
User.find(params[:id])
Using ORMs correctly will generally remove any risk of SQL injection, though the proper approach differs from ORM to ORM. In ActiveRecord, there are methods that take additional arguments that allow information to be safely inserted into SQL strings.
For example:
User.where('email = ? and deleted_at IS NULL', params[:email])
The question mark syntax indicates to the ORM where to insert the escaped version of the second parameter. Most other ORMs will provide similar functionality but it’s good practice to understand how much sanitization is taken care of at the ORM level and the ORM’s limits in handling sanitization.
🚩 The biggest red flag to look for in code review is string interpolation being used in SQL queries. This is generally unsafe.
Catching External Control of File Paths
External control of file paths is something that crops up when users are allowed to upload a file or initialize a resource server-side, such as a SQLite database. It can also happen on read if a system is not configured properly to only read from certain locations.
The danger in allowing the user to control the path is that they can attempt to write files to dangerous places, or gain access to the contents of source code or other files that contain privileged information.
Catching this type of security issue requires that the programmer or reviewer be aware of instances where a file path can be inserted into a parameter - this could be something straightforward like a file upload path or file read command, or somewhere slightly more unexpected such as a database connection string.
I look for instances where this information is passed in as a full path rather than as a sub-path of a containing folder or the more secure route of saving off the intended path to the database but writing the file to a random location on disk to avoid possible security issues.
Conclusion
Injection vulnerabilities result from insecure handling of user inputs. They are relatively simple to fix once the underlying issues that cause them are understood, and are frequently found by experienced reviewers who know what to look for. The prevalence of injection vulnerabilities today is one of the best arguments for continuing to perform code review in many organizations - this type of vulnerability is most frequently caught through human inspection of the offending code.
Find this useful? Be sure to check out these additional articles by Will Barrett:
- How Giant Data Leaks Happen - Understanding Cryptographic Failures (OWASP Number 2 for 2021)
- How to Catch the Top OWASP 2021 Security Category - Broken Access Control - in Code Review Part 1 and Part 2
- The Top 5 Most Common Security Issues I Discover When Reviewing Code
- Navigating Time Pressure in the Code Review Process
- What We Can Learn from the Ruby on Rails Project about Code Review
- 5 Ways I Provide Value as a PullRequest Reviewer When I Start Reviewing a New Project
- 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
