diff --git a/scripts/__init_script__.php b/scripts/__init_script__.php index 814acebc46..ba3c9562ae 100644 --- a/scripts/__init_script__.php +++ b/scripts/__init_script__.php @@ -29,3 +29,7 @@ if (!@constant('__LIBPHUTIL__')) { } phutil_load_library(dirname(__FILE__).'/../src/'); + +// NOTE: This is dangerous in general, but we know we're in a script context and +// are not vulnerable to CSRF. +AphrontWriteGuard::allowDangerousUnguardedWrites(true); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8e097b6371..3e2e54abe6 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -71,6 +71,7 @@ phutil_register_library_map(array( 'AphrontRequest' => 'aphront/request', 'AphrontRequestFailureView' => 'view/page/failure', 'AphrontResponse' => 'aphront/response/base', + 'AphrontScopedUnguardedWriteCapability' => 'aphront/writeguard/scopeguard', 'AphrontSideNavView' => 'view/layout/sidenav', 'AphrontTableView' => 'view/control/table', 'AphrontTokenizerTemplateView' => 'view/control/tokenizer', @@ -78,6 +79,7 @@ phutil_register_library_map(array( 'AphrontURIMapper' => 'aphront/mapper', 'AphrontView' => 'view/base', 'AphrontWebpageResponse' => 'aphront/response/webpage', + 'AphrontWriteGuard' => 'aphront/writeguard', 'CelerityAPI' => 'infrastructure/celerity/api', 'CelerityResourceController' => 'infrastructure/celerity/controller', 'CelerityResourceMap' => 'infrastructure/celerity/map', diff --git a/src/aphront/writeguard/AphrontWriteGuard.php b/src/aphront/writeguard/AphrontWriteGuard.php new file mode 100644 index 0000000000..b192f77270 --- /dev/null +++ b/src/aphront/writeguard/AphrontWriteGuard.php @@ -0,0 +1,256 @@ +dispose(); + * + * Normally, you do not need to manage guards yourself -- the Aphront stack + * handles it for you. + * + * @param AphrontRequest Request to read CSRF token information from. + * @return this + * @task manage + */ + public function __construct(AphrontRequest $request) { + if (self::$instance) { + throw new Exception( + "An AphrontWriteGuard already exists. Dispose of the previous guard ". + "before creating a new one."); + } + if (self::$allowUnguardedWrites) { + throw new Exception( + "An AphrontWriteGuard is being created in a context which permits ". + "unguarded writes unconditionally. This is not allowed and indicates ". + "a serious error."); + } + $this->request = $request; + self::$instance = $this; + } + + + /** + * Dispose of the active write guard. You must call this method when you are + * done with a write guard. You do not normally need to call this yourself. + * + * @return void + * @task manage + */ + public function dispose() { + if ($this->allowDepth > 0) { + throw new Exception( + "Imbalanced AphrontWriteGuard: more beginUnguardedWrites() calls than ". + "endUnguardedWrites() calls."); + } + self::$instance = null; + } + + +/* -( Protecting Writes )-------------------------------------------------- */ + + + /** + * Declare intention to perform a write, validating that writes are allowed. + * You should call this method before executing a write whenever you implement + * a new storage engine where information can be permanently kept. + * + * Writes are permitted if: + * + * - The request has valid CSRF tokens. + * - Unguarded writes have been temporarily enabled by a call to + * @{method:beginUnguardedWrites}. + * - All write guarding has been disabled with + * @{method:allowDangerousUnguardedWrites}. + * + * If none of these conditions are true, this method will throw and prevent + * the write. + * + * @return void + * @task protect + */ + public static function willWrite() { + if (!self::$instance) { + if (!self::$allowUnguardedWrites) { + throw new Exception( + "Unguarded write! There must be an active AphrontWriteGuard to ". + "perform writes."); + } else { + // Unguarded writes are being allowed unconditionally. + return; + } + } + + $instance = self::$instance; + + if ($instance->allowDepth == 0) { + $instance->request->validateCSRF(); + } + } + + +/* -( Disabling Write Protection )----------------------------------------- */ + + + /** + * Enter a scope which permits unguarded writes. This works like + * @{method:beginUnguardedWrites} but returns an object which will end + * the unguarded write scope when its __destruct() method is called. This + * is useful to more easily handle exceptions correctly in unguarded write + * blocks: + * + * // Restores the guard even if do_logging() throws. + * function unguarded_scope() { + * $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + * do_logging(); + * } + * + * @return AphrontScopedUnguardedWriteCapability Object which ends unguarded + * writes when it leaves scope. + * @task disable + */ + public static function beginScopedUnguardedWrites() { + self::beginUnguardedWrites(); + return new AphrontScopedUnguardedWriteCapability(); + } + + + /** + * Begin a block which permits unguarded writes. You should use this very + * sparingly, and only for things like logging where CSRF is not a concern. + * + * You must pair every call to @{method:beginUnguardedWrites} with a call to + * @{method:endUnguardedWrites}: + * + * AphrontWriteGuard::beginUnguardedWrites(); + * do_logging(); + * AphrontWriteGuard::endUnguardedWrites(); + * + * @return void + * @task disable + */ + public static function beginUnguardedWrites() { + if (!self::$instance) { + return; + } + self::$instance->allowDepth++; + } + + /** + * Declare that you have finished performing unguarded writes. You must + * call this exactly once for each call to @{method:beginUnguardedWrites}. + * + * @return void + * @task disable + */ + public static function endUnguardedWrites() { + if (!self::$instance) { + return; + } + if (self::$instance->allowDepth <= 0) { + throw new Exception( + "Imbalanced AphrontWriteGuard: more endUnguardedWrites() calls than ". + "beginUnguardedWrites() calls."); + } + self::$instance->allowDepth--; + } + + + /** + * Allow execution of unguarded writes. This is ONLY appropriate for use in + * script contexts or other contexts where you are guaranteed to never be + * vulnerable to CSRF concerns. Calling this method is EXTREMELY DANGEROUS + * if you do not understand the consequences. + * + * If you need to perform unguarded writes on an otherwise guarded workflow + * which is vulnerable to CSRF, use @{method:beginUnguardedWrites}. + * + * @return void + * @task disable + */ + public static function allowDangerousUnguardedWrites($allow) { + if (self::$instance) { + throw new Exception( + "You can not unconditionally disable AphrontWriteGuard by calling ". + "allowDangerousUnguardedWrites() while a write guard is active. Use ". + "beginUnguardedWrites() to temporarily allow unguarded writes."); + } + self::$allowUnguardedWrites = true; + } + + +/* -( Internals )---------------------------------------------------------- */ + + + /** + * When the object is destroyed, make sure @{method:dispose} was called. + */ + public function __destruct() { + if (isset(self::$instance)) { + throw new Exception( + "AphrontWriteGuard was not properly disposed of! Call dispose() on ". + "every AphrontWriteGuard object you instantiate."); + } + } +} diff --git a/src/aphront/writeguard/__init__.php b/src/aphront/writeguard/__init__.php new file mode 100644 index 0000000000..e90cf76d3e --- /dev/null +++ b/src/aphront/writeguard/__init__.php @@ -0,0 +1,12 @@ +setUserID($current_user->getID()); - $oauth_info->save(); + + $this->saveOAuthInfo($oauth_info); return id(new AphrontRedirectResponse()) ->setURI('/settings/page/'.$provider_key.'/'); @@ -149,7 +150,7 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { $session_key = $known_user->establishSession('web'); - $oauth_info->save(); + $this->saveOAuthInfo($oauth_info); $request->setCookie('phusr', $known_user->getUsername()); $request->setCookie('phsid', $session_key); @@ -317,4 +318,12 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { return $oauth_info; } + private function saveOAuthInfo(PhabricatorUserOAuthInfo $info) { + // UNGUARDED WRITES: Logging-in users don't have their CSRF set up yet. + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $info->save(); + } + + + } diff --git a/src/applications/auth/controller/oauth/__init__.php b/src/applications/auth/controller/oauth/__init__.php index b3ee8dda26..034e422097 100644 --- a/src/applications/auth/controller/oauth/__init__.php +++ b/src/applications/auth/controller/oauth/__init__.php @@ -9,6 +9,7 @@ phutil_require_module('phabricator', 'aphront/response/400'); phutil_require_module('phabricator', 'aphront/response/dialog'); phutil_require_module('phabricator', 'aphront/response/redirect'); +phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'applications/auth/controller/base'); phutil_require_module('phabricator', 'applications/auth/oauth/provider/base'); phutil_require_module('phabricator', 'applications/auth/view/oauthfailure'); diff --git a/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php index f2f08e6177..731c5f6a2e 100644 --- a/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php @@ -91,12 +91,23 @@ class PhabricatorConduitAPIController $api_request = new ConduitAPIRequest($params); + $allow_unguarded_writes = false; $auth_error = null; if ($method_handler->shouldRequireAuthentication()) { $auth_error = $this->authenticateUser($api_request, $metadata); + // If we've explicitly authenticated the user here and either done + // CSRF validation or are using a non-web authentication mechanism. + $allow_unguarded_writes = true; + } + + if ($method_handler->shouldAllowUnguardedWrites()) { + $allow_unguarded_writes = true; } if ($auth_error === null) { + if ($allow_unguarded_writes) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + } try { $result = $method_handler->executeMethod($api_request); $error_code = null; @@ -106,6 +117,9 @@ class PhabricatorConduitAPIController $error_code = $ex->getMessage(); $error_info = $method_handler->getErrorDescription($error_code); } + if ($allow_unguarded_writes) { + unset($unguarded); + } } else { list($error_code, $error_info) = $auth_error; } @@ -132,7 +146,9 @@ class PhabricatorConduitAPIController // we only really care about having these logs for real CLI clients, if // even that. if (empty($metadata['authToken'])) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); $log->save(); + unset($unguarded); } $result = array( @@ -170,6 +186,7 @@ class PhabricatorConduitAPIController $request = $this->getRequest(); if ($request->getUser()->getPHID()) { + $request->validateCSRF(); $api_request->setUser($request->getUser()); return null; } diff --git a/src/applications/conduit/controller/api/__init__.php b/src/applications/conduit/controller/api/__init__.php index efb0757303..706e380ddf 100644 --- a/src/applications/conduit/controller/api/__init__.php +++ b/src/applications/conduit/controller/api/__init__.php @@ -7,6 +7,7 @@ phutil_require_module('phabricator', 'aphront/response/file'); +phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'applications/conduit/controller/base'); phutil_require_module('phabricator', 'applications/conduit/method/base'); phutil_require_module('phabricator', 'applications/conduit/protocol/request'); diff --git a/src/applications/conduit/method/base/ConduitAPIMethod.php b/src/applications/conduit/method/base/ConduitAPIMethod.php index 4cf59a2148..3ba1a83760 100644 --- a/src/applications/conduit/method/base/ConduitAPIMethod.php +++ b/src/applications/conduit/method/base/ConduitAPIMethod.php @@ -52,6 +52,10 @@ abstract class ConduitAPIMethod { return true; } + public function shouldAllowUnguardedWrites() { + return false; + } + public static function getAPIMethodNameFromClassName($class_name) { $match = null; $is_valid = preg_match( diff --git a/src/applications/conduit/method/conduit/connect/ConduitAPI_conduit_connect_Method.php b/src/applications/conduit/method/conduit/connect/ConduitAPI_conduit_connect_Method.php index 6e3e04d813..f9cf7c6be3 100644 --- a/src/applications/conduit/method/conduit/connect/ConduitAPI_conduit_connect_Method.php +++ b/src/applications/conduit/method/conduit/connect/ConduitAPI_conduit_connect_Method.php @@ -25,6 +25,10 @@ class ConduitAPI_conduit_connect_Method extends ConduitAPIMethod { return false; } + public function shouldAllowUnguardedWrites() { + return true; + } + public function getMethodDescription() { return "Connect a session-based client."; } diff --git a/src/applications/conduit/method/daemon/launched/ConduitAPI_daemon_launched_Method.php b/src/applications/conduit/method/daemon/launched/ConduitAPI_daemon_launched_Method.php index 6c7b3e02e2..40de95cf49 100644 --- a/src/applications/conduit/method/daemon/launched/ConduitAPI_daemon_launched_Method.php +++ b/src/applications/conduit/method/daemon/launched/ConduitAPI_daemon_launched_Method.php @@ -26,6 +26,10 @@ class ConduitAPI_daemon_launched_Method extends ConduitAPIMethod { return false; } + public function shouldAllowUnguardedWrites() { + return false; + } + public function getMethodDescription() { return "Used by daemons to log run status."; } diff --git a/src/applications/conduit/method/daemon/log/ConduitAPI_daemon_log_Method.php b/src/applications/conduit/method/daemon/log/ConduitAPI_daemon_log_Method.php index f7ba004d09..c89469d77c 100644 --- a/src/applications/conduit/method/daemon/log/ConduitAPI_daemon_log_Method.php +++ b/src/applications/conduit/method/daemon/log/ConduitAPI_daemon_log_Method.php @@ -26,6 +26,10 @@ class ConduitAPI_daemon_log_Method extends ConduitAPIMethod { return false; } + public function shouldAllowUnguardedWrites() { + return false; + } + public function getMethodDescription() { return "Used by daemons to log events."; } diff --git a/src/applications/differential/view/inlinecomment/DifferentialInlineCommentView.php b/src/applications/differential/view/inlinecomment/DifferentialInlineCommentView.php index 5e90dcaf79..0b40eb8eca 100644 --- a/src/applications/differential/view/inlinecomment/DifferentialInlineCommentView.php +++ b/src/applications/differential/view/inlinecomment/DifferentialInlineCommentView.php @@ -158,7 +158,10 @@ final class DifferentialInlineCommentView extends AphrontView { $content = $this->markupEngine->markupText($content); if ($inline->getID()) { $inline->setCache($content); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); $inline->save(); + unset($unguarded); } } diff --git a/src/applications/differential/view/inlinecomment/__init__.php b/src/applications/differential/view/inlinecomment/__init__.php index 71452b2ed7..aee6d7ccc2 100644 --- a/src/applications/differential/view/inlinecomment/__init__.php +++ b/src/applications/differential/view/inlinecomment/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'infrastructure/javelin/markup'); phutil_require_module('phabricator', 'view/base'); diff --git a/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php b/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php index 8976952bc0..723322fc5b 100644 --- a/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php +++ b/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php @@ -129,7 +129,10 @@ final class DifferentialRevisionCommentView extends AphrontView { $content = $this->markupEngine->markupText($content); if ($comment->getID()) { $comment->setCache($content); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); $comment->save(); + unset($unguarded); } } $content = diff --git a/src/applications/differential/view/revisioncomment/__init__.php b/src/applications/differential/view/revisioncomment/__init__.php index f105a69222..8dee4bedd6 100644 --- a/src/applications/differential/view/revisioncomment/__init__.php +++ b/src/applications/differential/view/revisioncomment/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'applications/differential/constants/action'); phutil_require_module('phabricator', 'applications/differential/storage/comment'); phutil_require_module('phabricator', 'infrastructure/celerity/api'); diff --git a/src/applications/files/engine/localdisk/PhabricatorLocalDiskFileStorageEngine.php b/src/applications/files/engine/localdisk/PhabricatorLocalDiskFileStorageEngine.php index e3b0b2d87b..26bb03d481 100644 --- a/src/applications/files/engine/localdisk/PhabricatorLocalDiskFileStorageEngine.php +++ b/src/applications/files/engine/localdisk/PhabricatorLocalDiskFileStorageEngine.php @@ -66,6 +66,7 @@ final class PhabricatorLocalDiskFileStorageEngine execx('mkdir -p %s', $parent); } + AphrontWriteGuard::willWrite(); Filesystem::writeFile($root.'/'.$name, $data); return $name; @@ -89,6 +90,7 @@ final class PhabricatorLocalDiskFileStorageEngine public function deleteFile($handle) { $path = $this->getLocalDiskFileStorageFullPath($handle); if (Filesystem::pathExists($path)) { + AphrontWriteGuard::willWrite(); Filesystem::remove($path); } } diff --git a/src/applications/files/engine/localdisk/__init__.php b/src/applications/files/engine/localdisk/__init__.php index b6639e48e0..354e96acea 100644 --- a/src/applications/files/engine/localdisk/__init__.php +++ b/src/applications/files/engine/localdisk/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'applications/files/engine/base'); phutil_require_module('phabricator', 'infrastructure/env'); diff --git a/src/applications/files/engine/s3/PhabricatorS3FileStorageEngine.php b/src/applications/files/engine/s3/PhabricatorS3FileStorageEngine.php index fa3eb607dd..41f5fb5abf 100644 --- a/src/applications/files/engine/s3/PhabricatorS3FileStorageEngine.php +++ b/src/applications/files/engine/s3/PhabricatorS3FileStorageEngine.php @@ -49,6 +49,7 @@ final class PhabricatorS3FileStorageEngine $name = 'phabricator/'.sha1(Filesystem::readRandomBytes(20)); + AphrontWriteGuard::willWrite(); $s3->putObject( $data, $this->getBucketName(), @@ -76,6 +77,8 @@ final class PhabricatorS3FileStorageEngine * @task impl */ public function deleteFile($handle) { + + AphrontWriteGuard::willWrite(); $this->newS3API()->deleteObject( $this->getBucketName(), $handle); diff --git a/src/applications/files/engine/s3/__init__.php b/src/applications/files/engine/s3/__init__.php index 80acf0c1d2..e2e88309c4 100644 --- a/src/applications/files/engine/s3/__init__.php +++ b/src/applications/files/engine/s3/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'applications/files/engine/base'); phutil_require_module('phabricator', 'infrastructure/env'); diff --git a/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php b/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php index 2588264a8e..e11852f786 100644 --- a/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php +++ b/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php @@ -179,7 +179,9 @@ class ManiphestTransactionDetailView extends ManiphestView { $comments = $this->markupEngine->markupText($comments); $comment_transaction->setCache($comments); if ($comment_transaction->getID() && !$this->preview) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); $comment_transaction->save(); + unset($unguarded); } } } diff --git a/src/applications/maniphest/view/transactiondetail/__init__.php b/src/applications/maniphest/view/transactiondetail/__init__.php index bbb604b5bc..b1c34a4630 100644 --- a/src/applications/maniphest/view/transactiondetail/__init__.php +++ b/src/applications/maniphest/view/transactiondetail/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'applications/maniphest/constants/priority'); phutil_require_module('phabricator', 'applications/maniphest/constants/status'); phutil_require_module('phabricator', 'applications/maniphest/constants/transactiontype'); diff --git a/src/applications/people/storage/user/PhabricatorUser.php b/src/applications/people/storage/user/PhabricatorUser.php index 33f206b481..043ee96e2f 100644 --- a/src/applications/people/storage/user/PhabricatorUser.php +++ b/src/applications/people/storage/user/PhabricatorUser.php @@ -252,6 +252,9 @@ class PhabricatorUser extends PhabricatorUserDAO { $entropy = Filesystem::readRandomBytes(20); $session_key = sha1($entropy); + // UNGUARDED WRITES: Logging-in users don't have CSRF stuff yet. + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + queryfx( $conn_w, 'INSERT INTO %T '. diff --git a/src/applications/people/storage/user/__init__.php b/src/applications/people/storage/user/__init__.php index 2f78f50a24..d7fd517a2d 100644 --- a/src/applications/people/storage/user/__init__.php +++ b/src/applications/people/storage/user/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'applications/people/storage/base'); phutil_require_module('phabricator', 'applications/people/storage/log'); phutil_require_module('phabricator', 'applications/people/storage/preferences'); diff --git a/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php b/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php index 4f5a21bc01..4cce4ed84e 100644 --- a/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php +++ b/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php @@ -208,6 +208,12 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection { try { $this->requireConnection(); + // TODO: Do we need to include transactional statements here? + $is_write = !preg_match('/^(SELECT|SHOW)\s/', $raw_query); + if ($is_write) { + AphrontWriteGuard::willWrite(); + } + $start = microtime(true); $profiler = PhutilServiceProfiler::getInstance(); @@ -216,6 +222,7 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection { 'type' => 'query', 'config' => $this->configuration, 'query' => $raw_query, + 'write' => $is_write, )); $result = @mysql_query($raw_query, $this->connection); diff --git a/src/storage/connection/mysql/__init__.php b/src/storage/connection/mysql/__init__.php index f1ec809cb5..7b181925c5 100644 --- a/src/storage/connection/mysql/__init__.php +++ b/src/storage/connection/mysql/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'storage/connection/base'); phutil_require_module('phabricator', 'storage/exception/accessdenied'); phutil_require_module('phabricator', 'storage/exception/base'); diff --git a/webroot/index.php b/webroot/index.php index c9912c5cfc..44159da28b 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -116,6 +116,9 @@ $application->setHost($host); $application->setPath($path); $application->willBuildRequest(); $request = $application->buildRequest(); + +$write_guard = new AphrontWriteGuard($request); + $application->setRequest($request); list($controller, $uri_data) = $application->buildController(); try { @@ -136,9 +139,13 @@ try { $response->setRequest($request); $response_string = $response->buildResponseString(); } catch (Exception $ex) { + $write_guard->dispose(); phabricator_fatal('[Rendering Exception] '.$ex->getMessage()); } +$write_guard->dispose(); + + $code = $response->getHTTPResponseCode(); if ($code != 200) { header("HTTP/1.0 {$code}");