4 ms·
How to prevent a leak like Tumblr’s
- nbpoole 16y ago"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.
- highwind81 16y agoYeah, 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.
- nbpoole 16y agoUntil 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.
- jcstauffer 16y agoOur 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.
- KevBurnsJr 16y agoWow, you're right. I sort of assumed it would pick up on an error like this.
- nbpoole 16y agoIt 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. ;)
- KevBurnsJr 16y agoI 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
- KevBurnsJr 16y agoI suppose you could add some code to Varnish to tell it to squelch any and all 500 responses.
- KevBurnsJr 16y agoLooks 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 http://blog.kevburnsjr.com/php-fatal-error-500
- d0m 16y agoWell, 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.
- revorad 16y agouse emacs, not vi :-P
- d0m 16y agoI'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?
- nbpoole 16y agoYes
- ZoFreX 16y agoIt'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.
- tszming 16y agoStatic code analysis is nice to have, but at least they should have some form of automated testing.
- orlandu63 16y agoDoes 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.
- getsat 16y agoHow about: test on your staging environment before deploying to production?