Port Cross-site Request Forgery - Form API fixes from SA-CORE-2015-003 to Drupal 8

This should do it.

A patch that doesn't break all forms should work better :)

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -31,6 +32,8 @@
   +  use StringTranslationTrait;
   

   This is sad and problematic. Hopefully it can be removed if we refactor to delegate to FormValidator.

   Otherwise, we need to inject the string translation service to not depend on \Drupal.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -105,6 +108,27 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
   +  protected $safeCoreValueCallables = [
   

   We need to make this extensible, in this patch, not in a follow-up.

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -817,6 +841,23 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
   +        if (isset($element['#token'])) {
   +          $input = $form_state->getUserInput();
   +          if (empty($input['form_token']) || !$this->csrfToken->validate($input['form_token'], $element['#token'])) {
   +            // Set an early form error to block certain input processing since
   +            // that opens the door for CSRF vulnerabilities.
   +            $url = $this->requestStack->getCurrentRequest()->getRequestUri();
   +
   +            // Setting this error will cause the form to fail validation.
   +            $form_state->setErrorByName('form_token', $this->t('The form has become outdated. Copy any unsaved work in the form below and then <a href="@link">reload this page</a>.', array('@link' => $url)));
   +
   +            // This value is checked in self::handleInputElement().
   +            $form_state->setInvalidToken(TRUE);
   +
   +            // Make sure file uploads do not get processed.
   +            $_FILES = array();
   +          }
   +        }
          }
   

   Large parts of this are duplicating stuff that's in \Drupal\Core\Form\FormValidator::validateForm(), we should address that instead of copy/pasting.

We need to make this extensible, in this patch, not in a follow-up.

Why wasn't it made extensible in the D7 patch?

Large parts of this are duplicating stuff that's in \Drupal\Core\Form\FormValidator::validateForm(), we should address that instead of copy/pasting.

If by "large parts" you mean those two lines of code that get the URL and set the form error, then sure, how about this?

+++ b/core/lib/Drupal/Core/Form/FormValidatorInterface.php
@@ -54,4 +54,14 @@ public function executeValidateHandlers(&$form, FormStateInterface &$form_state)
+  public function setInvalidTokenError(FormStateInterface $form_state);

I would feel a lot better if we could put this on the FormState object instead, but that one doesn't use StringTranslationTrait either...

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -105,6 +105,27 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
+  protected $safeCoreValueCallables = [

Don't we want a more fundamental solution in D8, rather than using the whitelist that D7 *had* to use, because anything else would mean a BC break? Or is that for a follow-up?

No idea, I asked a similar thing in #7.1 :)

Fixed the newly added test, the other two test failures are above my knowledge in this area.

So the reasons we did not make it extensible in the sdo issue was:
a) it would have taken longer
b) we had many options, and wanted to debate the approach in public amongst more than those working on the security issue

If by "large parts" you mean those two lines of code that get the URL

I meant the if() parts too, because they are very similar, but I guess they are slightly different? Hm. But yes, that change is a step in the right direction.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -817,6 +845,20 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
+            // Set an early form error to block certain input processing since
+            // that opens the door for CSRF vulnerabilities.
+            $this->setInvalidTokenError($form_state);
+
+            // This value is checked in self::handleInputElement().
+            $form_state->setInvalidToken(TRUE);

Could setInvalidToken and setInvalidTokenError be combined? I know they're currently on different objects... Or one could call the other

Could setInvalidToken and setInvalidTokenError be combined? I know they're currently on different objects... Or one could call the other

I don't think we can do that because the call to setInvalidTokenError() from FromValidator::validateForm() has a check for $form_state->hasInvalidToken() in its if() condition.

Could we add a new empty interface for those elements to say that their value callbacks are safe to run? Then any element that meets the criteria can opt-in to being safe to run. I don't have a great name for that interface, but seems the easiest way to communicate it.

Could we add a new empty interface for those elements to say that their value callbacks are safe to run?

That could potentially work for the valueCallback() method of a class, but wouldn't work for a '#value_callback' override, such as color_palette_color_value().

#17: Can we require #value_callbacks to be classes?

We could at least require them to be classes and implement that interface if they want to be treated as safe? And any other callable is unsafe by default.

#19: that was the alternative that came to mind :)

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -105,6 +105,27 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
   /**
+   * Defines element value callables which are safe to run even when the form
+   * state has an invalid CSRF token.
+   *
+   * @todo Drupal 8 has some new form elements, check if any of them could be
+   *   added to this list.
+   *
+   * @var array
+   */

Can we explain why its okay to call them?

So for example, a ValueCallbackIsSafe interface that any class can implement, and implementing that means that that class's valueCallback() method (and only that one) is safe to invoke even if there are errors already flagged (such as an invalid CSRF token)? Then, what if a class subclasses such a class and overrides valueCallback() with an unsafe implementation? I don't think there's a way to unimplement an interface in a subclass.

Seems like what we really want is to add some metadata to a function/method. Any reason not to do that with an annotation? One reason is we don't yet annotate functions in Drupal, only classes, but is that really a compelling reason not to start to do so?

Could we make the white-list a container parameter and leave it as is?

If your module has another safe value callback, implement ServiceModifierInterface and add to the white-list.

We currently don't do any annotation parsing on runtime, only on rebuilds. So we'd either need to break that pattern (could get expensive doing it every time though?) or build a registry of safe-to-call methods. I'm not sure if those are compelling reasons not to do it but they're the concerns that immediately came to mind.

Then, what if a class subclasses such a class and overrides valueCallback() with an unsafe implementation?

Is there any unsafe implementation other than a file upload? If so how likely is it that someone would subclass a class that's not for a file upload and add one if so?

It'd also be possible to mark the method itself as final. I've never seen this used anywhere, but PHP does allow it.

I like #23. Just for completeness, some other options:

1) Call all value callbacks, and make it their responsibility to check $form_state->getErrors() prior to doing something unsafe.

2) Add a by-reference parameter to valueCallback(). E.g. function valueCallback(&$element, $input, FormStateInterface $form_state, &$is_safe). Then, when FormBuilder invokes value callbacks on elements, if there are form errors, it first invokes the callback with $input=FALSE, and if that invocation results in $is_safe being set to TRUE, then it invokes it again with the user's input. Extra function call, but only during relatively rare situations.

3) Create a $form_state->setValueCallbackIsSafe($value_callback) method, and let modules decide where they want to invoke that from to mark their value callback as safe.

Is there any unsafe implementation other than a file upload?

If we only cared about file uploads, then we wouldn't need to whitelist value callbacks at all, we could just empty out $_FILES and be done. color_palette_color_value() is another example of one though: it currently implements its own CSRF token validation because that predates SA-CORE-2015-003. Since that's an example, there's probably others out there somewhere.

This was discussed on the EU criticals call, by Alex Pott, catch, larowlan, dawehner and I. The consensus was:

• This issue should only fix the critical part, and use the same approach that D7 uses: a hardcoded whitelist.
• File a follow-up to allow contrib/custom to whitelist additional value callbacks, OR use the interface approach discussed in the last few comments, OR somehow change Form API to ensure that value callbacks are always safe.
• By the time we are working on that follow-up, we'll have more feedback from actual D7 sites, which will inform us whether the whitelist approach is sufficient, if it's not then one of the other approaches listed in the previous point will have to be used.

Here's a fix for a couple of the test fails including a small FAPI fix that might need to go back to D7.

