Fix conservative CSRF token cycling limit

Summary:
We currently cycle CSRF tokens every hour and check for the last two valid ones.
This means that a form could go stale in as little as an hour, and is certainly
stale after two.

When a stale form is submitted, you basically get a terrible heisen-state where
some of your data might persist if you're lucky but more likely it all just
vanishes. The .js file below outlines some more details.

This is a pretty terrible UX and we don't need to be as conservative about CSRF
validation as we're being. Remedy this problem by:

  - Accepting the last 6 CSRF tokens instead of the last 1 (i.e., pages are
valid for at least 6 hours, and for as long as 7).
  - Using JS to refresh the CSRF token every 55 minutes (i.e., pages connected
to the internet are valid indefinitely).
  - Showing the user an explicit message about what went wrong when CSRF
validation fails so the experience is less bewildering.

They should now only be able to submit with a bad CSRF token if:

  - They load a page, disconnect from the internet for 7 hours, reconnect, and
submit the form within 55 minutes; or
  - They are actually the victim of a CSRF attack.

We could eventually fix the first one by tracking reconnects, which might be
"free" once the notification server gets built. It will probably never be an
issue in practice.

Test Plan:
  - Reduced CSRF cycle frequency to 2 seconds, submitted a form after 15
seconds, got the CSRF exception.
  - Reduced csrf-refresh cycle frequency to 3 seconds, submitted a form after 15
seconds, got a clean form post.
  - Added debugging code the the csrf refresh to make sure it was doing sensible
things (pulling different tokens, finding all the inputs).

Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley
Differential Revision: 660

src/__celerity_resource_map__.php

@@ -293,17 +293,6 @@ celerity_register_resource_map(array(
     ),
     'disk' => '/rsrc/js/javelin/lib/behavior.js',
   ),
-  0 =>
-  array(
-    'uri' => '/res/1da00bfe/rsrc/js/javelin/lib/__tests__/URI.js',
-    'type' => 'js',
-    'requires' =>
-    array(
-      0 => 'javelin-uri',
-      1 => 'javelin-php-serializer',
-    ),
-    'disk' => '/rsrc/js/javelin/lib/__tests__/URI.js',
-  ),
   'javelin-behavior-aphront-basic-tokenizer' =>
   array(
     'uri' => '/res/5e183bd5/rsrc/js/application/core/behavior-tokenizer.js',
@@ -601,6 +590,17 @@ celerity_register_resource_map(array(
     ),
     'disk' => '/rsrc/js/application/maniphest/behavior-transaction-preview.js',
   ),
+  0 =>
+  array(
+    'uri' => '/res/1da00bfe/rsrc/js/javelin/lib/__tests__/URI.js',
+    'type' => 'js',
+    'requires' =>
+    array(
+      0 => 'javelin-uri',
+      1 => 'javelin-php-serializer',
+    ),
+    'disk' => '/rsrc/js/javelin/lib/__tests__/URI.js',
+  ),
   'javelin-behavior-owners-path-editor' =>
   array(
     'uri' => '/res/9cf78ffc/rsrc/js/application/owners/owners-path-editor.js',
@@ -652,6 +652,18 @@ celerity_register_resource_map(array(
     ),
     'disk' => '/rsrc/js/application/core/behavior-watch-anchor.js',
   ),
+  'javelin-behavior-refresh-csrf' =>
+  array(
+    'uri' => '/res/39aa51f7/rsrc/js/application/core/behavior-refresh-csrf.js',
+    'type' => 'js',
+    'requires' =>
+    array(
+      0 => 'javelin-request',
+      1 => 'javelin-behavior',
+      2 => 'javelin-dom',
+    ),
+    'disk' => '/rsrc/js/application/core/behavior-refresh-csrf.js',
+  ),
   'javelin-behavior-workflow' =>
   array(
     'uri' => '/res/079f49c3/rsrc/js/application/core/behavior-workflow.js',
@@ -1137,7 +1149,7 @@ celerity_register_resource_map(array(
   ),
   'phriction-document-css' =>
   array(
-    'uri' => '/res/c08b5f24/rsrc/css/application/phriction/phriction-document-css.css',
+    'uri' => '/res/77d5f57f/rsrc/css/application/phriction/phriction-document-css.css',
     'type' => 'css',
     'requires' =>
     array(

src/__phutil_library_map__.php

@@ -14,6 +14,7 @@ phutil_register_library_map(array(
     'AphrontAjaxResponse' => 'aphront/response/ajax',
     'AphrontApplicationConfiguration' => 'aphront/applicationconfiguration',
     'AphrontAttachedFileView' => 'view/control/attachedfile',
+    'AphrontCSRFException' => 'aphront/exception/csrf',
     'AphrontController' => 'aphront/controller',
     'AphrontCrumbsView' => 'view/layout/crumbs',
     'AphrontDatabaseConnection' => 'storage/connection/base',
@@ -464,6 +465,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectProfileEditController' => 'applications/project/controller/profileedit',
     'PhabricatorProjectStatus' => 'applications/project/constants/status',
     'PhabricatorRedirectController' => 'applications/base/controller/redirect',
+    'PhabricatorRefreshCSRFController' => 'applications/auth/controller/refresh',
     'PhabricatorRemarkupRuleDifferential' => 'infrastructure/markup/remarkup/markuprule/differential',
     'PhabricatorRemarkupRuleDiffusion' => 'infrastructure/markup/remarkup/markuprule/diffusion',
     'PhabricatorRemarkupRuleImageMacro' => 'infrastructure/markup/remarkup/markuprule/imagemacro',
@@ -626,6 +628,7 @@ phutil_register_library_map(array(
     'Aphront404Response' => 'AphrontResponse',
     'AphrontAjaxResponse' => 'AphrontResponse',
     'AphrontAttachedFileView' => 'AphrontView',
+    'AphrontCSRFException' => 'AphrontException',
     'AphrontCrumbsView' => 'AphrontView',
     'AphrontDefaultApplicationConfiguration' => 'AphrontApplicationConfiguration',
     'AphrontDefaultApplicationController' => 'AphrontController',
@@ -981,6 +984,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectProfileController' => 'PhabricatorProjectController',
     'PhabricatorProjectProfileEditController' => 'PhabricatorProjectController',
     'PhabricatorRedirectController' => 'PhabricatorController',
+    'PhabricatorRefreshCSRFController' => 'PhabricatorAuthController',
     'PhabricatorRemarkupRuleDifferential' => 'PhabricatorRemarkupRuleObjectName',
     'PhabricatorRemarkupRuleDiffusion' => 'PhutilRemarkupRule',
     'PhabricatorRemarkupRuleImageMacro' => 'PhutilRemarkupRule',

src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php

@@ -133,6 +133,7 @@ class AphrontDefaultApplicationConfiguration
         '$' => 'PhabricatorLoginController',
         'email/$' => 'PhabricatorEmailLoginController',
         'etoken/(?P<token>\w+)/$' => 'PhabricatorEmailTokenController',
+        'refresh/$' => 'PhabricatorRefreshCSRFController',
       ),
 
       '/logout/$' => 'PhabricatorLogoutController',

src/aphront/exception/csrf/AphrontCSRFException.php

@@ -0,0 +1,21 @@
+<?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.
+ */
+
+class AphrontCSRFException extends AphrontException {
+
+}

src/aphront/exception/csrf/__init__.php

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

src/aphront/request/AphrontRequest.php

@@ -118,9 +118,29 @@ class AphrontRequest {
   }
 
   final public function isFormPost() {
-    return $this->getExists(self::TYPE_FORM) &&
-           $this->isHTTPPost() &&
-           $this->getUser()->validateCSRFToken($this->getStr('__csrf__'));
+    $post = $this->getExists(self::TYPE_FORM) &&
+            $this->isHTTPPost();
+
+    if (!$post) {
+      return false;
+    }
+
+    $valid = $this->getUser()->validateCSRFToken($this->getStr('__csrf__'));
+    if (!$valid) {
+      // This should only be able to happen if you load a form, pull your
+      // internet for 6 hours, and then reconnect and immediately submit,
+      // but give the user some indication of what happened since the workflow
+      // is incredibly confusing otherwise.
+      throw new AphrontCSRFException(
+        "The form you just submitted did not include a valid CSRF token. ".
+        "This token is a technical security measure which prevents a ".
+        "certain type of login hijacking attack. However, the token can ".
+        "become invalid if you leave a page open for more than six hours ".
+        "without a connection to the internet. To fix this problem: reload ".
+        "the page, and then resubmit it.");
+    }
+
+    return true;
   }
 
   final public function getCookie($name, $default = null) {

src/aphront/request/__init__.php

@@ -6,6 +6,8 @@
 
 
 
+phutil_require_module('phabricator', 'aphront/exception/csrf');
+
 phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');
 

src/applications/auth/controller/refresh/PhabricatorRefreshCSRFController.php

@@ -0,0 +1,32 @@
+<?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.
+ */
+
+class PhabricatorRefreshCSRFController extends PhabricatorAuthController {
+
+  public function processRequest() {
+    $request = $this->getRequest();
+    $user = $request->getUser();
+
+    return id(new AphrontAjaxResponse())
+      ->setContent(
+        array(
+          'token' => $user->getCSRFToken(),
+        ));
+  }
+
+}

src/applications/auth/controller/refresh/__init__.php

@@ -0,0 +1,15 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'aphront/response/ajax');
+phutil_require_module('phabricator', 'applications/auth/controller/base');
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('PhabricatorRefreshCSRFController.php');

src/applications/people/storage/user/PhabricatorUser.php

@@ -130,7 +130,35 @@ class PhabricatorUser extends PhabricatorUserDAO {
   }
 
   public function validateCSRFToken($token) {
-    for ($ii = -1; $ii <= 1; $ii++) {
+
+    // When the user posts a form, we check that it contains a valid CSRF token.
+    // Tokens cycle each hour (every CSRF_CYLCE_FREQUENCY seconds) and we accept
+    // either the current token, the next token (users can submit a "future"
+    // token if you have two web frontends that have some clock skew) or any of
+    // the last 6 tokens. This means that pages are valid for up to 7 hours.
+    // There is also some Javascript which periodically refreshes the CSRF
+    // tokens on each page, so theoretically pages should be valid indefinitely.
+    // However, this code may fail to run (if the user loses their internet
+    // connection, or there's a JS problem, or they don't have JS enabled).
+    // Choosing the size of the window in which we accept old CSRF tokens is
+    // an issue of balancing concerns between security and usability. We could
+    // choose a very narrow (e.g., 1-hour) window to reduce vulnerability to
+    // attacks using captured CSRF tokens, but it's also more likely that real
+    // users will be affected by this, e.g. if they close their laptop for an
+    // hour, open it back up, and try to submit a form before the CSRF refresh
+    // can kick in. Since the user experience of submitting a form with expired
+    // CSRF is often quite bad (you basically lose data, or it's a big pain to
+    // recover at least) and I believe we gain little additional protection
+    // by keeping the window very short (the overwhelming value here is in
+    // preventing blind attacks, and most attacks which can capture CSRF tokens
+    // can also just capture authentication information [sniffing networks]
+    // or act as the user [xss]) the 7 hour default seems like a reasonable
+    // balance. Other major platforms have much longer CSRF token lifetimes,
+    // like Rails (session duration) and Django (forever), which suggests this
+    // is a reasonable analysis.
+    $csrf_window = 6;
+
+    for ($ii = -$csrf_window; $ii <= 1; $ii++) {
       $valid = $this->getCSRFToken($ii);
       if ($token == $valid) {
         return true;

src/view/page/standard/PhabricatorStandardPageView.php

@@ -122,6 +122,7 @@ class PhabricatorStandardPageView extends AphrontPageView {
     require_celerity_resource('phabricator-standard-page-view');
 
     Javelin::initBehavior('workflow', array());
+    Javelin::initBehavior('refresh-csrf', array());
     Javelin::initBehavior(
       'phabricator-keyboard-shortcuts',
       array(

webroot/rsrc/js/application/core/behavior-refresh-csrf.js

@@ -0,0 +1,44 @@
+/**
+ * @provides javelin-behavior-refresh-csrf
+ * @requires javelin-request
+ *           javelin-behavior
+ *           javelin-dom
+ */
+
+/**
+ * We cycle CSRF tokens every hour but accept the last 6, which means that if
+ * you leave a page open for more than 6 hours before submitting it you may hit
+ * CSRF protection. This is a super confusing workflow which potentially
+ * discards data, and we can't recover from it by re-issuing a CSRF token
+ * since that would leave us vulnerable to CSRF attacks.
+ *
+ * Our options basically boil down to:
+ *
+ *   - Increase the CSRF window to something like 24 hours. This makes the CSRF
+ *     implementation vaguely less secure and only mitigates the problem.
+ *   - Code all forms to understand GET, POST and POST-with-invalid-CSRF. This
+ *     is a huge undertaking and difficult to test properly since it is hard
+ *     to remember to test the third phantom state.
+ *   - Use JS to refresh the CSRF token.
+ *
+ * Since (1) mitigates rather than solving and (2) is a huge mess, this
+ * behavior implements (3) and refreshes all the CSRF tokens on the page every
+ * 55 minutes. This allows forms to remain valid indefinitely.
+ */
+JX.behavior('refresh-csrf', function(config) {
+
+  function refresh_csrf() {
+    new JX.Request('/login/refresh/', function(r) {
+        var inputs = JX.DOM.scry(document.body, 'input');
+        for (var ii = 0; ii < inputs.length; ii++) {
+          if (inputs[ii].name == '__csrf__') {
+            inputs[ii].value = r.token;
+          }
+        }
+      })
+      .send();
+  }
+
+  // Refresh the CSRF tokens every 55 minutes.
+  setInterval(refresh_csrf, 1000 * 60 * 55);
+});