28 \ November 2018 \ http://www.phparch.comSecurity Corner
Five Risks to Look for In a Code Review
Eric Mann
Development teams use code review as a way to keep track of one another’s progress on issues
and tasks in the work queue. Code reviews are also a stellar way to proactively detect and address
security concerns before they become critical to the success of the project.In September, we talked about
cultivating a mindset of “professional
paranoia.” The goal was to help you
think like an attacker so you can code
proactively to defend against common-
place attacks and vulnerabilities. A key
parallel to this is understanding how to
review code—both your own and that
of colleagues—from a security perspec-
tive to catch potential issues before
they’re found by the outside world.
This month, we’re going to outline
five key items developers should look
for in any security-minded code review.
This list is not exhaustive but will help
keep common areas of concern top-of-
mind while you’re auditing your team’s
code.
- Inappropriately-Trusted
Input
One of the first things hard-core
security experts will talk about is the
concept of sources versus sinks. A
source is anything that can provide
data or input to your function, code,
or application. A sink is anything your
function, code, or application provides
data to. Said another way, a source is
something that provides data, a sink is
something that takes data.
When building web applications,
the most visible source of data is the
end user. A human enters text into a
submission form or clicks a button
to select an option. These interaction
points provide data to our application—
data entirely under the control of the
us er.
It’s easy to think our end users are
all like us. We build an application to
do a specific thing or accept a specific
input; we assume our users will only
ever use the application to do that thing
or will only ever provide that kind ofinput. This is a fatal assumption. Users
can make mistakes. They can provide
erroneous information—submitting
the word “three” instead of the number
3 , for example.
Users might also be malicious; they
might provide bad data on purpose
to make our application error, fail, or
behave in unexpected ways. Never trust
the user.
Unless the source of the data being
used by a function is another trusted
component of your application, always
assume it’s a potential vector for mali-
cious use. A hacker might attempt to
inject an SQL statement into a search
field. Another might embed a JavaS-
cript include into a DNS TXT record
to inject a script into your page, see
DNS and Cross-Site Scription^1. A data-
base query response might include
malformed Unicode characters.
Unless proven otherwise, always
assume the source of data flowing into
your application (or function or script
or lambda routine) could be poten-
tially malicious. All inputs should be1 DNS and Cross-Site Scription:
https://phpa.me/dns-xsssanitized as soon as possible before
they’re used; for example, immediately
in a function after the data is passed in
as in Listing 1.
The above function takes a simple
search parameter and queries an inter-
nal API for content that potentially
matches the user-supplied search term.
This could be passing the data into a SQL
statement. It could be POSTing a search
document to ElasticSearch. Ultimately,
the point is that the function is explic-
itly sanitizing^2 the incoming/untrusted
source of data before attempting to use
it. PHP provides its own filter functions,
most frameworks will bundle a valida-
tion library, and many of these libraries
can be used as standalone components
in your own application.
When conducting a code review,
ensure all data coming from untrusted
sources is sanitized before being used by
application logic. Take the time to make
sure the codebase is using standard,
well-documented means of sanitization
—like PHP builtins— rather than trying
to reinvent the wheel.2 explicitly sanitizing:
http://php.net/filter.filters.sanitizeListing 1- function search_content($term) {
- // Immediately sanitize the search term
- $sanitized = filter_var($term, FILTER_SANITIZE_STRING);
- // Get search results from the internal system
- $results = $this->query_elasticsearch($sanitized);
- if (count($results) == 0) {
- $results = $this->query_database_content($sanitized);
- }
- // Render a search result template, with the sanitized
- // search term and the results from the database.
- $this->render($sanitized, $results);
- }