php[architect] November 2018

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

Five Risks to Look for In a Code Review

Security Corner

code itself was heavily obfuscated^6.
Anyone reading through WordPress’
core code would find a script with
eval()s and Dvorak-Querty obfus-
cation logic that looked like a buried,
malicious payload.
Including this feature was a fun find
for those of us who knew what was
going on. It absolutely terrified those
who didn’t. It also made it harder for
anyone to audit the codebase of active
sites to find actual maliciously-evaled
payloads.
In short: don’t be too clever when
writing your code. Simple, boring code
is safe, easily tested, and less likely to
give other developers panic attacks
when they read it during a review.

Edge Cases

Engineers who work with lower-level languages like C are quite familiar with the concept of fuzzing^7. This is the prac- tice of testing code or functionality by passing in data that is just out of the bounds of what's usually expected. Does a function expect an integer input between 0 and 10? Great. What happens if a negative number is passed instead? Or an integer greater than 10? Or a floating point number that’s still in range? It’s easy to ignore these edge cases as they fall outside the expected behav- ior of our application. However, if we expose these functions to end users (or if end users can directly manipulate the data being passed into these functions), those users could potentially trigger erroneous functionality within our application. Edge cases make for incredible inte- gration tests for an application. Take the time to write both passing tests—those that verify the application’s expected functionality—and failing tests. What happens when incorrect data is passed in? These failing tests help to verify error conditions (edge cases) will trigger

6 heavily obfuscated:
https://phpa.me/greeb-jsphp-easter-egg

7 fuzzing:
https://en.wikipedia.org/wiki/Fuzzing
8 strict mode:
http://php.net/migration70.new-features

expected, permissible error states. Does the application expect content IDs to only be positive integers? Rather than using abs(int($id)) to cast incom- ing data, explicitly test that $id is an integer—and positive—before proceed- ing. If a negative or non-integer value are passed in, throw an exception that can be caught and properly handled elsewhere in the stack. When in doubt, use type hints. PHP 7 supports typehints out of the box, and you can enforce them with strict mode^8. Your code review should include close checking of input and return types to ensure data is created and used as expected.

Documentation
Many developers defend their undoc-
umented code as self-documenting.
They assert that the logic and intention
of their code should be obvious by
merely reading the code itself. While
they’re partially right—the code should
be readable and easy to follow—proper
documentation ensures the maintain-
ability and understandability of code
into the future.
If your code is not documented and if
you fail to explicitly call out what your
code is doing and why—it will be difficult
for future auditors to ensure that your
code is still operating as expected. Code
documentation is an artifact. It’s a sign-
post to help future developers (perhaps
yourself, perhaps an outside auditor)
understand the intention behind the
logic fleshed out by your code.

The what of your code should be obvious; probably self-documenting. The why needs to be explicitly called out through documentation. More often than not, the why behind your code approach will help identify the edge cases we discussed earlier and encourage the team to flesh out its testing infrastructure. Well-documented, well-tested, well-reviewed code is often the most stable, secure, and maintain- able in a project.

In Conclusion There is no silver bullet when it comes to software development, code review, or security. Everything we do in this field is an iterative process with each step an improvement upon the last. As said before, these are not the only five things you should do during a security-minded code review. They are, however, five things every code review should look at. Are the sources of and sinks for data properly protected? Is any code overly clever or obfuscated in such a way that its function is non-obvious? Are there edge cases that can and should be tested for and protected? Did the development team thoroughly document their work? These checks alone will save your team time, effort, and frustration if and when a security issue arises. Take the time now to ensure your internal code reviews address each of them and don’t stop there!

Eric is a seasoned web developer experienced with multiple languages and platforms. He’s been working with PHP for more than a decade and focuses his time on helping developers get started and learn new skills with their tech of choice. Eric works as a Tekton for Tozny, a privacy and security-focused startup in the Portland area. @EricMann

php[architect] November 2018

Get our desktop app

Company

Features

Documentation

Resources