php[architect] November 2018

http://www.phparch.com \ November 2018 \ 29

Five Risks to Look for In a Code Review

Security Corner

2. Unescaped Output
   Similar to checking that all input is properly sanitized,
   developers should verify all dynamic content is properly
   escaped before it’s ever passed back out to userland.

Output escaping is a detail that applies more to applica-

tions serving content directly to users. If your application

is a component that passes data to a separate application

for manipulation before it’s presented, output escaping is

less important to your use case and might introduce unin-

tentional encoding artifacts. Be intentional with the data

contracts your API exposes and document any escaping

or content encoding before it introduces problems into the

workflow.

Escaping all dynamic content on output is an added check
in your application to prevent malicious injection from flow-
ing out to your end users. Assume an attacker has successfully
injected a JavaScript include into your database. Assume
further that the functions leveraging this data have failed to
properly sanitize their input before they use it in operation.
Your application’s next defense is to escape the content before
it’s printed to the screen.
Assume we have the same search function as before, but
neglected to sanitize the search term (see Listing 2).

Now assume the render() method above echos the search

term in HTML:

<p>You searched for: <?php echo $term; ?></p>

The malicious actor will now be able to embed the markup

of their choice on your page!

A safer solution is to always escape everything. Said another

way: no variable should ever be printed directly to the browser

without first being escaped for output. This escaping should

happen as late as possible in the workflow (i.e. just before the

content is output) to ensure later reviewers can easily track

what and when data is being cleaned before being passed to

the users.

<p>You searched for:

<?php echo htmlspecialchars($term, ENT_QUOTES, 'UTF-8'); ?>

</p>

The htmlspecialchars^3 function will automatically encode

all HTML entities in the passed variable, including double/

single quotes when the ENT_QUOTES flag is set. It will further

encode all content as UTF-8 based on the third parameter,

ensuring malformed Unicode won’t sneak in and break your

output.

In your code review, make sure all variable data is escaped

before it’s presented to the user—immediately before it’s

presented—to ensure no malicious content has snuck in.

3. (Un)intentional Obfuscation
   Developers like to show off. We like to demonstrate how
   well we can optimize our code, cut down on wasted space, or
   shave ms off a function’s execution time. Unfortunately, this
   habit can work against our best interests.

A developer I once worked with would consistently go

through every PR I submitted and delete trailing semi-co-

lons from all of my JS routines and CSS definitions. He

claimed the extra few bytes we were saving in transmis-

sion would make a difference. Until one of those missing

semi-colons led to an incorrectly-concatenated script that

broke our production system. His passion for premature

optimization and one-lining scripts led to a difficult-to-di-

agnose error and lost us a week of productivity.

Too-clever code can be difficult to follow. Nested ternaries

or inappropriately one-lined functions obfuscate business

logic, become hard to test, and make it easy for accidental

bugs to slip through the cracks. Worst still, allowing obfus-

cated code to live in our projects might hide maliciously

obfuscated code as well.

Everyone knows that debugging is twice as hard as writ-

ing a program in the first place. So if you’re as clever as

you can be when you write it, how will you ever debug it?

–Brian Kernighan

Once upon a time, WordPress included an Easter Egg^4 in

the post editor to catch when authors attempted to compare

a single post revision against itself. A self-comparison would

result in a JavaScript routine presenting a Matrix-esque

screen as a bit of an inside joke among the team. Unfortu-

nately, end users didn’t get the joke^5 and the team eventually

removed the Easter Egg entirely. Part of the reason it was

so controversial wasn’t the UI change, but the fact that the

3 htmlspecialchars: http://php.net/function.htmlspecialchars

4 WordPress included an Easter Egg: https://wp.me/pBMYe-1eC

5 end users didn’t get the joke:

https://phpa.me/lists-automattic-2010-oct

Listing 2

1. function search_content($term) {


2. // Get search results from the internal system


3. $results = $this->query_elasticsearch($term);


4. 
   


5. if (count($results) == 0) {


6. $results = $this->query_database_content($term);


7. }


8. 
   


9. // Render a search result template, with the sanitized


10. // search term and the results from the database.


11. $this->render($term, $results);


12. }