1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-09 16:32:39 +01:00

Create AphrontWriteGuard, a backup mechanism for CSRF validation

Summary:
Provide a catchall mechanism to find unprotected writes.

  - Depends on D758.
  - Similar to WriteOnHTTPGet stuff from Facebook's stack.
  - Since we have a small number of storage mechanisms and highly structured
read/write pathways, we can explicitly answer the question "is this page
performing a write?".
  - Never allow writes without CSRF checks.
  - This will probably break some things. That's fine: they're CSRF
vulnerabilities or weird edge cases that we can fix. But don't push to Facebook
for a few days unless you're prepared to deal with this.
  - **>>> MEGADERP: All Conduit write APIs are currently vulnerable to CSRF!
<<<**

Test Plan:
  - Ran some scripts that perform writes (scripts/search indexers), no issues.
  - Performed normal CSRF submits.
  - Added writes to an un-CSRF'd page, got an exception.
  - Executed conduit methods.
  - Did login/logout (this works because the logged-out user validates the
logged-out csrf "token").
  - Did OAuth login.
  - Did OAuth registration.

Reviewers: pedram, andrewjcg, erling, jungejason, tuomaspelkonen, aran,
codeblock
Commenters: pedram
CC: aran, epriestley, pedram
Differential Revision: 777
This commit is contained in:
epriestley 2011-08-03 11:49:27 -07:00
parent 68c30e1a71
commit 39b4d20ce5
29 changed files with 397 additions and 2 deletions

View file

@ -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);

View file

@ -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',

View file

@ -0,0 +1,256 @@
<?php
/*
* Copyright 2011 Facebook, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
/**
* Guard writes against CSRF. The Aphront structure takes care of most of this
* for you, you just need to call:
*
* AphrontWriteGuard::willWrite();
*
* ...before executing a write against any new kind of storage engine. MySQL
* databases and the default file storage engines are already covered, but if
* you introduce new types of datastores make sure their writes are guarded. If
* you don't guard writes and make a mistake doing CSRF checks in a controller,
* a CSRF vulnerability can escape undetected.
*
* If you need to execute writes on a page which doesn't have CSRF tokens (for
* example, because you need to do logging), you can temporarily disable the
* write guard by calling:
*
* AphrontWriteGuard::beginUnguardedWrites();
* do_logging_write();
* AphrontWriteGuard::endUnguardedWrites();
*
* This is dangerous, because it disables the backup layer of CSRF protection
* this class provides. You should need this only very, very rarely.
*
* @task protect Protecting Writes
* @task disable Disabling Protection
* @task manage Managing Write Guards
* @task internal Internals
*
* @group aphront
*/
final class AphrontWriteGuard {
private static $instance;
private static $allowUnguardedWrites = false;
private $request;
private $allowDepth = 0;
/* -( Managing Write Guards )---------------------------------------------- */
/**
* Construct a new write guard for a request. Only one write guard may be
* active at a time. You must explicitly call @{method:dispose} when you are
* done with a write guard:
*
* $guard = new AphrontWriteGuard();
* // ...
* $guard->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.");
}
}
}

View file

@ -0,0 +1,12 @@
<?php
/**
* This file is automatically generated. Lint this module to rebuild it.
* @generated
*/
phutil_require_module('phabricator', 'aphront/writeguard/scopeguard');
phutil_require_source('AphrontWriteGuard.php');

View file

@ -0,0 +1,28 @@
<?php
/*
* Copyright 2011 Facebook, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
/**
* @group aphront
*/
final class AphrontScopedUnguardedWriteCapability {
final public function __destruct() {
AphrontWriteGuard::endUnguardedWrites();
}
}

View file

@ -0,0 +1,12 @@
<?php
/**
* This file is automatically generated. Lint this module to rebuild it.
* @generated
*/
phutil_require_module('phabricator', 'aphront/writeguard');
phutil_require_source('AphrontScopedUnguardedWriteCapability.php');

View file

@ -129,7 +129,8 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController {
}
$oauth_info->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();
}
}

View file

@ -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');

View file

@ -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;
}

View file

@ -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');

View file

@ -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(

View file

@ -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.";
}

View file

@ -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.";
}

View file

@ -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.";
}

View file

@ -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);
}
}

View file

@ -6,6 +6,7 @@
phutil_require_module('phabricator', 'aphront/writeguard');
phutil_require_module('phabricator', 'infrastructure/javelin/markup');
phutil_require_module('phabricator', 'view/base');

View file

@ -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 =

View file

@ -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');

View file

@ -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);
}
}

View file

@ -6,6 +6,7 @@
phutil_require_module('phabricator', 'aphront/writeguard');
phutil_require_module('phabricator', 'applications/files/engine/base');
phutil_require_module('phabricator', 'infrastructure/env');

View file

@ -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);

View file

@ -6,6 +6,7 @@
phutil_require_module('phabricator', 'aphront/writeguard');
phutil_require_module('phabricator', 'applications/files/engine/base');
phutil_require_module('phabricator', 'infrastructure/env');

View file

@ -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);
}
}
}

View file

@ -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');

View file

@ -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 '.

View file

@ -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');

View file

@ -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);

View file

@ -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');

View file

@ -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}");