How to prevent a leak like Tumblr’s | Hacker News Reader

How to prevent a leak like Tumblr’s

blog.kevburnsjr.com

10 pointsKevBurnsJr19 comments

"The only solution I see to this is pre-commit syntax checking for committed PHP files."

How would that have helped here? The file was obviously still valid syntactically. Try it for yourself: create a PHP file that looks like

    i?php
    $config_data = array();

and run it through PHP's syntax check (php -l /path/to/file from the command line). It will verify that the script is valid. However, the content will still not be parsed as PHP.

One way to fix this is to store your configuration data in another file format (eg: YAML) outside of the document root. Since the configuration file doesn't try to be parsed as PHP, it can't be leaked in this way.

Well, arguably, the syntax checker could detect "valid but dangerous" PHP statements. So, for instance, if there is a terminating </php> but no <php>, that could be triggered as suspicious or dangerous. (I'm just saying this as a general example, but I know it might not be the best solution for different reasons).

Also, unit tests should definitely find that error.. no? I mean, a simple high level test that use something in config.php should fail.

Yeah, my solution is to have a separate config file too but have a config file solely for password and never check that file into the repository.

Of course whenever someone does a fresh pull of the project, that is one file that they'd have to create for themselves. (So this is documented very clearly.)

It's minor inconvenience but it solves the problems like this.

Our solution is to store environment-specific config data (such as DB name, username, password, etc.) in the Apache config:

SetEnv DB_USER=kermit SetEnv DB_PASS=Shhh

And then connect to the DB using code such as:

mysql_connect(getenv('DB_NAME'), getenv('DB_USER'), getenv('DB_PASS'))

Environmental config data should never be stored in source code.

Until someone types i?php when when creating the file ;)

Edit: Unless of course you're using YAML or some other representation rather than a pure PHP file.

Wow, you're right. I sort of assumed it would pick up on an error like this.

It physically can't: without the opening PHP tag the parser just thinks the entire file is HTML to be outputted to the browser, which is still a valid PHP file. ;)

I suppose you could also write a quick bash script to ensure the first 5 characters of any .class.php file matches "<?php", but it wouldn't be very generally applicable and wouldn't fix a problem like this for a file named config.php

I suppose you could add some code to Varnish to tell it to squelch any and all 500 responses.

Looks like PHP returns a 200 when it encounters a fatal error.

Edit: Here's a way to force 500 for fatal errors in PHP http://blog.kevburnsjr.com/php-fatal-error-500

I'm far from being a PHP expert, but wouldn't writing critical password/data in a file instead of directly in the source code, avoided that issue? I mean, we would have seen the code to read the file.. and not the pass?

It's a symptom of PHP's slightly horrible execution model. When you request a page, a new PHP process spins up, parses it, compiles it, and executes it. There is no shared state between requests within PHP (generally - there are ways to add it) and so if configuration was stored in YAML or XML, it would have to be parsed /on every request/.

Many people have made suggestions on how to trap these kinds of error... storing PHP files outside of document root is a good practice, otherwise the code can be revealed if the webserver stops interpreting PHP files for whatever reason.

Any amount of testing would catch accidentally dumping the configuration file, if it isn't being parsed then nothing on the site will work. Development -> testing -> production setups aren't as common in the PHP world as they should be, though.

Personally, I'm tempted to say store configuration in a .ini or YAML file or similar. If you're building a small site, it won't be a big speed issue anyway. If it becomes a speed issue, there are many ways to mitigate it, such as compiling the human-written config file to a PHP file, removing the human element that is more likely to transpose a character.

Yes

Does the person not use syntax highlighting? A missing <?php tag would cause the PHP code to be highlighted as HTML code which would look very strange to anyone who has edited a PHP file with a syntax-highlighting-enabled editor.

Static code analysis is nice to have, but at least they should have some form of automated testing.

How about: test on your staging environment before deploying to production?

use emacs, not vi :-P

I wanted to joke about something like that ;) (Even thought I much more prefer vim, it happens to me from time to time to see a i/j/whatever in my file and getting random errors)

Maybe the individual actually was using emacs, but thought they were in vim.

Reply on news.ycombinator.com