[2013-02-28] Require ADMIN for ?flush=1 (stop denial of service attacks)

[silverstripe-issues]

created by: @chillu (ischommer)
created at: 2013-02-28
original ticket: http://open.silverstripe.org/ticket/8290

---

See https://groups.google.com/forum/?fromgroups=#!topic/silverstripe-dev/XDUDZtr9Gbk

Already enforced for flush=all, but not for flush=1.

Ensure that you can execute the flush unauthenticated if Database::is_ready() == false,
because otherwise we're getting into a chicken and egg situation: Can't log in due
to stale manifest (and SQL errors etc), can't clear stale manifest because not logged in.

[tractorcow]

How does this look?

// In Director.php

    /**
     * Check requested flush level, given permission is available, or the 
     * database is not ready.
     * 
     * @param string $value An optional flush value to require. E.g. 'all'
     * @return boolean A flag indicating that the requsted flush level is given and authorised
     */
    public static function is_flush($value = null) {

        // Rule out unflushed cases
        if(empty($_GET['flush'])) return false;
        if($value && ($_GET['flush'] != $value)) return false;

        // Allow flush in dev or CLI
        if(self::isDev() || self::is_cli()) return true;

        // If a database error would otherwise prevent authentication, permit flush
        if(!DB::isActive() || !DB::getConn()->hasTable('Member')) return true;

        // Safely check permission, erring on the side of safety
        try {
            $permission = @Permission::check('ADMIN');
            if(is_bool($permission)) return $permission;
        } catch(Exception $ex) {
            exceptionHandler($ex);
        }

        return true;
    }

[simonwelsh]

The problem here is flushing the manifests is resource intensive. That code won’t work until after the manifests, especially the config one, have been loaded.

[UndefinedOffset]

What if the flush requires the site to be in dev mode? Then at least so long as the site is in live mode it won't do it? If I remember right in 3.0 flush is passed into the various manifest builders in Core.php which would be before the session/database is ready right?

[hafriedlander]

This can't check the environment - normally you set dev mode through _ss_environment, which gets loaded as part of config loading, so too late.

Some things we could do, most of which are annoying one way or another

• Monitor when flush was last called, and forbid flushing too frequently
• Make flushing CLI-only
• Have a secret token in the codebase, require flush to be passed the token as a param (flush=mysecret instead of flush=1)
• Do flush checking after startup, as per @tractorcow but if flush is indicated then make the browser reload with a one-time use token as a GET parameter

That last one is nicest, but if the site doesn't work at all because flush is needed you'd have to fall back to CLI. You might be able to combine it with error checking, so it the site has an error the flush happens anyway.

[camspiers]

I think that I have a possible solution to this, critique it at will. And I have started work on a PR.

The idea is instead of passing in a flush value to the instantiation of SS_ConfigStaticManifest, SS_ClassManifest, SS_ConfigManifest and SS_TemplateManifest you let them run as they would without that value (so, if the cache exists then use it, if not then generate it and set a prop indicating that a generate has happened). Later on in the execution (after the db is ready in main.php) you check for flush and admin, and then call the regenerate method on the manifests if they weren't already generated.

If you want me to continue with this solution I can likely finish today.

[simonwelsh]

What happens if you reference a new class in your _config.php? Flush needs to be able to happen before user code is loaded.

[tractorcow]

I wonder if this could be solved in a simpler way. Could there be a physical token (i.e. a file) which either locks or unlocks the ability to flush? A site could be 'secured' from flushing by having a blank _flush_exclude in the site root.

This is similar in the way that the _manifest_exclude is used to exclude a directory from being autoloaded.

This eliminates the necessity for DB or class loading, but has the downside of requiring physical access to the site root. The default case could be as the code currently works - No file means flushing is available at all times, and it could be something uploaded to productions servers when not being maintained.

[camspiers]

@simonwelsh That is a good point. I didn't think about that. :)

[camspiers]

Monitor when flush was last called, and forbid flushing too frequently

Definitely an option, but hopefully we can find something nicer.

Make flushing CLI-only

I really like this option, but I imagine that it would be a support issue. I work on one site where I don't actually have SSH access and can only run cron jobs.

Have a secret token in the codebase, require flush to be passed the token as a param (flush=mysecret instead of flush=1)