The current code in FormBuilder::processForm() looks like the wrong/too late place for the check, since we it looks like we might call this earlier code in the method with #token being false

      if (!isset($input['form_token']) && isset($form['#token'])) {
        $input['form_token'] = $this->csrfToken->get($form['#token']);
      }

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -105,6 +105,27 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
   +  protected $safeCoreValueCallables = [
   

   I protested this going into D7 without an extensible fix, and was told that it would not go into D8 without one. In the interest of progress, I'll roll over once again...

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -521,11 +542,6 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
   -      // Form constructors may explicitly set #token to FALSE when cross site
   -      // request forgery is irrelevant to the form, such as search forms.
   -      if (isset($form['#token']) && $form['#token'] === FALSE) {
   -        unset($form['#token']);
   -      }
   
   @@ -648,25 +664,23 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   -    if ($user && $user->isAuthenticated() && !$form_state->isProgrammed()) {
   -      // Form constructors may explicitly set #token to FALSE when cross site
   -      // request forgery is irrelevant to the form, such as search forms.
   -      if (isset($form['#token']) && $form['#token'] === FALSE) {
   -        unset($form['#token']);
   -      }
   ...
   +    if ($form_state->isProgrammed() || (isset($form['#token']) && $form['#token'] === FALSE)) {
   +      unset($form['#token']);
   +    }
   

   Now that the prepareForm hunk is moved out of the $user->isAuthenticated hunk, this processForm code is unreachable. Removing it is not necessary to fix the bug, but should be done because it is now dead code. +1

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -817,6 +838,20 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
   +            // Set an early form error to block certain input processing since
   +            // that opens the door for CSRF vulnerabilities.
   +            $this->setInvalidTokenError($form_state);
   +
   +            // This value is checked in self::handleInputElement().
   +            $form_state->setInvalidToken(TRUE);
   

   The only call to setInvalidToken() is right after calling setInvalidTokenError().

   +++ b/core/lib/Drupal/Core/Form/FormValidator.php
   @@ -105,13 +105,12 @@ public function validateForm($form_id, &$form, FormStateInterface &$form_state)
   +      if (!$this->csrfToken->validate($form_state->getValue('form_token'), $form['#token']) || $form_state->hasInvalidToken()) {
   +        $this->setInvalidTokenError($form_state);
   

   Yet this second call to setInvalidTokenError() is right after a hasInvalidToken() check. I didn't dig into the code flow here, but that seems wrong somehow. As I said before, setInvalidToken() and setInvalidTokenError() have a very confusing relationship.

@timplunkett I think it's better to get it in with no extensibility than to rush the extensibility in. Gives us time to figure out between container parameter vs. interface etc. as a pure API addition. Once we add the extensibility we're stuck with it until 9.x.

I guess #29.3 just means two calls to setInvalidTokenError, and $form_state->setErrorByName already handles duplicate errors.

So from my perspective, this is RTBC.

Nope not RTBC. Explaining why in a follow-up comment, just wanted to prevent it from being committed.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -105,6 +105,27 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
   +  protected $safeCoreValueCallables = [
   +    '\Drupal\Core\Render\Element\Token::valueCallback',
   +    '\Drupal\Core\Render\Element\Textarea::valueCallback',
   +    '\Drupal\Core\Render\Element\Textfield::valueCallback',
   +    '\Drupal\Core\Render\Element\Checkbox::valueCallback',
   +    '\Drupal\Core\Render\Element\Checkboxes::valueCallback',
   +    '\Drupal\Core\Render\Element\Radios::valueCallback',
   +    '\Drupal\Core\Render\Element\PasswordConfirm::valueCallback',
   +    '\Drupal\Core\Render\Element\Select::valueCallback',
   +    '\Drupal\Core\Render\Element\Tableselect::valueCallback',
   +  ];
   

   We likely need to expand this list, we have a ton more elements.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -1077,7 +1112,14 @@ protected function handleInputElement($form_id, &$element, FormStateInterface &$
   +          if (!$form_state->hasInvalidToken() || in_array($value_callable, $this->safeCoreValueCallables)) {
   

   This in_array is not going to work anymore, since this callable can now be an array. In fact, any normal element is defined as array($definition['class'], 'valueCallback') right now.

This means the test coverage is inadequate.

Indeed - just tested this patch with a local proxy and tampering with the form token during the POST causes all the input text to be lost.

This fixes the basic value callback problem. I added one more valid one and made a list in comments of those I excluded. the date ones looked a little complex for my taste, including loading config and there have been past vulns in DateTime handling like: http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-0273

Still needs more tests. Possibly, new tests should be backported to 7.

Some start on more test coverage. Not getting radios to work, however.

They also seem not to work in manual testing, so possibly there is something extra going on in validation which is not reached.

Here's a unit test.

Should we make the method public? I find unit tests of internal methods a bit suspect.

Wanting to test a protected method is not a reason to make it public.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -817,6 +844,20 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
   +            // Make sure file uploads do not get processed.
   +            $_FILES = array();
   

   Should we call out to $request->files = new FileBag(); ?

2. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -105,6 +105,19 @@ class FormState implements FormStateInterface {
   +   * If set to TRUE the form will skip calling form element value callbacks,
   +   * except for a select list of callbacks provided by Drupal core that are
   +   * known to be safe.
   +   *
   +   * This property is uncacheable.
   +   *
   +   * @see self::setInvalidToken()
   +   *
   +   * @var bool
   +   */
   +  protected $invalidToken = FALSE;
   

   Does that mean we should implement serialize() / unserialize() ?

3. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -681,6 +682,47 @@ public function providerTestChildAccessInheritance() {
   +    $method = new \ReflectionMethod(FormBuilder::class, 'valueCallableIsSafe');
   +    $method->setAccessible(true);
   +    $is_safe = $method->invoke($this->formBuilder, $callback);
   +    $this->assertSame($expected, $is_safe);
   

   I think its kinda fine to test that.

Radio wasn't working because we didn't add it to the list.

Added 41.1
Not sure what you mean by 41.2, we document a lot of form_state properties like this, it's just to show that it's not in $form_state->getCacheableArray().

Not sure what you mean by 41.2, we document a lot of form_state properties like this, it's just to show that it's not in $form_state->getCacheableArray().

Ah I wasn't aware of it.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -856,6 +858,7 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
                $_FILES = array();
   

   Well, my intention was to remove the first line

2. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -723,6 +724,53 @@ function () {},
   +    $data[] = [FALSE, NULL, FALSE];
   +    $data[] = [FALSE, NULL, FALSE];
   

   This is twice the same test case

Discussed with @webchick, @alexpott, @effulgentsia, and @catch. As a forward-port for a security advisory, this issue is definitely critical.

Thank you for addressing my feedback.

Turns out 8 is more annoying than expected - basically everything is extending FormElement so has a valueCallback defined by inheritance. I think I added now most all of the things. Added a few more test cases.

Also, we should include the default Drupal\Core\Render\Element\FormElement::valueCallback - which does nothing, but returns NULL so the input is taken directly.

It feels like there could be another lingering security bug from #2380053: [core] Posting an array as value of a form element is allowed even when a string is expected (and bypasses #maxlength constraints) - first step: text fields

In 7 we have:

  $types['password'] = array(
    '#input' => TRUE,
    '#size' => 60,
    '#maxlength' => 128,
    '#process' => array('ajax_process_form'),
    '#theme' => 'password',
    '#theme_wrappers' => array('form_element'),
    // Use the same value callback as for textfields; this ensures that we only
    // get string values.
    '#value_callback' => 'form_type_textfield_value',
  );

But here in 8 the Password type just returns NULL and doesn't use the check from textfield, and textfield doesn't check the value is scalar.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -105,6 +106,45 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
      /**
   +   * Defines element value callables which are safe to run even when the form
   +   * state has an invalid CSRF token.
   +   *
   +   * Excluded from this list on purpose:
   +   *  - Drupal\file\Element\ManagedFile::valueCallback
   +   *  - Drupal\Core\Datetime\Element\Datelist::valueCallback
   +   *  - Drupal\Core\Datetime\Element\Datetime::valueCallback
   +   *  - Drupal\Core\Render\Element\ImageButton::valueCallback
   +   *  - Drupal\file\Plugin\Field\FieldWidget\FileWidget::value
   +   *  - color_palette_color_value
   +   *
   +   * @var array
   +   */
   

   Does that mean we should have a blacklist instead? It is really not clear for me given how many things we define for core, that we don't need extensibility ... just have a look at TableSelect ... this could be easily a custom thing

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -994,6 +1048,24 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
    
   ...
   +   * Helper function to normalize the different callable formats.
   +   */
   +  protected function valueCallableIsSafe($value_callable) {
   

   can we typehint with @callable? We also need docs

We really want a whitelist instead of a blacklist, but possibly we could do it via an added method (which would give immediate extensibility).

We really want a whitelist instead of a blacklist, but possibly we could do it via an added method (which would give immediate extensibility).

Well I guess this issue is about fixing core, but it feels odd to not provide a mechanism for contrib to deal with it.

@dawehner - see earlier comments. Since this is only about the UX case when the CSRF token is invalid, we decided to wait to see if any contrib is meaningfully impacted

So, tried looking at an added method, but that doesn't work since the FAPI array element doesn't have any reference to the corresponding class (and using a class is not required).

Since we do check is_callable() earlier in the FormBuilder, I think type hinting here is reasonable.

Thank you!

Adding credit.

Committed dcd5951 and pushed to 8.0.x. Thanks!

We need that followup for extensibility by contrib.