SA-CORE-2017-003 by Berdir, David_Rothstein, Wim Leers, alexpott, catch, cilefen, larowlan, mlhess, pwolanin, quicksketch, samuel.mortenson, smaz, stefan.r, xjm

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.

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');
+  }
+
+}

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');
   }
 
   /**

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.

core/modules/file/src/FileAccessControlHandler.php

@@ -40,10 +40,22 @@ 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.
+        // 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();
+        }
+      }
+    }
 
     if ($operation == 'delete' || $operation == 'update') {
       $account = $this->prepareUser($account);
@@ -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
+    // Deny access to fields that should only be set on file creation, and
-    // persistent before even validating it.
+    // "status" which should only be changed based on a file's usage.
-    if ($field_definition->getName() === 'status' && $operation === 'edit') {
+    $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);

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.');
   }
 
 }

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'));
   }
 
   /**

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

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;
 
 /**
@@ -43,6 +44,13 @@ abstract class MediaKernelTestBase extends KernelTestBase {
    */
   protected $testConstraintsMediaType;
 
+  /**
+   * A user.
+   *
+   * @var \Drupal\user\UserInterface
+   */
+  protected $user;
+
   /**
    * {@inheritdoc}
    */
@@ -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();

core/tests/Drupal/Tests/Component/Serialization/YamlPeclTest.php

@@ -26,6 +26,16 @@ class YamlPeclTest extends YamlTestBase {
     $this->assertEquals($data, YamlPecl::decode(YamlPecl::encode($data)));
   }
 
+  /**
+   * 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.
    *

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]);
+  }
+
 }

core/tests/Drupal/Tests/Component/Serialization/YamlTest.php

@@ -77,6 +77,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.
    */