This article was originally published in the March 2016 edition of the php[architect] magazine in the "Security Corner" column.
Most developers involuntarily cringe when they hear the words "legacy application" and with good reason. More often than not a legacy application usually comes with outdated and poorly architected code that's a mess to try to work with. I've seen cases where the "if it's not broken, don't fix it" mentality always wins out even if the code they're adding has to conform to other broken standards already in the codebase.
One of the key things that makes legacy applications tough to deal with is their complexity. Usually this is caused by a developer some time in the past (maybe even you!) who made some poor architecture decisions - or none at all - and pieced together a system that works but is extremely fragile. In these systems a single change could result in a chain reaction of failures, sometimes without the developer even knowing until some hapless user stumbles across it.
Legacy applications also come with their share of bugs. Sometimes these bugs are "features" and other times they're just annoyances that people just know to work around. These bugs can come in many shapes and sizes and can cover a wide range of topics inside the same application. However, in this series of columns I want to reflect on one certain kind of issue in legacy applications - security problems. While not all bugs in legacy applications are security issues, these can be some of the more serious. For example, if a user is able to bypass your login using a simple SQL injection because your queries use concatenation, that's a pretty major hole.
I want to take some time in the next few issues to give you some things to think about when refactoring those legacy applications and tips on how to make it more secure as you go. In this first part of the series I'm going to look at a few "quick hits" that you can more immediately detect in your current codebase. These are things that are pretty easy to fix too, making them relatively simple to implement.
As with any development advice, take these suggestions with a grain of salt. All codebases are different and there will be different challenges in each. Sometimes fixes that could be quick for one codebase could be a nightmare for another...it just depends on how the feature is used.
The superglobals are super-handy in PHP but they can also cause a lot of trouble if used incorrectly. Since they're global everywhere, there's all sorts of issues around decoupling and global state but I'm not going to focus on that stuff here. That's more for a good old fashioned software engineering discussion. Instead I want to help you poke around your code and find the places where these superglobals might be used and abused in a way that could compromise your application's security.
One of the most common places they're abused is in building SQL statements and using them completely unfiltered. If you're poking around your code and see this happening this is an immediate refactor point. While the data from the superglobals should always be considered tainted, there is a simple fix that can help protect from SQL injection issues: using prepared statements and bound parameters. In PHP both mysqli and PDO support this functionality and I highly suggest looking into it. Making the switch is usually only a few lines different from how you might be using the mysql_
functions right now but it's worth it in the long run.
The worst usual offender is the use of $_REQUEST
. So, why is using this variable a bad thing? By its nature it doesn't mark any
difference between which source the data is coming from. It combines the information from $_GET
, $_POST
and $_COOKIE
in to
one data set. The real tick here is that, if there's values with the same key name (say a GET
and POST
variable both named userId
)
one would overwrite the other. The order of this is governed by a php.ini
setting that is completely in the control of the server
you're using to run your code: variables_order
. Since this isn't something that you can rely on as always being consistent, you
can't trust the data that's in $_REQUEST
as it's presented.
Another major issue I've seen in older codebases is the hard-coding of credentials. It's common to look at classes that connect to external services or even your own databases and see properties for a username
and password
right there in the code. While it might have made sense at the time, there's a major issue here - if someone ever managed to get a copy of the source code, either from the site or version control, they'd have immediate access to those credentials.
To solve this issue, there's a few steps to take:
.env
file that's not checked in to your repository but lives somewhere else on the server. You can then use a library like phpdotenv (https://github.com/vlucas/phpdotenv) to pull those values in and use them directly. This prevents those credentials from falling into the wrong hands.These are just a few suggestions to help you get started in refactoring your application to make it more secure. The things I've discussed here are more specific to security concerns but refactoring for simplicity is a good idea no matter what part of the code you're working with. Legacy code, with all its complexity can be less secure than some of its more well-architected cousins but with a little work you can quickly bring it up to speed.
With over 12 years of experience in development and a focus on application security Chris is on a quest to bring his knowledge to the masses, making application security accessible to everyone. He also is an avodcate for security in the PHP community and provides application security training and consulting services.