The file could be generated if it didn't exist, meaning security by default. Would it be placed in users codebase i.e. mysite and not framework or cms? I imagine this would be necessary as at least in my case, deps are handled by composer and therefore can't be modded.

Do flush checking after startup, as per @tractorcow but if flush is indicated then make the browser reload with a one-time use token as a GET parameter

Can you expand on this more?

[hafriedlander]

Can you expand on this more?

Two requests.

First request looks like ?flush=1. After core/Core.php is included, Director::is_flush is called. It sees this is a legitimate flush (because is_dev, or Permission::check or whatever), so does two things. First, it writes a file somewhere like $secret_token="4asdf7";, then it does header("Location: $current_location?flush=4asdf7"); die;.

Second request, before core/Core.php, checks flush against token in written file. Matches, so (a) deletes file, and (b) actually does the flush

Issue: core/Core.php might error out because the manifest needs flushing (new class referenced in _config.php a la @simonwelsh). So we need to detect an error when including core/Core.php, and if there is one, and flush=1, do the same thing as that first request anyway (write token, redirect), without actually calling Director::is_flush.

[chillu]

+1 for Hamish's approach. BTW, I've introduced a Security.token YAML config in order to safely switch databases via a web URL for Behat behaviour testing. At the moment it needs to be generated manually (through RandomGenerator and dev/generatetoken. Theoretically we could avoid the redirect with that. It would only become useful for flush=1 once its generated by default, presumably in the installer.

[camspiers]

Would this approach still support cli? I guess if flush=1 and cli then regenerate?

[sminnee]

IMO, CLI should just work, so the initial call can just run if Director::is_cli() is true.

[chillu]

Here's my current status. Its complex, and ugly. https://github.com/chillu/silverstripe-framework/tree/pulls/flush-ddos

Redirecting from Core.php is difficult, because our logic for retrieving the URL from the CGI environment isn't trivial, and not encapsulated. I don't want to start refactoring this too heavily because Im aware of Andrew's work for 3.2 bootstrapper. Even if we get the URL, a redirection won't work because the "Class not found" E_FATAL will have already been sent to the browser, meaning we can't send further headers. A temporary display_errors=0 or ob_start() could fix this, but seems like a hack? I've tried registering a fallback autoloader that triggers the manifest build instead of the PHP shutdown function, but that triggers for each class_exists() check in core for a rightfully missing class, e.g. Translatable.

Using a one-time use token has one serious disadvantage: You can't refresh the browser window on the redirection target. Particularly in template development where you need to flush every couple of seconds to check a template change, it will get highly annoying to manually rewrite the URL. We could start the session earlier in case a flush is requested explicitly, and make it a session-based token?

For now, instead of redirecting I'm building the manifest in the first request (using the PHP shutdown function).
Then a message gets output after the fatal error asking to refresh the page:
http://monosnap.com/image/wAQsf2wQeQSLN3SbtXuEtXjc7.png
Its not pretty, and depending on your error_reporting() might not even show...

Here's some test cases I've run:

• Database ready, dev mode, first build on empty cache: Flush triggered by initial loadManifest() call
• Database not ready, dev mode, first build on empty cache: Builds db, flush triggered by initial loadManifest() call
• Database not ready, live mode through _config.php, first build on empty cache: Builds db, flush triggered by initial loadManifest() call
• Missing and undefined class. live mode through _config.php: Regenerates twice, shows error message (same on reload, as expected)
• Missing but defined class, live mode through _config.php: Shows error, works on refresh
• Missing but defined class, live mode through _config.php, ?flush=1: Regenerates, works on first load
• Missing but defined class, dev/build, live mode through _config.php, without ?flush=1: Shows error, works on refresh

P.S.: We wouldn't have this problem in the first place if we used PSR0 autoloading, and removed our weird reliance on implicit registration through PHP interfaces like TemplateGlobalProvider and i18nEntityProvider, right?

[chillu]

Talked to Sam, action points:

• introduce a JS refresh on class error (and plaintext error on CLI)
• move DB::connect to Core.php, introduce EnvironmentUnconfiguredException to

Rough HTML code (needs cross browser testing to ensure it works after outputting the E_FATAL error message):

<p id=pleaseRefresh>Please refresh</p>
<script>
document.getElementById('pleaseRefresh').innerHTML = 'Refreshing...';
setTimeout(function() {document.location.refresh() }, 1000);
</script>

[chillu]

Update: chillu@9d6acfc
@sminnee Could you peer review please?

I've added an infinite loop protection to the JS refresh, with a flushed=1 marker. This is necessary to avoid cases where a manifest flush doesn't fix the issue. Side effect is that any subsequent refreshed on this new URL with the marker won't cause a manifest build, but that's unavoidable.

On CLI, I'm just doing a die() with a note that manifest has been cleared, and the user should rerun the command.

More tests performed:

• Database not ready, live mode through _config.php, first build on empty cache through CLI: Builds cache, doesn't build DB (on php cli-script.php dev), but works
  • Missing but defined class, live mode, CLI: Shows error, works on refresh
  • Missing but defined class, live mode, CLI, flush=1: Works on first try
  • No missing classes, live mode, flush=1, not logged in: Works on first try
  • No missing classes, live mode, flush=1, not logged in: die() with a message asking to login
  • No missing classes, live mode, flush=1, logged in: Works on first try

[chillu]

Fixed for 2.4 as well. Created pull requests on github.com/silverstripe so somebody can pick up the commits in my absence (starting in a couple of hours). Please don't forget to squash the 3.0 commits after peer review, before merge.
Also needs to be merged and tested into 3.1 and master.

P.S.: I don't envy @ajshort in merging this all into https://github.com/ajshort/sapphire/tree/composer :/

[tractorcow]

Hi @chillu, I'm a little concerned about some of these changes. moving DB::connect into core.php is going to change some of the assumptions around the code that may be relied on by other applications. A single large file is quite rigid.

I can only speak of my own experience with the code, but for instance my dynamiccache module relies on core.php to initialise the configuration system, but intercepts requests to conditionally delay or prevent execution that requires database connection. Such a change as proposed would make this impossible (at least, not without duplicating the entire code in an external module).

Wouldn't you agree that 3.1 RC is close enough to warrant delaying such a significant change?

You could also argue that I'm the only one affected though. :) It might not be the best example, but there will often be times where individually projects will need to interact with the bootstrapping process, by having their own bootstrapping files (while including core ones it depends on).

I know of about 2-3 projects where this has been necessary (one where DB connection had to be conditionally executed based on cached results, another where session loading had to be delayed to integrate with another application on the same domain).

Could core.php be broken up from a "one file to rule them all" bootstrapper into something that ties together a set of individually includable files? One for each of the sections under?

///////////////////////////////////////////////////////////////////////////////
// MANIFEST

You could still get all the functionality in this pull request, but presented as a toolbox of steps, rather than one mandatory process.

[ss23]

@tractorcow While it's a valid point, I feel like leaving this in for longer could be worse. Not that I think people going to DoS you is likely, but if it exists, someone might do it, and then your only real option is to hack on the codebase, which would be worse than a (somewhat minor) compatibility break, or use a WAF or something to block the requests.

Then again, it hasn't been an issue for anyone so far... shrug

[tractorcow]

@ss23 That's appreciated and understood, thanks. :) Please check the considerations I've noted and let me know if it's acceptable to allow Core.php to remain extendable.

[hafriedlander]

For those concerned about the possibility of an attacker attempting to use this prior to a fix being released, there are server-level workarounds that will help mitigate attacks, including:

• Forbid the use of the flush GET parameter except from a whitelist of IPs. Something like this in the base .htaccess should work:

RewriteCond %{QUERY_STRING} ^flush\b [NC,OR]
RewriteCond %{QUERY_STRING} &flush\b [NC]
RewriteCond %{REMOTE_ADDR} !^xx.xx.xx.xx$
RewriteRule .* - [F,L]

You could also do something similar at the top of main.php, like

if (isset($_GET['flush']) && $_SERVER['REMOTE_ADDR'] != 'xx.xx.xx.xx') die;

You could also use a secret second GET parameter or other methods of identification if you're also worried about IP spoofing.

• Rate limit requests (with the flush GET parameter, or just in general) - for example with mod_qos under Apache

[moveforward]

@hafriedlander thanks for the suggestions. As there has been some discussion around this issue on the NZ PHP Users Group, it's also good to see the discussion here and to have some interim workarounds until a fix is released.

[simonwelsh]

The rewrite rules will need to also block ?flush\b and &flush\b (not sure if RewriteCond lets you use word-boundary matches)

[hafriedlander]

Good point Simon, I've updated that comment. Apache does seem to support \b.

[hafriedlander]

OK, another attempt, @sminnee & @bummzack do you see any problem with #2254 and #2255?

[bummzack]

@hafriedlander looks good. I agree that including _ss_environment.php beforehand is a good idea...

@sminnee Didn't know about that recommended approach for symlinked environments. Thanks for pointing it out.

[tractorcow]

I noted an issue with the current 3.1 framework which is probably related to the recent changes. Removing a module from an existing project and attempting to flush results in a crash. SS_ConfigManifest seems unflushable if there is an error which should require the cache to be invalidated.

Sorry I haven't been following the progress of this update as of late, so I can't say what's causing this.

[camspiers]

I have experienced the same thing. I renamed a module and had to do a
manual delete of the cache.

On Tuesday, 23 July 2013, Damian Mooyman wrote:

I noted an issue with the current 3.1 framework which is probably related
to the recent changes. Removing a module from an existing project and
attempting to flush results in a crash. SS_ConfigManifest seems
unflushable if there is an error which should require the cache to be
invalidated.

Sorry I haven't been following the progress of this update as of late, so
I can't say what's causing this.

[image: image]https://f.cloud.github.com/assets/936064/837647/9723ba48-f30b-11e2-97eb-c7f96846d358.png

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/1692#issuecomment-21372431
.

[hafriedlander]

@tractorcow @camspiers ErrorControlChain should detect if there's an error & suppress it, allowing the flush even when in live mode & not logged in, except with BASE_URL isn't defined, in which case it allows the error through - looks like there's a code path where removing a module triggers an error before BASE_URL gets set.

The new patch #2254 and #2255 should fix that, since BASE_URL is set prior to entering the chain

[sminnee]

@tractorcow @camspiers - I think this might actually be fixed by @hafriedlander's ErrorControlChain.

However, Robert Curry has noticed a bug with the current implementation that I think is a showstopper. If you use "@" to suppress an error message (not especially tidy, but it happens) then the error is no longer suppressed.

[camspiers]

@sminnee I will test against the latest. Thanks

[hafriedlander]

@camspiers I haven't merged into 3.1 yet, so you'll need to use latest 3.0

[camspiers]

Oh, right. :)

[camspiers]

I'm on 3.0 @ 88d0cbe and I am experiencing the following after renaming a module:

And then when I try a flush:

PHP Version info if relevant:

PHP 5.3.26 (cli) (built: Jul  6 2013 17:24:30)
Copyright (c) 1997-2013 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2013 Zend Technologies

[hafriedlander]

@camspiers Can you confirm #2257 fixes the issue?

[camspiers]

Yep. That fixes it :)

I still get the error without the flush (is that expected), but then I can add the flush and it works.

[hafriedlander]

Your issue should now by fixed in 3.1 @camspiers and @tractorcow. If you could test it again & let me know if you still have problems, that'd be great.

[tractorcow]

@hafriedlander Yep it's fixed!

[camspiers]

On 3.1 @ 541436f When running ?flush=1 in dev mode after I have renamed a module, I get this:

And then if I look in my php error log I have:

[24-Jul-2013 13:46:05 Pacific/Auckland] PHP Warning:  require_once(/Users/cameron/modules/silverstripe/testmodule/_config.php) [<a href='function.require-once'>function.require-once</a>]: failed to open stream: No such file or directory in /Users/cameron/modules/silverstripe/framework/core/manifest/ConfigManifest.php on line 141
[24-Jul-2013 13:46:05 Pacific/Auckland] PHP Fatal error:  require_once() [<a href='function.require'>function.require</a>]: Failed opening required '/Users/cameron/modules/silverstripe/testmodule/_config.php' (include_path=...) in /Users/cameron/modules/silverstripe/framework/core/manifest/ConfigManifest.php on line 141
[24-Jul-2013 13:46:05 Pacific/Auckland] PHP Warning:  Cannot modify header information - headers already sent in /Users/cameron/modules/silverstripe/framework/core/startup/ParameterConfirmationToken.php on line 97

[hafriedlander]

@camspiers Argh. You will still get the errors in the log, as ErrorControlChain doesn't suppress logging them. You shouldn't get that last warning though - looks like something PHP 5.3 specific where PHP thinks headers have already been sent. Looking now.

[camspiers]

Should probably mention that it doesn't flush. So it isn't just that I get
the errors.

On Wednesday, 24 July 2013, Hamish Friedlander wrote:

@camspiers https://github.com/camspiers Argh. You will still get the
errors in the log, as ErrorControlChain doesn't suppress logging them. You
shouldn't get that last warning though - looks like something PHP 5.3
specific where PHP thinks headers have already been sent. Looking now.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/1692#issuecomment-21459262
.

[hafriedlander]

Yeah, unless you get a flushtoken, it won't flush. Problem though: I manually removed my silverstripe cache as part of testing, and now I can't replicate the issue even in PHP 5.3. I did notice that if you run out of memory because of a corrupt cache you can't flush, but I don't think that's what you're seeing.

[ss23]

I suspect some of these changes are the cause of issues I'm having on latest 3.1, specifically, the changes to not catch more errors.

Previously, working was

$foo = DBField::create('HTMLText', null);
echo $foo;

Because it's a fatal catchable (Catchable fatal error: Method HTMLText::__toString() must return a string value), it was being caught and ignored.
Now it's not caught, and causing a fatal error in the script.

While I like getting this change into 3.1, I would prefer there weren't BC breaks like this so close to a release. Can we get that commit changed/reverted and fix it in 3.2 instead? (I think it does need to be fixed, of course).

[camspiers]

I'll do some more digging for you. Hopefully it is user error on my end.

On Wednesday, 24 July 2013, Hamish Friedlander wrote:

Yeah, unless you get a flushtoken, it won't flush. Problem though: I
manually removed my silverstripe cache as part of testing, and now I can't
replicate the issue even in PHP 5.3. I did notice that if you run out of
memory because of a corrupt cache you can't flush, but I don't think that's
what you're seeing.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/1692#issuecomment-21459616
.

[hafriedlander]

@ss23 that should be fixed as of this release - it only catches fatal errors that aren't otherwise caught by the error handling system.

Can we get that commit changed/reverted and fix it in 3.2 instead?

We need to fix this issue before 3.1.0. In fact, this same fix is being applied to 2.4 and 3.0 as well. There are alternative solutions with varying impacts (see #1692 (comment)) but I can't see us releasing 3.1.0 without this being addressed somehow.

[ss23]

@hafriedlander Ah, sorry, I see I wasn't running on the very latest 3.1. Indeed, looks like it's back to working now.

[hafriedlander]

@camspiers Cool, thanks. I've put my experimental changes on a personal branch at https://github.com/hafriedlander/silverstripe-framework/commits/fix/flush_31 - it fixes issues related to running out of memory, and as a last ditch if headers_sent is true it'll just dump a link rather than try and send headers anyway.

[camspiers]

Yuss!. I figured it out. It was my setup that was causing in. I have php_value auto_prepend_file "/Users/cameron/Sites/xhgui/external/header.php in my .htaccess and that was outputting something (no idea what).

Sorry about that Hamish.

What do you think of the use of meta refresh for the case where headers have been sent?

[hafriedlander]

Probably not a bad idea - there'll always be someone who accidentally leaves a space before the opening <?php stanza somewhere, and ugly is better than WSOD.

[bummzack]

@hafriedlander Oh man, I don't envy you. You seem to have opened pandoras box with these changes :-) Just wanted to let you know that everything works fine now with a symlinked install. Thanks for all your work on this.

[UndefinedOffset]

Shouldn't the core also check for admin on the Image class https://github.com/silverstripe/silverstripe-framework/blob/3.1/model/Image.php#L415 seeing how that can also be very taxing?

[UndefinedOffset]

Never mind... as simon_w pointed out in the channel the flush key is being unset https://github.com/silverstripe/silverstripe-framework/blob/3.0/main.php#L108-L110

[chillu]

This has been released as part of 3.1.0-rc1