FIX Prevent DOS by checking for env and admin on ?flush=1 (#1692) #2243

Merged
merged 1 commit into from Jul 19, 2013

Projects

None yet

5 participants

@hafriedlander
Member

Ensure that flush=1 only causes an actual flush if one of the following is true

  • You are in dev mode
  • You are logged in as an admin
  • An error occurred while attempted to start up the page

Fixes #1692. This is the version for 3.0 (and 3.1 though there'll be a couple of merge errors there). Version for 2.4 is at #2246

@hafriedlander
Member

OK, in live mode it will now redirect you to the login page. Uses REQUEST_URI - the internet seems to indicate this isn't very reliable in IIS, but we use it all over the place, and I can't find somewhere in the bootstrap where we fix it for IIS.

@camspiers camspiers and 1 other commented on an outdated diff Jul 18, 2013
core/startup/ErrorControlChain.php
+ public function error() {
+ if ($this->suppression && defined('BASE_URL')) throw Exception('Generic Error');
+ else return false;
+ }
+
+ public function fatalError() {
+ if ($this->handleFatalErrors && $this->suppression && defined('BASE_URL')) {
+ if(($error = error_get_last()) && ($error['type'] == 1)) {
+ ob_clean();
+ $this->error = true;
+ $this->step();
+ }
+ }
+ }
+
+ public function then($ignorePrevErrors, $callback = null) {
@camspiers
camspiers Jul 18, 2013 Contributor

Maybe $ignorePrevErrors should be named $ignorePrevErrorsOrCallback, thoughts?

@hafriedlander
hafriedlander Jul 18, 2013 Member

Previous errors are always ignored. Passing true means execute calls $callback even when there has been errors. False (or one argument) means execute only calls callback when no errors in previous callbacks.

@hafriedlander
hafriedlander Jul 18, 2013 Member

I have been thinking about splitting this into three methods though, thenIfGood, thenAlways and thenIfErrored, just for fluency

@camspiers
camspiers Jul 18, 2013 Contributor

Maybe I am not understanding the code, but what I meant was that because $ignorePrevErrors can actually be a callback, from a docs and API perspective, it being named $ignorePrevErrors is misleading.

@hafriedlander
hafriedlander Jul 18, 2013 Member

Oh, right. Yeah maybe. I ended up going with my other thought though, since it's easy to miss the magic first parameter. See the rebased PR I'll push in a minute.

@camspiers
Contributor

Very awesome :). I just wonder if its complexity warrants unit testing?

@hafriedlander
Member

Possibly, although the two trickiest things are both to do with fatal error catching - one is having it happen at all, the other is the fact that ob_clean gets rid of the error message PHP generates (which works in at least 5.3 & 5.4 and I'm testing 5.2 & 5.5 now, but it doesn't seem like PHP's internal fatal error handler calling ob_start is documented anywhere)

I'll get this cleaned up for 3.0 & 2.4 first, then have a look at some basic tests.

@hafriedlander
Member

OK, basic tests for ErrorControlChain in. Thanks for poking me, I caught a bug because of them.

@camspiers
Contributor

I think that is should be able to be tested without causing a fatal error. We aren't really looking to test that php's shutdown function behaviour is working, we just need to test things like when fatalError is called, callbacks with onErrorState equal to null or true will still be called etc.

We can introduce a new method getLastError:

protected function getLastError() {
    return error_get_last();
}

then mock execute, and mock getLastError call fatalError and check that the callbacks where onErrorState is null or true are called.

@camspiers camspiers commented on an outdated diff Jul 19, 2013
core/startup/ParameterConfirmationToken.php
+
+ public function reloadWithToken() {
+ global $url;
+
+ // Are we http or https?
+ $proto = 'http';
+
+ if(isset($_SERVER['HTTP_X_FORWARDED_PROTOCOL'])) {
+ if(strtolower($_SERVER['HTTP_X_FORWARDED_PROTOCOL']) == 'https') $proto = 'https';
+ }
+
+ if((!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] != 'off')) $proto = 'https';
+ if(isset($_SERVER['SSL'])) $proto = 'https';
+
+ // What's our host
+ $host = $_SERVER['HTTP_HOST'] ;
@camspiers
camspiers Jul 19, 2013 Contributor

Minor, whitespace ;

@camspiers camspiers commented on an outdated diff Jul 19, 2013
core/startup/ParameterConfirmationToken.php
+ * Class ParameterConfirmationToken
+ *
+ * When you need to use a dangerous GET parameter that needs to be set before core/Core.php is
+ * established, this class takes care of allowing some other code of confirming the parameter,
+ * by generating a one-time-use token & redirecting with that token included in the redirected URL
+ *
+ * WARNING: This class is experimental and designed specifically for use pre-startup in main.php
+ * It will likely be heavily refactored before the release of 3.2
+ */
+class ParameterConfirmationToken {
+ protected $parameterName = null;
+ protected $parameter = null;
+ protected $token = null;
+
+ protected function pathForToken($token) {
+ if (defined(BASE_PATH)) {
@camspiers
camspiers Jul 19, 2013 Contributor

I think this should be defined('BASE_PATH')

@hafriedlander
Member

Thanks, fixed. I've introduced lastErrorWasFatal to ErrorControlChain to be able to override in a mock, but haven't added a test to use it (I really dislike PHPUnit's built in mocking, and this one test isn't good enough reason to pull Phockito in as a dependancy).

@sminnee sminnee merged commit 7656a22 into silverstripe:3.0 Jul 19, 2013

1 check failed

default Scrutinizer: 1 Comments, 0 Changed Files — Travis: Passed
Details
@camspiers
Contributor

Phockito would definitely make things more readable :). Considering the desired functionality can be achieved with PHPUnit I think it would still be good to test with PHPUnit test doubles. I think it would be great to add Phockito prior to the upcoming transition to using better dependency injection practices throughout framework.

Maybe a guide to testing practices that covered dealing with dependencies would be good prior to the transition.

@ARNHOE
Contributor
ARNHOE commented Aug 7, 2013

@hafriedlander @chillu
I think since this commit; dont shoot me if isnt; there is a problem with showing friendly errors. I have a hosting where I need to disable magic_quotes_qpc I think before this commit, it was giving this error:

// No more magic_quotes!
        trigger_error('get_magic_quotes_gpc support is being removed from Silverstripe. Please set this to off in ' .
        ' your php.ini and see http://php.net/manual/en/security.magicquotes.php', E_USER_WARNING);

But now it gives:

Fatal error: Call to undefined function stripslashes_recursively() in framework/core/Constants.php on line 129
@chillu
Member
chillu commented Aug 7, 2013

@ARNHOE mentioned on IRC that this might've fixed it #2285

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment