Actually there also may be other classes implementing Serializable interface. The \Drupal\views\ViewExecutable is just the first known one to cause the trouble.

Problem/Motivation

There is a PHP bug in the unserialize() function - https://bugs.php.net/bug.php?id=66052 . Precisely in nested unserialization.

When one serialized string contains object which implements Serializable interface, and this object calls another unserialize() with string containing references (serialized as r:NUMBER;), these references are substituted by values from the first string upon unserialization, not from the current one. This behavior leads to corrupted data in runtime and to fatal errors in some cases, and that data may accidentally be saved to persistent storage if not validated explicitly.

The ViewExecutable::execute() method is known to call unserialize() during its job. Examples of Fatals caused by this nested unserialization: #2750463: Fatal error: Call to a member function getFieldStorageDefinition() on array, #2701829: Cannot set batches in Views preventing port of VBO. There may be more. (Other code from ViewExecutable::unserialize() was not tested by me.)

A simple test case, which can be run in /devel/php form on any D8 dev site using the content view (/admin/content):

$field_manager = \Drupal::service('entity_field.manager');

// Get the definition of node's nid field, for example. Only get it not from 
// the field manager directly, but from the item data definition. It should be
// the same base field definition object (the field and item definitions refer
// to each other).
$nid_definition = $field_manager->getBaseFieldDefinitions('node')['nid']
  ->getItemDefinition()
  ->getFieldDefinition();
dpm($nid_definition, 'Expected:');

// Load and execute the view.
$view_entity = \Drupal\views\Entity\View::load('content');
$view = \Drupal::service('views.executable')->get($view_entity);
$view->execute('page_1');

// Reset static cache...
$field_manager->useCaches(FALSE);
// ... but leave possibility to use database cache.
$field_manager->useCaches(TRUE);

// Magic line.
unserialize(serialize(['SOMETHING UNEXPECTED', $view]));

// The same code as in the beginning of the test.
$nid_definition = $field_manager->getBaseFieldDefinitions('node')['nid']
  ->getItemDefinition()
  ->getFieldDefinition();
dpm($nid_definition, 'After nested unserialization:');

Proposed resolution

As the bug is still not fixed in PHP, we need to provide conditions to avoid its appearance in Drupal.

I propose to implement some kind of lazy execution in ViewExecutable, if that possible. Ideally, unserialize() method should simply assign values to object properties.

For impatient I'm attaching a patch with a very rough workaround for a limited use case - it covers only ViewExecutable class and the Database cache backend. But it may slow down your project, so don't use it on production.

Remaining tasks

  • Examine other classes with methods unserialize() and __wakeup().

Comments

pingwin4eg created an issue. See original summary.

mxh’s picture

As already written in https://www.drupal.org/node/2750463#comment-12081985, I suspect that the "trigger" of the problem is calling the serialize() function inside the ViewExecutable::serialize() method.

Attaching a patch which replaces the Serializable implementation by an alternative using magic methods.
By this, I also removed the usage of DependencySerializationTrait, because it seems that it had no use anyway (__sleep() and __wakeup() aren't used anymore when a class implements the \Serializable interface). I haven't found any other point where this might be relevant, thus removed.

mxh’s picture

Status: Active » Needs review
axel.rutz’s picture

Code looks straightforward to me.

By this, I also removed the usage of DependencySerializationTrait, because it seems that it had no use anyway (__sleep() and __wakeup() aren't used anymore when a class implements the \Serializable interface). I haven't found any other point where this might be relevant, thus removed.

OK storage ID is serialized, but what about the other injected services (routeProvider) and data (user, viewsData)? My gut feeling is, add viewsData and the IDs of user and routeProvider.

  public function __construct(ViewEntityInterface $storage, AccountInterface $user, ViewsData $views_data, RouteProviderInterface $route_provider) {
    // Reference the storage and the executable to each other.
    $this->storage = $storage;
    $this->storage->set('executable', $this);
    $this->user = $user;
    $this->viewsData = $views_data;
    $this->routeProvider = $route_provider;
  }
mxh’s picture

Status: Needs review » Needs work

OK storage ID is serialized, but what about the other injected services (routeProvider) and data (user, viewsData)? My gut feeling is, add viewsData and the IDs of user and routeProvider.

Yes, while __wakeup() already loads the user data, it's still missing the views.views_data and router.route_provider service. Seems that this was already broken before. Writing new patch...

mxh’s picture

Now attaching all necessary services on the executable, plus added circular referencing from storage to executable just like in the constructor method.

Needs some manual serialization tests for proper reviewing.

axel.rutz’s picture

+++ b/core/modules/views/src/ViewExecutable.php
@@ -2466,52 +2475,64 @@ public function getDependencies() {
       $this->user = \Drupal::currentUser();
+      $this->viewsData = \Drupal::service('views.views_data');
+      $this->routeProvider = \Drupal::service('router.route_provider');
 

Can we thoroughly assume these objects can not change? Use cases for this might sound esoteric but what can be thought will be done... Storing the IDs imho does not cost us much, are are there disadvantages?

axel.rutz’s picture

Status: Needs review » Needs work
mxh’s picture

Agree, except for the current user object. Since the executable makes some access checks on it, it should be ensured that the object actually represents the current user.

Manuel Garcia’s picture

Priority: Normal » Major
axel.rutz’s picture

Status: Needs review » Needs work

Agree, except for the current user object. Since the executable makes some access checks on it, it should be ensured that the object actually represents the current user.

Imho to the contrary. There are legitimate use cases where the view user is *not* the current user. I remember working on a D7 commerce issue where the payment gateway made a callback, bootstrapped as anonymous user, and had to execute a view for the transaction's user. That's an ideal case for executing a serialized view for another user with their access context. What do you think?

EDIT: And another viewpoint: If we don't respect the original user, the user property of the view does not make any sense and it should simply use the global current user.

mxh’s picture

Disagree with the following arguments:

  • The whole executable object expects the user property to be the current user. Other objects which use the executable object would adopt this expectation. Exceptional use cases should handle the change of this behavior on their own.
  • Assuming that the user (id) property would be serialized and unserialized: A user who is able to use unserialized view executables, which have been serialized by other users, might then bypass access restrictions.

EDIT: And another viewpoint: If we don't respect the original user, the user property of the view does not make any sense and it should simply use the global current user.

It does make sense, since it's there to let other objects being able to use the object, without the need of dependency injection or global container usage.

axel.rutz’s picture

Hmm...

> The whole executable object expects the user property to be the current user.

Is there any evidence for that? I see evidence for the opposite, besides the user property, read comments in e.g. node_query_node_access_alter().

> A user who is able to use unserialized view executables, ... might then bypass access restrictions.

To unserialize arbitrary objects you already need execute-php, which is far stronger. So i see no privilege escalation potential here.

But hey no problem, we can give the component maintainer both patches to decide. It's anyway up to them.

mxh’s picture

> The whole executable object expects the user property to be the current user.

Is there any evidence for that? I see evidence for the opposite, besides the user property, read comments in e.g. node_query_node_access_alter().

Documentation of the ViewExecutable property user, ::__construct() as well as ::getUser():

<?php
/**
   * The current user.
   *
   * @var \Drupal\Core\Session\AccountInterface
   */
  protected $user;

// ...

  /**
   * Constructs a new ViewExecutable object.
   *
   * @param \Drupal\views\ViewEntityInterface $storage
   *   The view config entity the actual information is stored on.
   * @param \Drupal\Core\Session\AccountInterface $user
   *   The current user.
   * @param \Drupal\views\ViewsData $views_data
   *   The views data.
   * @param \Drupal\Core\Routing\RouteProviderInterface $route_provider
   *   The route provider.
   */
  public function __construct(ViewEntityInterface $storage, AccountInterface $user, ViewsData $views_data, RouteProviderInterface $route_provider) {

// ...

/**
   * Gets the current user.
   *
   * Views plugins can receive the current user in order to not need dependency
   * injection.
   *
   * @return \Drupal\Core\Session\AccountInterface
   *   The current user.
   */
  public function getUser() {
    return $this->user;
  }
?>

Besides there's also no setUser() method which allows to change the user object, this should indicate that the executable view always expects the current(ly logged in) user.

> A user who is able to use unserialized view executables, ... might then bypass access restrictions.

To unserialize arbitrary objects you already need execute-php, which is far stronger. So i see no privilege escalation potential here.

Sorry, I might have not explained it correctly. I meant the permission to use / access executables which have been serialized before. The permission to access unserialized data doesn't require a permission to unserialize.

But hey no problem, we can give the component maintainer both patches to decide. It's anyway up to them.

Yes, and since there might be something different which needs to be done before it's RTBC, I'd wait for a response from them. But feel free to provide an alternative patch which contains the solution you'd prefer.

mxh’s picture

I see evidence for the opposite, besides the user property, read comments in e.g. node_query_node_access_alter().

I've read the comments in node_query_node_access_alter(), but I'm not sure which comment is related to this issue and would indicate a different behavior for the ViewExecutable. Could you please point to a concrete example / comment?

axel.rutz’s picture

Regarding node_query_node_access_alter(), i meant that it is prepared for non-current-user queries:

  // Read meta-data from query, if provided.
  if (!$account = $query->getMetaData('account')) {
    $account = \Drupal::currentUser();
  }

But in fact i did not check that all ViewExecutable is prepared for that. Well, if not we can add a feature issue for that.

But important for this is the objection you stated about permissions:

Sorry, I might have not explained it correctly. I meant the permission to use / access executables which have been serialized before.

Is there a user permission to "use any serialized view" in any *.permissions.yml? I did not get that if.

mxh’s picture

Is there a user permission to "use any serialized view" in any *.permissions.yml? I did not get that if.

I brought in the term "permission" but that wasn't intended to focus the discussion on permission definitions. Although there doesn't exist a concrete permission in core about using any serialized view, there might be use cases for it.

The problem I see here is the general possibility that a user might get valid access to a view, which might have been serialized before by another user (e.g. maybe guest or admin). The ViewExecutable class provides the current user object, and this object must not differ from \Drupal::currentUser() IMO.

mxh’s picture

Status: Needs work » Needs review

I'd like to have at least the error itself getting fixed in core, thus changing status to try gaining some attention of the maintainers.

Please review the patch and give feedback whether it solves (at least partially) the serialization problem for you.
What the serialization process should include like it has been discusses above, may be of course discussed further, although it's not really part of this issue.