Commit dcd59513 authored by alexpott's avatar alexpott

Issue #2554233 by pwolanin, tim.plunkett, amateescu, dawehner, Wim Leers,...

Issue #2554233 by pwolanin, tim.plunkett, amateescu, dawehner, Wim Leers, catch, effulgentsia, larowlan, Abdullah Hussam, greggles, David_Rothstein: Port Cross-site Request Forgery - Form API fixes from SA-CORE-2015-003 to Drupal 8
parent f71a3759
......@@ -21,6 +21,7 @@
use Drupal\Core\Render\ElementInfoManagerInterface;
use Drupal\Core\Theme\ThemeManagerInterface;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use Symfony\Component\HttpFoundation\FileBag;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpFoundation\Response;
......@@ -104,6 +105,45 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
*/
protected $formCache;
/**
* 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
*/
protected $safeCoreValueCallables = [
'Drupal\Core\Render\Element\Checkbox::valueCallback',
'Drupal\Core\Render\Element\Checkboxes::valueCallback',
'Drupal\Core\Render\Element\Email::valueCallback',
'Drupal\Core\Render\Element\FormElement::valueCallback',
'Drupal\Core\Render\Element\MachineName::valueCallback',
'Drupal\Core\Render\Element\Number::valueCallback',
'Drupal\Core\Render\Element\PathElement::valueCallback',
'Drupal\Core\Render\Element\Password::valueCallback',
'Drupal\Core\Render\Element\PasswordConfirm::valueCallback',
'Drupal\Core\Render\Element\Radio::valueCallback',
'Drupal\Core\Render\Element\Radios::valueCallback',
'Drupal\Core\Render\Element\Range::valueCallback',
'Drupal\Core\Render\Element\Search::valueCallback',
'Drupal\Core\Render\Element\Select::valueCallback',
'Drupal\Core\Render\Element\Tableselect::valueCallback',
'Drupal\Core\Render\Element\Table::valueCallback',
'Drupal\Core\Render\Element\Tel::valueCallback',
'Drupal\Core\Render\Element\Textarea::valueCallback',
'Drupal\Core\Render\Element\Textfield::valueCallback',
'Drupal\Core\Render\Element\Token::valueCallback',
'Drupal\Core\Render\Element\Url::valueCallback',
'Drupal\Core\Render\Element\Weight::valueCallback',
];
/**
* Constructs a new FormBuilder.
*
......@@ -521,11 +561,6 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
// Only process the input if we have a correct form submission.
if ($form_state->isProcessingInput()) {
// 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']);
}
// Form values for programmed form submissions typically do not include a
// value for the submit button. But without a triggering element, a
// potentially existing #limit_validation_errors property on the primary
......@@ -648,25 +683,23 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
// since tokens are session-bound and forms displayed to anonymous users are
// very likely cached, we cannot assign a token for them.
// During installation, there is no $user yet.
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']);
}
// Otherwise, generate a public token based on the form id.
else {
$form['#token'] = $form_id;
$form['form_token'] = array(
'#id' => Html::getUniqueId('edit-' . $form_id . '-form-token'),
'#type' => 'token',
'#default_value' => $this->csrfToken->get($form['#token']),
// Form processing and validation requires this value, so ensure the
// submitted form value appears literally, regardless of custom #tree
// and #parents being set elsewhere.
'#parents' => array('form_token'),
);
}
// Form constructors may explicitly set #token to FALSE when cross site
// request forgery is irrelevant to the form, such as search forms.
if ($form_state->isProgrammed() || (isset($form['#token']) && $form['#token'] === FALSE)) {
unset($form['#token']);
}
elseif ($user && $user->isAuthenticated()) {
// Generate a public token based on the form id.
$form['#token'] = $form_id;
$form['form_token'] = array(
'#id' => Html::getUniqueId('edit-' . $form_id . '-form-token'),
'#type' => 'token',
'#default_value' => $this->csrfToken->get($form['#token']),
// Form processing and validation requires this value, so ensure the
// submitted form value appears literally, regardless of custom #tree
// and #parents being set elsewhere.
'#parents' => array('form_token'),
);
}
if (isset($form_id)) {
......@@ -740,6 +773,13 @@ protected function buildFormAction() {
return $parsed['path'] . ($parsed['query'] ? ('?' . UrlHelper::buildQuery($parsed['query'])) : '');
}
/**
* {@inheritdoc}
*/
public function setInvalidTokenError(FormStateInterface $form_state) {
$this->formValidator->setInvalidTokenError($form_state);
}
/**
* {@inheritdoc}
*/
......@@ -817,6 +857,20 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
$input = $form_state->getUserInput();
if ($form_state->isProgrammed() || (!empty($input) && (isset($input['form_id']) && ($input['form_id'] == $form_id)))) {
$form_state->setProcessInput();
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.
$this->setInvalidTokenError($form_state);
// This value is checked in self::handleInputElement().
$form_state->setInvalidToken(TRUE);
// Make sure file uploads do not get processed.
$this->requestStack->getCurrentRequest()->files = new FileBag();
}
}
}
else {
$form_state->setProcessInput(FALSE);
......@@ -993,6 +1047,31 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
return $element;
}
/**
* Helper function to normalize the different callable formats.
*
* @param callable $value_callable
* The callable to be checked.
*
* @return bool
* TRUE if the callable is safe even if the CSRF token is invalid, FALSE
* otherwise.
*/
protected function valueCallableIsSafe(callable $value_callable) {
// The same static class method callable may be formatted in two array and
// two string forms:
// ['\Classname', 'methodname']
// ['Classname', 'methodname']
// '\Classname::methodname'
// 'Classname::methodname'
if (is_callable($value_callable, FALSE, $callable_name)) {
// The third parameter of is_callable() is set to a string form, but we
// still have to normalize further by stripping a leading '\'.
return in_array(ltrim($callable_name, '\\'), $this->safeCoreValueCallables);
}
return FALSE;
}
/**
* Adds the #name and #value properties of an input element before rendering.
*/
......@@ -1082,7 +1161,14 @@ protected function handleInputElement($form_id, &$element, FormStateInterface &$
// If we have input for the current element, assign it to the #value
// property, optionally filtered through $value_callback.
if ($input_exists) {
$element['#value'] = call_user_func_array($value_callable, array(&$element, $input, &$form_state));
// Skip all value callbacks except safe ones like text if the CSRF
// token was invalid.
if (!$form_state->hasInvalidToken() || $this->valueCallableIsSafe($value_callable)) {
$element['#value'] = call_user_func_array($value_callable, array(&$element, $input, &$form_state));
}
else {
$input = NULL;
}
if (!isset($element['#value']) && isset($input)) {
$element['#value'] = $input;
......
......@@ -104,6 +104,19 @@ class FormState implements FormStateInterface {
*/
protected $rebuild = FALSE;
/**
* 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;
/**
* Used when a form needs to return some kind of a
* \Symfony\Component\HttpFoundation\Response object, e.g., a
......@@ -1283,6 +1296,21 @@ public function cleanValues() {
return $this;
}
/**
* {@inheritdoc}
*/
public function setInvalidToken($invalid_token) {
$this->invalidToken = (bool) $invalid_token;
return $this;
}
/**
* {@inheritdoc}
*/
public function hasInvalidToken() {
return $this->invalidToken;
}
/**
* Wraps ModuleHandler::loadInclude().
*/
......
......@@ -561,6 +561,24 @@ public function setRebuild($rebuild = TRUE);
*/
public function isRebuilding();
/**
* Flags the form state as having or not an invalid token.
*
* @param bool $invalid_token
* Whether the form has an invalid token.
*
* @return $this
*/
public function setInvalidToken($invalid_token);
/**
* Determines if the form has an invalid token.
*
* @return bool
* TRUE if the form has an invalid token, FALSE otherwise.
*/
public function hasInvalidToken();
/**
* Converts support notations for a form callback to a valid callable.
*
......
......@@ -105,13 +105,12 @@ public function validateForm($form_id, &$form, FormStateInterface &$form_state)
}
// If the session token was set by self::prepareForm(), ensure that it
// matches the current user's session.
// matches the current user's session. This is duplicate to code in
// FormBuilder::doBuildForm() but left to protect any custom form handling
// code.
if (isset($form['#token'])) {
if (!$this->csrfToken->validate($form_state->getValue('form_token'), $form['#token'])) {
$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)));
if (!$this->csrfToken->validate($form_state->getValue('form_token'), $form['#token']) || $form_state->hasInvalidToken()) {
$this->setInvalidTokenError($form_state);
// Stop here and don't run any further validation handlers, because they
// could invoke non-safe operations which opens the door for CSRF
......@@ -127,6 +126,16 @@ public function validateForm($form_id, &$form, FormStateInterface &$form_state)
$this->handleErrorsWithLimitedValidation($form, $form_state, $form_id);
}
/**
* {@inheritdoc}
*/
public function setInvalidTokenError(FormStateInterface $form_state) {
$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)));
}
/**
* Handles validation errors for forms with limited validation.
*
......
......@@ -54,4 +54,14 @@ public function executeValidateHandlers(&$form, FormStateInterface &$form_state)
*/
public function validateForm($form_id, &$form, FormStateInterface &$form_state);
/**
* Sets a form_token error on the given form state.
*
* @param \Drupal\Core\Form\FormStateInterface $form_state
* The current state of the form.
*
* @return $this
*/
public function setInvalidTokenError(FormStateInterface $form_state);
}
......@@ -37,6 +37,18 @@ function testManagedFile() {
$this->drupalPostForm($path, array(), t('Save'));
$this->assertRaw(t('The file ids are %fids.', array('%fids' => implode(',', array()))), 'Submitted without a file.');
// Submit with a file, but with an invalid form token. Ensure the file
// was not saved.
$last_fid_prior = $this->getLastFileId();
$edit = [
$file_field_name => drupal_realpath($test_file->getFileUri()),
'form_token' => 'invalid token',
];
$this->drupalPostForm($path, $edit, t('Save'));
$this->assertText('The form has become outdated. Copy any unsaved work in the form below');
$last_fid = $this->getLastFileId();
$this->assertEqual($last_fid_prior, $last_fid, 'File was not saved when uploaded with an invalid form token.');
// Submit a new file, without using the Upload button.
$last_fid_prior = $this->getLastFileId();
$edit = array($file_field_name => drupal_realpath($test_file->getFileUri()));
......
......@@ -12,6 +12,7 @@
use Drupal\Component\Utility\SafeMarkup;
use Drupal\Core\Form\FormState;
use Drupal\Core\Render\Element;
use Drupal\Core\Url;
use Drupal\form_test\Form\FormTestDisabledElementsForm;
use Drupal\simpletest\WebTestBase;
use Drupal\user\RoleInterface;
......@@ -232,6 +233,67 @@ function testRequiredCheckboxesRadio() {
$this->assertRaw("The form_test_validate_required_form form was submitted successfully.", 'Validation form submitted successfully.');
}
/**
* Tests that input is retained for safe elements even with an invalid token.
*
* Submits a test form containing several types of form elements.
*/
public function testInputWithInvalidToken() {
// We need to be logged in to have CSRF tokens.
$account = $this->createUser();
$this->drupalLogin($account);
// Submit again with required fields set but an invalid form token and
// verify that all the values are retained.
$edit = array(
'textfield' => $this->randomString(),
'checkboxes[bar]' => TRUE,
'select' => 'bar',
'radios' => 'foo',
'form_token' => 'invalid token',
);
$this->drupalPostForm(Url::fromRoute('form_test.validate_required'), $edit, 'Submit');
$this->assertFieldByXpath('//div[contains(@class, "error")]', NULL, 'Error message is displayed with invalid token even when required fields are filled.');
$this->assertText('The form has become outdated. Copy any unsaved work in the form below');
// Verify that input elements retained the posted values.
$this->assertFieldByName('textfield', $edit['textfield']);
$this->assertNoFieldChecked('edit-checkboxes-foo');
$this->assertFieldChecked('edit-checkboxes-bar');
$this->assertOptionSelected('edit-select', 'bar');
$this->assertFieldChecked('edit-radios-foo');
// Check another form that has a textarea input.
$edit = array(
'textfield' => $this->randomString(),
'textarea' => $this->randomString() . "\n",
'form_token' => 'invalid token',
);
$this->drupalPostForm(Url::fromRoute('form_test.required'), $edit, 'Submit');
$this->assertFieldByXpath('//div[contains(@class, "error")]', NULL, 'Error message is displayed with invalid token even when required fields are filled.');
$this->assertText('The form has become outdated. Copy any unsaved work in the form below');
$this->assertFieldByName('textfield', $edit['textfield']);
$this->assertFieldByName('textarea', $edit['textarea']);
// Check another form that has a number input.
$edit = array(
'integer_step' => mt_rand(1, 100),
'form_token' => 'invalid token',
);
$this->drupalPostForm(Url::fromRoute('form_test.number'), $edit, 'Submit');
$this->assertFieldByXpath('//div[contains(@class, "error")]', NULL, 'Error message is displayed with invalid token even when required fields are filled.');
$this->assertText('The form has become outdated. Copy any unsaved work in the form below');
$this->assertFieldByName('integer_step', $edit['integer_step']);
// Check a form with a Url field
$edit = array(
'url' => $this->randomString(),
'form_token' => 'invalid token',
);
$this->drupalPostForm(Url::fromRoute('form_test.url'), $edit, 'Submit');
$this->assertFieldByXpath('//div[contains(@class, "error")]', NULL, 'Error message is displayed with invalid token even when required fields are filled.');
$this->assertText('The form has become outdated. Copy any unsaved work in the form below');
$this->assertFieldByName('url', $edit['url']);
}
/**
* Tests validation for required textfield element without title.
*
......
......@@ -33,7 +33,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
'#title' => $type,
);
}
$form['submit'] = array(
'#type' => 'submit',
'#value' => 'Submit',
);
return $form;
}
......
......@@ -12,10 +12,12 @@
use Drupal\Core\Access\AccessResultForbidden;
use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Drupal\Core\Form\EnforcedResponseException;
use Drupal\Core\Form\FormBuilder;
use Drupal\Core\Form\FormBuilderInterface;
use Drupal\Core\Form\FormInterface;
use Drupal\Core\Form\FormState;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Session\AccountInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
......@@ -681,6 +683,90 @@ public function providerTestChildAccessInheritance() {
return $data;
}
/**
* @covers ::valueCallableIsSafe
*
* @dataProvider providerTestValueCallableIsSafe
*/
public function testValueCallableIsSafe($callback, $expected) {
$method = new \ReflectionMethod(FormBuilder::class, 'valueCallableIsSafe');
$method->setAccessible(true);
$is_safe = $method->invoke($this->formBuilder, $callback);
$this->assertSame($expected, $is_safe);
}
public function providerTestValueCallableIsSafe() {
$data = [];
$data['string_no_slash'] = [
'Drupal\Core\Render\Element\Token::valueCallback',
TRUE,
];
$data['string_with_slash'] = [
'\Drupal\Core\Render\Element\Token::valueCallback',
TRUE,
];
$data['array_no_slash'] = [
['Drupal\Core\Render\Element\Token', 'valueCallback'],
TRUE,
];
$data['array_with_slash'] = [
['\Drupal\Core\Render\Element\Token', 'valueCallback'],
TRUE,
];
$data['closure'] = [
function () {},
FALSE,
];
return $data;
}
/**
* @covers ::doBuildForm
*
* @dataProvider providerTestInvalidToken
*/
public function testInvalidToken($expected, $valid_token, $user_is_authenticated) {
$form_token = 'the_form_token';
$form_id = 'test_form_id';
if (is_bool($valid_token)) {
$this->csrfToken->expects($this->any())
->method('get')
->willReturnArgument(0);
$this->csrfToken->expects($this->atLeastOnce())
->method('validate')
->will($this->returnValueMap([
[$form_token, $form_id, $valid_token],
[$form_id, $form_id, $valid_token],
]));
}
$current_user = $this->prophesize(AccountInterface::class);
$current_user->isAuthenticated()->willReturn($user_is_authenticated);
$property = new \ReflectionProperty(FormBuilder::class, 'currentUser');
$property->setAccessible(TRUE);
$property->setValue($this->formBuilder, $current_user->reveal());
$expected_form = $form_id();
$form_arg = $this->getMockForm($form_id, $expected_form);
$form_state = new FormState();
$input['form_id'] = $form_id;
$input['form_token'] = $form_token;
$form_state->setUserInput($input);
$this->simulateFormSubmission($form_id, $form_arg, $form_state, FALSE);
$this->assertSame($expected, $form_state->hasInvalidToken());
}
public function providerTestInvalidToken() {
$data = [];
$data['authenticated_invalid'] = [TRUE, FALSE, TRUE];
$data['authenticated_valid'] = [FALSE, TRUE, TRUE];
// If the user is not authenticated, we will not have a token.
$data['anonymous'] = [FALSE, NULL, FALSE];
return $data;
}
}
class TestForm implements FormInterface {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment