Magento 1.5.0.1 Security Fix

During last week’s Imagine conference, Magento released (among other things) the 1.5.0.0 version of their Community Edition offering. About a day after its release a bug that compromised a security layer was found. Twelve hours later Magento released 1.5.0.1 which removed the new database file storage feature, which effectively removed the bug from the system.

What Happened

If we search through the source (ack, grep) for get.php, you’ll find one relevant result

app/code/core/Mage/Core/Model/Store.php
69:    const MEDIA_REWRITE_SCRIPT          = 'get.php/';

Then, if we search the codebase for the constant defined above, we can see it’s used only once

#File:     app/code/core/Mage/Core/Model/Store.php
/**
 * Gets URL for media catalog.
 * If we use Database file storage and server doesn't support rewrites (.htaccess in media folder)
 * we have to put name of fetching media script exactly into URL
 *
 * @param null|boolean $secure
 * @param string $type
 * @return string
 */
protected function _updateMediaPathUseRewrites($secure=null, $type = self::URL_TYPE_MEDIA)
{
    $secure = is_null($secure) ? $this->isCurrentlySecure() : (bool)$secure;
    $secureStringFlag = $secure ? 'secure' : 'unsecure';
    $url = $this->getConfig('web/' . $secureStringFlag .  '/base_' . $type . '_url');
    if (!$this->getConfig(self::XML_PATH_USE_REWRITES)
        && Mage::helper('core/file_storage_database')->checkDbUsage()) {

        $urlStart = $this->getConfig('web/' . $secureStringFlag .  '/base_url');
        $url = str_replace($urlStart, $urlStart . self::MEDIA_REWRITE_SCRIPT, $url);
    }
    return $url;
}

Based on what I’ve seen here, it looks like 1.5 introduces a feature that allows core media files to be stored and served from the database. With the Database File Storage system in place, Magento would redirect requests for media files to get.php. This was done by the code above, or via the .htaccess file in the media folder.

#File: media/.htaccess
Options All -Indexes
<IfModule mod_php5.c>
php_flag engine 0
</IfModule>

AddHandler cgi-script .php .pl .py .jsp .asp .htm .shtml .sh .cgi
Options -ExecCGI
<IfModule mod_rewrite.c>

############################################
## enable rewrites

    Options +FollowSymLinks
    RewriteEngine on

    #RewriteCond %{REQUEST_URI} !^/(media|skin|js)/

############################################
## never rewrite for existing files
    RewriteCond %{REQUEST_FILENAME} !-f

############################################
## rewrite everything else to index.php

    RewriteRule .* ../get.php [L]
</IfModule>

Once redirected, get.php looks for the media file specified in the database, and if it finds it, its contents are written out to the file system. Finally, the code then serves out the file that was just created.

The whys of the new (and since deleted) Database File Storage system elude me. I can think of a few scenarios, mostly centered around Magento trying to reduce the time spent dealing with permissions/deployment related support tickets. Without knowing the exact problem Magento’s trying to solve here, it’s hard to say if this is a good idea or not. The real problem though is the feature’s implementation.

The Implementation Problem

The public facing web is one of the most hostile programming environments you’ll find online. Two things most veterans know is

1. Never trust user input to contain what you think it will. URLs are user input

2. Be extremely paranoid when reading and writing to the local file system

The get.php file-server failed at both of these. Regarding trusting user input, PHP best practices say we should use the user input to lookup a list of legitimate values. A simple example of this

$legit_values('one.txt','two.txt','three.txt');
$filename = $_GET['file'];
if(in_array($filename,$legit_values))
{
    file_get_contents($filename);
}
else
{
    throw new Exception("Nice try buddy, go back to h4x0r school");
}

What’s ironic is get.php already had a list of valid files stored in the database, as that is, (or seems to be), the whole point of this feature. Adding an Exception in an else branch would have taken care of the problem.

if ($databaseFileSotrage->getId()) {
    ...
}
else
{
    throw new Exception("No file found in database, you're not that clever buddy");
    //as a side note, it's probably best not to taunt a young h4x0r for failing 
}

If I’d been code reviewing this, I also would have called out the saving/serving of the file. While this works, it’s the sort of thing that could easily fail under load. An extra file system write operation on each page load is the sort of thing that adds up sooner rather than later. A better approach here would have been to have PHP directly output the contents read from the database. That’s ultimately what Varien_File_Transfer_Adapter_Http ends up doing after it re-reads the file from the file system anyways.

Layered Security

So, with get.php in place, someone who knew about this problem could view any file in a Magento installation by constructing a URL like this

http://magento.example.com/get.php/app/etc/local.xml

The above URL would expose, among other things, the MySQL username and password of a Magento system, as well as anything else in local.xml.

I don’t want to downplay the seriousness of this sort of thing. Bugs like his have no place in production releases of software. However, if the above exploit was all someone needed to get at your database, then you’re doing security wrong. If you’ve ever heard system administrators crow about putting your databases behind firewalls, this is the reason why.

The only servers that need to talk to the database are the front-end web servers, which is why your firewall should be configured to refuse all outside connections to your database server. That way, even if your database server password falls into nefarious hands, the nefarious hands are blocked by your firewall.

Other scary potential exploits include things like

http://magento.example.com/get.php/../../../../../../../etc/passwd

Again though, if Apache (or whatever front-end web server you’re using) has read access to files outside the site root, you’re still doing security wrong. If you’ve ever had a system administrator admonish you for running Apache as root, this is the reason why. If you’re interested in security your web server should only have access to the files it needs to have access to. As big a pain as *nix permissions are, this is why they exist.

Long story short, if you’re taking security seriously and you have something that’s a single point of failure, you’re asking for trouble.

Why’d This Happen

These sorts of problems are pretty well known to most veteran web app developers, so how’d this slip into a Magento release? My guess is it’s because the feature was implemented by a non-veteran developer. I’d wager that most of us able to identify problems like this know about them because we wrote a bunch of crappy, un-secure code and either suffered the consequence or had a colleague point it out to us.

This industry, as a rule, lacks any sort of mentoring for programmers entering the workforce. A smart, capable developer who never encountered PHP might assume that PHP is smart enough to keep something so obviously wrong from happening, and until they screw up and until the screw-up is noticed by someone and until the screw-up is brought back to their attention, they’ll never learn.

Should this industry work this way? No. Does it? Yes.

Does it not work this way at your company? Get in touch, because I love working with people who get this right!

What DID Work

Magento Inc., considered as a single entity, shouldn’t let something like this slip through the cracks. As they continue to grow and continue to move into developing larger, more ambitious systems, they’ll need to address problems like this or the market will do what the market does to companies that don’t adjust.

Magento isn’t alone in this, it’s something that happens to every software company or community that grows at the rate Magento’s been growing. Every company makes mistakes, it’s how they respond to the mistakes that matters.

With that in mind, consider the following

1. The bug was quickly identified by the Magento community

2. Once identified, the core team was able to react quickly have a fix (i.e. removing the the feature) in place within 12 hours

3. Anyone quick enough to deploy 1.5.0.0 to production a day after release are also going to be the people who’ll deploy 1.5.0.1 to production the day of its release

So, despite the seriousness of the bug, the entire ecosystem ensured its effects were minimized, and no one was compromised. This is what open source advocates mean when they say open source produces more secure software. Because everyone can look at it, bugs can’t hide inside proprietary binaries.

Community Oversight

The tension between needing to keep security bugs private to reduce the chances of exploits vs. the need to make them public to keep informed people safe and force a fix is an old game that I’m not interested in playing. Magento recently re-publicized their security reporting process. If you find an exploit that’s a good place to start.

There is one thing tangent to that old argument that is worth exploring. Take a look at the Magento release archive.

The first 1.5 alpha was released on December 17th, and contained the get.php file. That means the security bug existed for almost two months in pre-release versions before being spotted the day after the source’s official release.

If we’re going to rake Magento Inc. over the coals for this bug, we’ll have to save a few hot cinders for ourselves. Those alphas, betas and release candidates get put out for a reason, and if we’d been watching them more carefully this problem would have been spotted way before it was actually a problem.

To that end, I’ve setup a quick and dirty webpage (with an RSS feed) that contains a summary of the daily diffs for Magento’s public 1.5 Trunk subversion repository. For example, here’s the diff where Magento pulled the feature and fixed the bug.

If you’re a developer using Magento and have strong opinions about security or architecture, I’d encourage you to subscribe and keep an eye on things. If you’re a developer with knowledge of current continuos integration methodologies and have have ideas for how the community can do this properly, get in touch.

Security bugs suck, but security bugs happen to all young platforms. As Magento Inc. enters it’s next phase of life as a funded startup, those of us that have been riding this train for a while get to have a say in how Magento, the platform, grows. We can spend it bitching about Magento Inc.’s priorities, or we can spend it solving our problems, our clients problem’s, and reaping the rewards. I hope it’s clear which side of that divide I stand on.