News for hackers. Stuff that matters.
Re:I think tumblr has a huge security hole by tjogin on Saturday March 19, 2011 @08:02AM (#2343386)
I think PHP has so many gotchas, so many ways to trip up even seasoned developers, that it's inevitable to miss something.
Re:I think tumblr has a huge security hole by VMG on Saturday March 19, 2011 @08:16AM (#2343400)
Putting passwords in the php file? That's not a seasoned developer in my mind.
Re:I think tumblr has a huge security hole by russss on Saturday March 19, 2011 @09:28AM (#2343525)
I'm going to disagree with you. The problem with PHP is that there's no easy way of sharing state between requests.

If you have a separate (ini-style) configuration file, every time you get a new request, the file will have to be read in from disk and parsed. On heavily-loaded web servers this can be a significant performance issue.

Configuration stored in a PHP file will be cached by your opcode cache and so doesn't incur any per-request parsing/reading overhead.

The problem here is not that Tumblr stored their configuration in PHP. The problem is their lack of testing their changes.

(As I mentioned elsewhere in the thread, this particular nasty issue can be solved by simply enforcing that every .php file begins in '<?'.)

Re:I think tumblr has a huge security hole by jasonlotito on Saturday March 19, 2011 @10:42AM (#2343690)
> If you have a separate (ini-style) configuration file, every time you get a new request...

it will just pull it from the cache, not needing to be parsed or read from disk.

Configuration stored in a PHP file is bad. Changing working code simply because you want to add a new slave is a horrible idea. It would be like having to recompile Apache from source every time you want to add a new vhost because all the entries are written in C.

Interpreted languages like PHP, Python, Perl, etc all make it easy to put config options directly into the language. When your app is small, it's fine. But as it gets larger, move it out.

But yes, that they didn't even catch this is in testing is the real problem.

Re:I think tumblr has a huge security hole by russss on Saturday March 19, 2011 @01:16PM (#2344103)
> it will just pull it from the cache, not needing to be parsed or read from disk.

I don't believe PHP's ini_read caches anything. Sure, your OS cache is going to have that file in, but PHP still has to do the fopen(), fread(), parse, fclose() dance on every page hit. This does not happen with an opcode-cached source file.

I have actually benchmarked this. Amortized across billions of page hits per month it produces a notable saving.

Kudos points out that you could use APC - this is absolutely true but IMO it basically equates to doing the same thing in a more complex way.

> Configuration stored in a PHP file is bad. Changing working code simply because you want to add a new slave is a horrible idea. It would be like having to recompile Apache from source every time you want to add a new vhost because all the entries are written in C.

This is a false comparison between interpreted and compiled languages. Not to mention that plenty of C programs use #defines for configuration. Varnish even translates its configuration language into C and dynamically loads it as a module, which provides significant flexibility.

In a dynamic language, and where you trust the person doing the configuration, there's no reason why your configuration shouldn't be in a source file. Both Django and Rails do it this way.

Re:I think tumblr has a huge security hole by carbon8 on Saturday March 19, 2011 @03:17PM (#2344428)
"In a dynamic language, and where you trust the person doing the configuration, there's no reason why your configuration shouldn't be in a source file. Both Django and Rails do it this way."

Not true. In Rails, sensitive information like database passwords or AWS keys are not stored in the source, you store them in configuration files, YAML files by default. It's also widely recommended that these not be checked into version control.

Re:I think tumblr has a huge security hole by russss on Saturday March 19, 2011 @03:43PM (#2344491)
My bad - it's been a while since I touched Rails and my memory was rusty.
Re:I think tumblr has a huge security hole by jasonlotito on Saturday March 19, 2011 @04:46PM (#2344675)
> I don't believe PHP's ini_read caches anything.

I wasn't referring to straight PHP. There are multiple ways to cache the config file's contents so you don't have to hit the disk to read.

> This is a false comparison between interpreted and compiled languages. Not to mention that plenty of C programs use #defines for configuration.

Your misunderstanding me here. I'm not saying #defines are bad. However, they are a different level of configuration from vhosts. You aren't #defining your virtual hosts. Same with php. You have the C code with specific options being set, and then php.ini for the user facing stuff.

> In a dynamic language, and where you trust the person doing the configuration, there's no reason why your configuration shouldn't be in a source file.

Except we saw at least one reason today.

Re:I think tumblr has a huge security hole by user24 on Saturday March 19, 2011 @02:41PM (#2344342)
> I don't believe PHP's ini_read caches anything.

so check if the ini file key is in memcache, if so get it from there, if not, ini_read it and populate the cache. Not hard.

Re:I think tumblr has a huge security hole by Kudos on Saturday March 19, 2011 @09:47AM (#2343558)
Sure there is, APC. But that aside, Linux won't read that file every time, it will end up living in the disk buffers that it maintains.
Re:I think tumblr has a huge security hole by moe on Saturday March 19, 2011 @10:42AM (#2343693)
The disk buffers make the operation cheap but not free, i.e. keeping the information cached is the right thing to do.
Re:I think tumblr has a huge security hole by VMG on Saturday March 19, 2011 @09:38PM (#2345252)
If that is a big problem, pass secrets as environment variables. These are getting passed and read anyway.
Re:I think tumblr has a huge security hole by tjogin on Saturday March 19, 2011 @08:49AM (#2343460)
A huge mistake, but Tumblr's staff are probably quite experienced, nonetheless.

How about that gotcha though? A single character is mistakenly changed from "<" to "i" and that exposes the source code to the browser. Think about that.

Re:I think tumblr has a huge security hole by hasenj on Saturday March 19, 2011 @09:57AM (#2343584)
I wonder if the 'i' has anything to do with vim
Re:I think tumblr has a huge security hole by moe on Saturday March 19, 2011 @10:37AM (#2343675)
Clearly they should ban vim from their development process! ;-)

No, joking aside, the normal way to handle this is to have the perimeter server (nginx or varnish) catch any 5* responses and turn them into a user-friendly error-page. That way you never expose sensitive stack traces to your users.

So, this is standard stuff and easy to fix. However who of us hasn't screwed up on a similarly trivial issue before? I wouldn't judge them too hard on this one, happens to the best of us.

Re:I think tumblr has a huge security hole by weinzierl on Saturday March 19, 2011 @12:49PM (#2344031)
Clearly they should make vim mandatory on every machine;-) I guess the 'i' comes from someone who is used to use vim but in this case using another editor.
Re:I think tumblr has a huge security hole by tjogin on Saturday March 19, 2011 @10:46AM (#2343700)
That wouldn't have caught this though, this code wasn't even executed, just sent as plain text.
Re:I think tumblr has a huge security hole by moe on Saturday March 19, 2011 @11:01AM (#2343741)
Nginx/varnish can also catch Content-type: text/plain (or text/php?) and treat that like a 5xx response. Yes, that's a bit of a hack, but unless tumblr needs to serve text-files (which I doubt) it's a legit safeguard.
Re:I think tumblr has a huge security hole by nikcub on Saturday March 19, 2011 @11:38AM (#2343826)
I know you were kidding, removing all text editors from your production servers might be a good idea
Re:I think tumblr has a huge security hole by bdlang on Saturday March 19, 2011 @11:57AM (#2343882)
This is my guess. Anyone who's used VIM for years knows that the first couple of times using a different editor, your text is littered with "i" and ":w".
Re:I think tumblr has a huge security hole by hasenj on Sunday March 20, 2011 @01:12AM (#2345608)
It could simply come from forgetting to exit from insert mode, and then coming back thinking the editor is in normal mode.
Re:I think tumblr has a huge security hole by vidar on Saturday March 19, 2011 @11:35AM (#2343817)
That was my first thought as well, as a long time vim user I kind of cringed.
Re:I think tumblr has a huge security hole by powdahound on Saturday March 19, 2011 @10:58AM (#2343732)
Well, a single character change + a developer not testing their code locally + not running high level unit tests + deploying untested code to production.
Re:I think tumblr has a huge security hole by datasink on Saturday March 19, 2011 @12:12PM (#2343914)
Tumblr has at least one full time sysadmin. I think it's more reasonable to suppose he made the change to a production config file, rather than a developer. A definite failing in their deployment process, but it really says nothing about the quality of the application code or developer testing.
Re:I think tumblr has a huge security hole by Joakal on Saturday March 19, 2011 @08:22AM (#2343413)
It's needed to connect to databases with authentication. What would you suggest?
Re:I think tumblr has a huge security hole by eam on Saturday March 19, 2011 @08:30AM (#2343427)
I believe the way to do it "correctly" would be to put the actual file containing the credentials in a non-public location and just do an include where you need to access it. At least that's how I do it. I could be wrong...
Re:I think tumblr has a huge security hole by drdaeman on Saturday March 19, 2011 @08:48AM (#2343459)
If one would made a similar typo in that file, its contents will be displayed too.

A solution would be to have a .ini-like (or some other simple-to-parse format) config file and PHP code to read its contents. PHP code could be leaked, but config file contents wouldn't.

Re:I think tumblr has a huge security hole by robryan on Saturday March 19, 2011 @09:15AM (#2343513)
Yeah seen a few client libraries that are doing that. Even putting them in a config only file lessens the chance of something like this happening as it is modified a lot less often.
[deleted]
Re:I think tumblr has a huge security hole by drdaeman on Saturday March 19, 2011 @09:12AM (#2343507)
I believe you are wrong. include()/require() would equally happily display any file's contents outside of `<?php ... ?>` scope (the case with "i?php"), within document root or not.

Edit: I've tested this:

    $ php -v
    PHP 5.2.6-3ubuntu4.6 with Suhosin-Patch 0.9.6.2 (cli) (built: Sep 16 2010 19:51:25) 
    Copyright (c) 1997-2008 The PHP Group
    Zend Engine v2.2.0, Copyright (c) 1998-2008 Zend Technologies

    $ cat test.php
    <?php
        require "/tmp/test2.php";
    ?>
    $ cat /tmp/test2.php
    i?php
        define("TEST", "test");
    ?>
    $ GET http://localhost/test.php
    i?php
        define("TEST", "test");
    ?>
Re:I think tumblr has a huge security hole by CWIZO on Saturday March 19, 2011 @10:07AM (#2343605)
Off course, but in this case, passwords would only be exposed if the config file had a miss-typed opening PHP tag. If "test.php" had it, you wouldn't be able to see the contents of "test2.php".
Re:I think tumblr has a huge security hole by drdaeman on Saturday March 19, 2011 @10:09AM (#2343606)
Yes, you are right. And in this exact case they (mis-)edited the file, that contained passwords (i.e. test2.php in my improvised example).
Re:I think tumblr has a huge security hole by CWIZO on Saturday March 19, 2011 @10:14AM (#2343618)
Sure, I was replying for this hypothetical situation that you guys ware discussing, where they would store passwords in a different file outside of webroot ...
Re:I think tumblr has a huge security hole by sewerhorse on Saturday March 19, 2011 @09:11AM (#2343503)
it's always included to the document root (the bootstrap file). it doesn't matter where you're including from, a broken open tag would cause errors like this one.
Re:I think tumblr has a huge security hole by windsurfer on Saturday March 19, 2011 @08:40AM (#2343443)
If you're doing an include, wouldn't the passwords still be in a PHP file?
Re:I think tumblr has a huge security hole by lsc on Saturday March 19, 2011 @08:44AM (#2343453)
if the php file is printed rather than executed, the include will not be followed. You'd see the "include /path/to/inaccessible/file" but you wouldn't see the passwords within the include.

I think. I haven't seriously used php since 2003 or so.

Re:I think tumblr has a huge security hole by russss on Saturday March 19, 2011 @09:06AM (#2343487)
Yes, but included files still need a <?php tag at the top, or PHP just prints them out.
Re:I think tumblr has a huge security hole by nbpoole on Saturday March 19, 2011 @08:47AM (#2343455)
That wouldn't help in this case, since a typo in the config file is making the file not be parsed as PHP.

If you want to avoid that possibility, you can use a non-PHP format (eg: YAML) and parse it.

Re:I think tumblr has a huge security hole by drdaeman on Saturday March 19, 2011 @08:39AM (#2343440)
Client SSL certificates? I don't know what DB they're using, but at least Postgres and MySQL support certificate-based auth.
Re:I think tumblr has a huge security hole by pilif on Saturday March 19, 2011 @08:50AM (#2343461)
How is a client certificate different from a long random password? Both have to be stored somewhere. The correct solution is to keep this (and everything else) out of your web root.

If that i?php blunder happened to me, my users would see 10 lines of code. One include for the framework (which lives outside the web root), plus three calls to get the framework to handle the current request.

Those 10 lines would be totally unproblematic

Re:I think tumblr has a huge security hole by drdaeman on Saturday March 19, 2011 @08:52AM (#2343463)
.pem file won't be leaked in this ("i?php") manner. Only file's location would be revealed.

Of course, you are completely right, it should still be inaccessible from the web.

Re:I think tumblr has a huge security hole by petervandijck on Saturday March 19, 2011 @08:35AM (#2343435)
Typically, these files are not web accessible. Problem solved.
Re:I think tumblr has a huge security hole by russss on Saturday March 19, 2011 @09:20AM (#2343518)
This bug was nothing to do with the file being web accessible. If you miss the opening <?php tag out in any file, regardless of whether it's in the web root, it will get printed straight out to the browser.
Re:I think tumblr has a huge security hole by jonhohle on Saturday March 19, 2011 @10:20AM (#2343638)
But you would never `include()` a PEM file.
Re:I think tumblr has a huge security hole by themonk on Sunday March 20, 2011 @02:24PM (#2346898)
No, better words would be: "Putting passwords in php file under doc root, why not keep it outside doc root?"
Re:I think tumblr has a huge security hole by zaidf on Saturday March 19, 2011 @08:22AM (#2343410)
Good point. Minus the keys/passwords the "harm" would be limited.
Re:I think tumblr has a huge security hole by Tomek_ on Saturday March 19, 2011 @09:56AM (#2343578)
Please name at least 5 of them.
Re:I think tumblr has a huge security hole by dexen on Saturday March 19, 2011 @01:03PM (#2344073)
Tsk, tsk. It's trendy to blame PHP for every problem out there.

Also, it's trendy to drop unseasoned developers onto it and expect them to build complex applications that are publicly accessible yet 100% foolproof. Go figure.

If you asked me, I couldn't even tell. I've encoundered some in the past, sure, but found (in docs) both rationale and the correct way to use stuff.

There was that reference-vs-value matter when passing objects around, in PHP v4.x, but that's fixed by v5.

Re:I think tumblr has a huge security hole by zaidf on Saturday March 19, 2011 @08:04AM (#2343388)
I understand that view.

But I am talking specifically about something that would just dump the code of the php file to the browser.

Re:I think tumblr has a huge security hole by Maxious on Saturday March 19, 2011 @08:25AM (#2343418)
I think this was more of a systemic failure - the file that failed in this case was also relied upon to disable development mode/error output on production servers. I'm pretty sure my production web servers return a HTTP code 500 with no further output when they encounter an error with the php.ini-production configuration file provided in the PHP source distribution.

The hitch was of course that the file in question had a "i?php" (someone was using vi and hit i too many times?) instead of "<?php" which lead the php interpreter to output that section of code as if it were HTML. That feature is something PHP could provide an option to disable and only allow explicit echo/print/printf which should be what templating engines use.

Re:I think tumblr has a huge security hole by KevBurnsJr on Saturday March 19, 2011 @10:56AM (#2343727)
Actually, PHP returns a 200 when it encounters a fatal error.

This explains why it was indexed by Google.

Re:I think tumblr has a huge security hole by yrashk on Saturday March 19, 2011 @08:06AM (#2343391)
they just had a single typo i?php instead of <?php I guess somebody's using vim
Re:I think tumblr has a huge security hole by russss on Saturday March 19, 2011 @09:13AM (#2343509)
At Last.fm we had a Subversion pre-commit hook which ensured that all .php files began with <?.

That would have easily caught this problem, no drama.

Re:I think tumblr has a huge security hole by SoftwareMaven on Saturday March 19, 2011 @05:54PM (#2344848)
Unless the file was checked in separately. If they were doing something like updating the db password, so they need to update "current running" and "up-and-coming" code. As a result, they may have made the change in production, with the typo, and the change in SVN, without the typo.

I would put my money on something like this being the cause.

Re:I think tumblr has a huge security hole by KevBurnsJr on Saturday March 19, 2011 @09:21AM (#2343519)
+1 to russ. PHP syntax checking on precommit would have caught this.

Edit: I'm wrong. It does not catch this.

Re:I think tumblr has a huge security hole by zaidf on Saturday March 19, 2011 @10:12AM (#2343615)
Not necessarily. Makes sense for syntax check to only check between php tags.

Root of the issue is that by default php outputs anything not between php tags to the browser.

Perhaps some sort of buffer control at the top most include can disable output to browser until a later time. That's pretty much how templating systems work except they don't disable output afaik thus leaving possibility of this open.

Re:I think tumblr has a huge security hole by bluesnowmonkey on Saturday March 19, 2011 @12:23PM (#2343949)
Exactly right. There's no reason for a config file (or any of the setup code) to be writing output. A good way to force this is by immediately starting an output buffer with a callback that returns nothing, then calling ob_end_clean() after setup is complete.
Re:I think tumblr has a huge security hole by dexen on Saturday March 19, 2011 @10:19AM (#2343636)
You could do ob_start(); include '...'; ob_end_clean();, but it turns out that die() (say, in a custom error handler) () causes an implicit buffer flush. So it's not a foolproof solution.
Re:I think tumblr has a huge security hole by bluesnowmonkey on Saturday March 19, 2011 @12:28PM (#2343965)
You can attach a callback to the buffer to prevent that implicit flush.

    ob_start(function() {});
Re:I think tumblr has a huge security hole by dexen on Saturday March 19, 2011 @12:52PM (#2344044)
Thanks, I'll investingate and probably adopt it for my code :D
Re:I think tumblr has a huge security hole by KevBurnsJr on Sunday March 20, 2011 @04:56AM (#2345877)
Or register_shutdown_function - http://blog.kevburnsjr.com/php-fatal-error-500
Re:I think tumblr has a huge security hole by dexen on Saturday March 19, 2011 @10:22AM (#2343643)
No chance. The problem was a missing <?php tag -- PHP syntax cheching would NOT have raised alarm, because everything before a <?php tag is not considered PHP.

Forcing every file to start with <?php is just PITA for developers working on templates.

Re:I think tumblr has a huge security hole by russss on Saturday March 19, 2011 @01:20PM (#2344115)
> Forcing every file to start with <?php is just PITA for developers working on templates.

Yep, our templates were .tpl files, which is probably a good convention to have even if you use raw PHP as your templating language.

But you get the drift. This is something you have to deal with when you use PHP.

Re:I think tumblr has a huge security hole by bluesnowmonkey on Saturday March 19, 2011 @12:13PM (#2343919)
If the templates are raw PHP, yeah. Constructing HTML in code like that is a PITA and a recipe for errors and XSS vulnerabilities. It's common to use templating engines like Smarty or Mustache. In that case, you can standardize on PHP files starting with an opening tag and having no closing tag. Then syntax checking is a piece of cake.
Re:I think tumblr has a huge security hole by dexen on Saturday March 19, 2011 @12:50PM (#2344034)
After using eZ Publish with its templating language (also implemented in PHP) for about a year, I've found `raw' PHP just more expressive and concise -- and my team, coming from various backgrounds, has that preferrence as well.

Whaddya know, PHP is a templating language, after all.

Also, it takes us less effort to ship product that matches performance requirements with `raw' PHP than with another layer of abstraction.

Re:I think tumblr has a huge security hole by jarin on Saturday March 19, 2011 @08:20AM (#2343405)
That's why I always git diff and run tests before committing, I do that all the time :-/
Re:I think tumblr has a huge security hole by drdaeman on Saturday March 19, 2011 @08:42AM (#2343449)
And this is the reason one should separate production and development environments and NEVER edit any files on production servers (let `git push` or something similiar handle it)
Re:I think tumblr has a huge security hole by agotterer on Saturday March 19, 2011 @12:33PM (#2343986)
Shouldn't this have been caught by checking chsnges before committing them or on a staging server.