summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorChris McCafferty2017-06-21 16:11:36 -0400
committerChris McCafferty2017-06-21 16:11:36 -0400
commitc732355412b84a6f7079d92b4029cd9400c56c50 (patch)
treef5c44998d3f7d72eec7a4ff309e4d76d9d9569ec
parente96b787dc4e411408cddfd575ee0e1d10e26f136 (diff)
SA-CORE-2017-003 by Berdir, David_Rothstein, Wim Leers, alexpott, catch, cilefen, larowlan, mlhess, pwolanin, quicksketch, samuel.mortenson, smaz, stefan.r, xjm
-rw-r--r--core/lib/Drupal/Component/Serialization/YamlPecl.php2
-rw-r--r--core/lib/Drupal/Core/Test/ObjectSerialization.php24
-rw-r--r--core/modules/config/src/Tests/ConfigSingleImportExportTest.php23
-rw-r--r--core/modules/file/file.module8
-rw-r--r--core/modules/file/src/FileAccessControlHandler.php37
-rw-r--r--core/modules/file/src/Tests/FilePrivateTest.php113
-rw-r--r--core/modules/file/tests/src/Kernel/AccessTest.php25
-rw-r--r--core/modules/file/tests/src/Kernel/FileItemValidationTest.php2
-rw-r--r--core/modules/media/tests/src/Kernel/MediaKernelTestBase.php17
-rw-r--r--core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php10
-rw-r--r--core/tests/Drupal/Tests/Component/Serialization/YamlSymfonyTest.php12
-rw-r--r--core/tests/Drupal/Tests/Component/Serialization/YamlTest.php17
12 files changed, 282 insertions, 8 deletions
diff --git a/core/lib/Drupal/Component/Serialization/YamlPecl.php b/core/lib/Drupal/Component/Serialization/YamlPecl.php
index ec48a29..a01f980 100644
--- a/core/lib/Drupal/Component/Serialization/YamlPecl.php
+++ b/core/lib/Drupal/Component/Serialization/YamlPecl.php
@@ -32,6 +32,8 @@ class YamlPecl implements SerializationInterface {
// Decode binary, since Symfony YAML parser encodes binary from 3.1
// onwards.
ini_set('yaml.decode_binary', 1);
+ // We never want to unserialize !php/object.
+ ini_set('yaml.decode_php', 0);
$init = TRUE;
}
// yaml_parse() will error with an empty value.
diff --git a/core/lib/Drupal/Core/Test/ObjectSerialization.php b/core/lib/Drupal/Core/Test/ObjectSerialization.php
new file mode 100644
index 0000000..99d44a5
--- /dev/null
+++ b/core/lib/Drupal/Core/Test/ObjectSerialization.php
@@ -0,0 +1,24 @@
+<?php
+
+namespace Drupal\Core\Test;
+
+/**
+ * Object to test that security issues around serialization.
+ */
+class ObjectSerialization {
+
+ /**
+ * ObjectSerialization constructor.
+ */
+ public function __construct() {
+ throw new \Exception('This object should never be constructed');
+ }
+
+ /**
+ * ObjectSerialization deconstructor.
+ */
+ public function __destruct() {
+ throw new \Exception('This object should never be destructed');
+ }
+
+}
diff --git a/core/modules/config/src/Tests/ConfigSingleImportExportTest.php b/core/modules/config/src/Tests/ConfigSingleImportExportTest.php
index 16cff0a..607e80c 100644
--- a/core/modules/config/src/Tests/ConfigSingleImportExportTest.php
+++ b/core/modules/config/src/Tests/ConfigSingleImportExportTest.php
@@ -155,6 +155,29 @@ EOD;
];
$this->drupalPostForm('admin/config/development/configuration/single/import', $edit, t('Import'));
$this->assertRaw(t('Configuration %name depends on the %owner module that will not be installed after import.', ['%name' => 'config_test.dynamic.second', '%owner' => 'does_not_exist']));
+
+ // Try to preform an update which would create a PHP object if Yaml parsing
+ // not securely set up.
+ // Perform an update.
+ $import = <<<EOD
+id: second
+uuid: $second_uuid
+label: !php/object "O:36:\"Drupal\\\Core\\\Test\\\ObjectSerialization\":0:{}"
+weight: 0
+style: ''
+status: '0'
+EOD;
+ $edit = [
+ 'config_type' => 'config_test',
+ 'import' => $import,
+ ];
+ $this->drupalPostForm('admin/config/development/configuration/single/import', $edit, t('Import'));
+ $this->assertRaw(t('Are you sure you want to update the %name @type?', ['%name' => 'second', '@type' => 'test configuration']));
+ $this->drupalPostForm(NULL, [], t('Confirm'));
+ $entity = $storage->load('second');
+ $this->assertRaw(t('The configuration was imported successfully.'));
+ $this->assertTrue(is_string($entity->label()), 'Entity label is a string');
+ $this->assertTrue(strpos($entity->label(), 'ObjectSerialization') > 0, 'Label contains serialized object');
}
/**
diff --git a/core/modules/file/file.module b/core/modules/file/file.module
index a6fc3f6..6902db6 100644
--- a/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -904,6 +904,14 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
// If we made it this far it's safe to record this file in the database.
$file->save();
$files[$i] = $file;
+ // Allow an anonymous user who creates a non-public file to see it. See
+ // \Drupal\file\FileAccessControlHandler::checkAccess().
+ if ($user->isAnonymous() && $destination_scheme !== 'public') {
+ $session = \Drupal::request()->getSession();
+ $allowed_temp_files = $session->get('anonymous_allowed_file_ids', []);
+ $allowed_temp_files[$file->id()] = $file->id();
+ $session->set('anonymous_allowed_file_ids', $allowed_temp_files);
+ }
}
// Add files to the cache.
diff --git a/core/modules/file/src/FileAccessControlHandler.php b/core/modules/file/src/FileAccessControlHandler.php
index a82c460..f4bda3c 100644
--- a/core/modules/file/src/FileAccessControlHandler.php
+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -40,8 +40,20 @@ class FileAccessControlHandler extends EntityAccessControlHandler {
}
elseif ($entity->getOwnerId() == $account->id()) {
// This case handles new nodes, or detached files. The user who uploaded
- // the file can always access if it's not yet used.
- return AccessResult::allowed();
+ // the file can access it even if it's not yet used.
+ if ($account->isAnonymous()) {
+ // For anonymous users, only the browser session that uploaded the
+ // file is positively allowed access to it. See file_save_upload().
+ // @todo Implement \Drupal\Core\Entity\EntityHandlerInterface so that
+ // services can be more properly injected.
+ $allowed_fids = \Drupal::service('session')->get('anonymous_allowed_file_ids', []);
+ if (!empty($allowed_fids[$entity->id()])) {
+ return AccessResult::allowed();
+ }
+ }
+ else {
+ return AccessResult::allowed();
+ }
}
}
@@ -79,9 +91,24 @@ class FileAccessControlHandler extends EntityAccessControlHandler {
* {@inheritdoc}
*/
protected function checkFieldAccess($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
- // No user can edit the status of a file. Prevents saving a new file as
- // persistent before even validating it.
- if ($field_definition->getName() === 'status' && $operation === 'edit') {
+ // Deny access to fields that should only be set on file creation, and
+ // "status" which should only be changed based on a file's usage.
+ $create_only_fields = [
+ 'uri',
+ 'filemime',
+ 'filesize',
+ ];
+ // The operation is 'edit' when the entity is being created or updated.
+ // Determine if the entity is being updated by checking if it is new.
+ $field_name = $field_definition->getName();
+ if ($operation === 'edit' && $items && ($entity = $items->getEntity()) && !$entity->isNew() && in_array($field_name, $create_only_fields, TRUE)) {
+ return AccessResult::forbidden();
+ }
+ // Regardless of whether the entity exists access should be denied to the
+ // status field as this is managed via other APIs, for example:
+ // - \Drupal\file\FileUsage\FileUsageBase::add()
+ // - \Drupal\file\Plugin\EntityReferenceSelection\FileSelection::createNewEntity()
+ if ($operation === 'edit' && $field_name === 'status') {
return AccessResult::forbidden();
}
return parent::checkFieldAccess($operation, $field_definition, $account, $items);
diff --git a/core/modules/file/src/Tests/FilePrivateTest.php b/core/modules/file/src/Tests/FilePrivateTest.php
index 7ecd485..6808a47 100644
--- a/core/modules/file/src/Tests/FilePrivateTest.php
+++ b/core/modules/file/src/Tests/FilePrivateTest.php
@@ -6,6 +6,7 @@ use Drupal\Core\Entity\Plugin\Validation\Constraint\ReferenceAccessConstraint;
use Drupal\Component\Utility\SafeMarkup;
use Drupal\file\Entity\File;
use Drupal\node\Entity\NodeType;
+use Drupal\user\RoleInterface;
/**
* Uploads a test to a private node and checks access.
@@ -110,6 +111,118 @@ class FilePrivateTest extends FileFieldTestBase {
$this->drupalLogin($account);
$this->drupalGet($file_url);
$this->assertResponse(403, 'Confirmed that access is denied for another user to the temporary file.');
+
+ // As an anonymous user, create a temporary file with no references and
+ // confirm that only the session that uploaded it may view it.
+ $this->drupalLogout();
+ user_role_change_permissions(
+ RoleInterface::ANONYMOUS_ID,
+ [
+ "create $type_name content" => TRUE,
+ 'access content' => TRUE,
+ ]
+ );
+ $test_file = $this->getTestFile('text');
+ $this->drupalGet('node/add/' . $type_name);
+ $edit = ['files[' . $field_name . '_0]' => drupal_realpath($test_file->getFileUri())];
+ $this->drupalPostForm(NULL, $edit, t('Upload'));
+ /** @var \Drupal\file\FileStorageInterface $file_storage */
+ $file_storage = $this->container->get('entity.manager')->getStorage('file');
+ $files = $file_storage->loadByProperties(['uid' => 0]);
+ $this->assertEqual(1, count($files), 'Loaded one anonymous file.');
+ $file = end($files);
+ $this->assertTrue($file->isTemporary(), 'File is temporary.');
+ $usage = $this->container->get('file.usage')->listUsage($file);
+ $this->assertFalse($usage, 'No file usage found.');
+ $file_url = file_create_url($file->getFileUri());
+ $this->drupalGet($file_url);
+ $this->assertResponse(200, 'Confirmed that the anonymous uploader has access to the temporary file.');
+ // Close the prior connection and remove the session cookie.
+ $this->curlClose();
+ $this->curlCookies = [];
+ $this->cookies = [];
+ $this->drupalGet($file_url);
+ $this->assertResponse(403, 'Confirmed that another anonymous user cannot access the temporary file.');
+
+ // As an anonymous user, create a permanent file, then remove all
+ // references to the file (so that it becomes temporary again) and confirm
+ // that only the session that uploaded it may view it.
+ $test_file = $this->getTestFile('text');
+ $this->drupalGet('node/add/' . $type_name);
+ $edit = [];
+ $edit['title[0][value]'] = $this->randomMachineName();
+ $edit['files[' . $field_name . '_0]'] = drupal_realpath($test_file->getFileUri());
+ $this->drupalPostForm(NULL, $edit, t('Save'));
+ $new_node = $this->drupalGetNodeByTitle($edit['title[0][value]']);
+ $file_id = $new_node->{$field_name}->target_id;
+ $file = File::load($file_id);
+ $this->assertTrue($file->isPermanent(), 'File is permanent.');
+ // Remove the reference to this file.
+ $new_node->{$field_name} = [];
+ $new_node->save();
+ $file = File::load($file_id);
+ $this->assertTrue($file->isTemporary(), 'File is temporary.');
+ $usage = $this->container->get('file.usage')->listUsage($file);
+ $this->assertFalse($usage, 'No file usage found.');
+ $file_url = file_create_url($file->getFileUri());
+ $this->drupalGet($file_url);
+ $this->assertResponse(200, 'Confirmed that the anonymous uploader has access to the file whose references were removed.');
+ // Close the prior connection and remove the session cookie.
+ $this->curlClose();
+ $this->curlCookies = [];
+ $this->cookies = [];
+ $this->drupalGet($file_url);
+ $this->assertResponse(403, 'Confirmed that another anonymous user cannot access the file whose references were removed.');
+
+ // As an anonymous user, create a permanent file that is referenced by a
+ // published node and confirm that all anonymous users may view it.
+ $test_file = $this->getTestFile('text');
+ $this->drupalGet('node/add/' . $type_name);
+ $edit = [];
+ $edit['title[0][value]'] = $this->randomMachineName();
+ $edit['files[' . $field_name . '_0]'] = drupal_realpath($test_file->getFileUri());
+ $this->drupalPostForm(NULL, $edit, t('Save'));
+ $new_node = $this->drupalGetNodeByTitle($edit['title[0][value]']);
+ $file = File::load($new_node->{$field_name}->target_id);
+ $this->assertTrue($file->isPermanent(), 'File is permanent.');
+ $usage = $this->container->get('file.usage')->listUsage($file);
+ $this->assertTrue($usage, 'File usage found.');
+ $file_url = file_create_url($file->getFileUri());
+ $this->drupalGet($file_url);
+ $this->assertResponse(200, 'Confirmed that the anonymous uploader has access to the permanent file that is referenced by a published node.');
+ // Close the prior connection and remove the session cookie.
+ $this->curlClose();
+ $this->curlCookies = [];
+ $this->cookies = [];
+ $this->drupalGet($file_url);
+ $this->assertResponse(200, 'Confirmed that another anonymous user also has access to the permanent file that is referenced by a published node.');
+
+ // As an anonymous user, create a permanent file that is referenced by an
+ // unpublished node and confirm that no anonymous users may view it (even
+ // the session that uploaded the file) because they cannot view the
+ // unpublished node.
+ $test_file = $this->getTestFile('text');
+ $this->drupalGet('node/add/' . $type_name);
+ $edit = [];
+ $edit['title[0][value]'] = $this->randomMachineName();
+ $edit['files[' . $field_name . '_0]'] = drupal_realpath($test_file->getFileUri());
+ $this->drupalPostForm(NULL, $edit, t('Save'));
+ $new_node = $this->drupalGetNodeByTitle($edit['title[0][value]']);
+ $new_node->setPublished(FALSE);
+ $new_node->save();
+ $file = File::load($new_node->{$field_name}->target_id);
+ $this->assertTrue($file->isPermanent(), 'File is permanent.');
+ $usage = $this->container->get('file.usage')->listUsage($file);
+ $this->assertTrue($usage, 'File usage found.');
+ $file_url = file_create_url($file->getFileUri());
+ $this->drupalGet($file_url);
+ $this->assertResponse(403, 'Confirmed that the anonymous uploader cannot access the permanent file when it is referenced by an unpublished node.');
+ // Close the prior connection and remove the session cookie.
+ $this->curlClose();
+ $this->curlCookies = [];
+ $this->cookies = [];
+ $this->drupalGet($file_url);
+ $this->assertResponse(403, 'Confirmed that another anonymous user cannot access the permanent file when it is referenced by an unpublished node.');
}
}
diff --git a/core/modules/file/tests/src/Kernel/AccessTest.php b/core/modules/file/tests/src/Kernel/AccessTest.php
index c71d7e8..cfd8e56 100644
--- a/core/modules/file/tests/src/Kernel/AccessTest.php
+++ b/core/modules/file/tests/src/Kernel/AccessTest.php
@@ -85,11 +85,30 @@ class AccessTest extends KernelTestBase {
}
/**
- * Tests that the status field is not editable.
+ * Tests file entity field access.
+ *
+ * @see \Drupal\file\FileAccessControlHandler::checkFieldAccess()
*/
- public function testStatusFieldIsNotEditable() {
+ public function testCheckFieldAccess() {
\Drupal::currentUser()->setAccount($this->user1);
- $this->assertFalse($this->file->get('status')->access('edit'));
+ /** @var \Drupal\file\FileInterface $file */
+ $file = File::create([
+ 'uri' => 'public://test.png'
+ ]);
+ // While creating a file entity access will be allowed for create-only
+ // fields.
+ $this->assertTrue($file->get('uri')->access('edit'));
+ $this->assertTrue($file->get('filemime')->access('edit'));
+ $this->assertTrue($file->get('filesize')->access('edit'));
+ // Access to the status field is denied whilst creating a file entity.
+ $this->assertFalse($file->get('status')->access('edit'));
+ $file->save();
+ // After saving the entity is no longer new and, therefore, access to
+ // create-only fields and the status field will be denied.
+ $this->assertFalse($file->get('uri')->access('edit'));
+ $this->assertFalse($file->get('filemime')->access('edit'));
+ $this->assertFalse($file->get('filesize')->access('edit'));
+ $this->assertFalse($file->get('status')->access('edit'));
}
/**
diff --git a/core/modules/file/tests/src/Kernel/FileItemValidationTest.php b/core/modules/file/tests/src/Kernel/FileItemValidationTest.php
index a5c2d69..c9a247b 100644
--- a/core/modules/file/tests/src/Kernel/FileItemValidationTest.php
+++ b/core/modules/file/tests/src/Kernel/FileItemValidationTest.php
@@ -45,6 +45,7 @@ class FileItemValidationTest extends KernelTestBase {
'status' => 1,
]);
$this->user->save();
+ $this->container->get('current_user')->setAccount($this->user);
}
/**
@@ -85,6 +86,7 @@ class FileItemValidationTest extends KernelTestBase {
// Test for max filesize.
$file = File::create([
'uri' => 'vfs://drupal_root/sites/default/files/test.txt',
+ 'uid' => $this->user->id(),
]);
$file->setPermanent();
$file->save();
diff --git a/core/modules/media/tests/src/Kernel/MediaKernelTestBase.php b/core/modules/media/tests/src/Kernel/MediaKernelTestBase.php
index 5d75bed..fa39962 100644
--- a/core/modules/media/tests/src/Kernel/MediaKernelTestBase.php
+++ b/core/modules/media/tests/src/Kernel/MediaKernelTestBase.php
@@ -7,6 +7,7 @@ use Drupal\KernelTests\KernelTestBase;
use Drupal\media\Entity\Media;
use Drupal\media\Entity\MediaType;
use Drupal\media\MediaTypeInterface;
+use Drupal\user\Entity\User;
use org\bovigo\vfs\vfsStream;
/**
@@ -44,6 +45,13 @@ abstract class MediaKernelTestBase extends KernelTestBase {
protected $testConstraintsMediaType;
/**
+ * A user.
+ *
+ * @var \Drupal\user\UserInterface
+ */
+ protected $user;
+
+ /**
* {@inheritdoc}
*/
protected function setUp() {
@@ -52,6 +60,7 @@ abstract class MediaKernelTestBase extends KernelTestBase {
$this->installEntitySchema('user');
$this->installEntitySchema('file');
$this->installSchema('file', 'file_usage');
+ $this->installSchema('system', 'sequences');
$this->installEntitySchema('media');
$this->installConfig(['field', 'system', 'image', 'file', 'media']);
@@ -59,6 +68,13 @@ abstract class MediaKernelTestBase extends KernelTestBase {
$this->testMediaType = $this->createMediaType('test');
// Create a test media type with constraints.
$this->testConstraintsMediaType = $this->createMediaType('test_constraints');
+
+ $this->user = User::create([
+ 'name' => 'username',
+ 'status' => 1,
+ ]);
+ $this->user->save();
+ $this->container->get('current_user')->setAccount($this->user);
}
/**
@@ -117,6 +133,7 @@ abstract class MediaKernelTestBase extends KernelTestBase {
$file = File::create([
'uri' => 'vfs://drupal_root/sites/default/files/' . $filename,
+ 'uid' => $this->user->id(),
]);
$file->setPermanent();
$file->save();
diff --git a/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php b/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
index 3884031..c2e0a0d 100644
--- a/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
+++ b/core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php
@@ -27,6 +27,16 @@ class YamlPeclTest extends YamlTestBase {
}
/**
+ * Ensures that php object support is disabled.
+ */
+ public function testObjectSupportDisabled() {
+ $object = new \stdClass();
+ $object->foo = 'bar';
+ $this->assertEquals(['O:8:"stdClass":1:{s:3:"foo";s:3:"bar";}'], YamlPecl::decode(YamlPecl::encode([$object])));
+ $this->assertEquals(0, ini_get('yaml.decode_php'));
+ }
+
+ /**
* Tests decoding YAML node anchors.
*
* @covers ::decode
diff --git a/core/tests/Drupal/Tests/Component/Serialization/YamlSymfonyTest.php b/core/tests/Drupal/Tests/Component/Serialization/YamlSymfonyTest.php
index f9c2fb9..86c818c 100644
--- a/core/tests/Drupal/Tests/Component/Serialization/YamlSymfonyTest.php
+++ b/core/tests/Drupal/Tests/Component/Serialization/YamlSymfonyTest.php
@@ -63,4 +63,16 @@ class YamlSymfonyTest extends YamlTestBase {
YamlSymfony::decode('foo: [ads');
}
+ /**
+ * Ensures that php object support is disabled.
+ *
+ * @covers ::encode
+ */
+ public function testObjectSupportDisabled() {
+ $this->setExpectedException(InvalidDataTypeException::class, 'Object support when dumping a YAML file has been disabled.');
+ $object = new \stdClass();
+ $object->foo = 'bar';
+ YamlSymfony::encode([$object]);
+ }
+
}
diff --git a/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
index 841ac05..cee0a94 100644
--- a/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
+++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
@@ -78,6 +78,23 @@ class YamlTest extends UnitTestCase {
}
/**
+ * Ensures that decoding php objects is similar for PECL and Symfony.
+ *
+ * @requires extension yaml
+ */
+ public function testObjectSupportDisabled() {
+ $object = new \stdClass();
+ $object->foo = 'bar';
+ // In core all Yaml encoding is done via Symfony and it does not support
+ // objects so in order to encode an object we hace to use the PECL
+ // extension.
+ // @see \Drupal\Component\Serialization\Yaml::encode()
+ $yaml = YamlPecl::encode([$object]);
+ $this->assertEquals(['O:8:"stdClass":1:{s:3:"foo";s:3:"bar";}'], YamlPecl::decode($yaml));
+ $this->assertEquals(['!php/object "O:8:\"stdClass\":1:{s:3:\"foo\";s:3:\"bar\";}"'], YamlSymfony::decode($yaml));
+ }
+
+ /**
* Data provider that lists all YAML files in core.
*/
public function providerYamlFilesInCore() {