EntityAutocomplete element settings allows sql injection and for arbitrary user-supplied data to be passed into unserialize()

Good find

unserializing user provided input is a sechole, hence this is critical

/**
   * Autocomplete the label of an entity.
   *
   * @param \Symfony\Component\HttpFoundation\Request $request
   *   The request object that contains the typed tags.
   * @param string $target_type
   *   The ID of the target entity type.
   * @param string $selection_handler
   *   The plugin ID of the entity reference selection handler.
   * @param string $selection_settings
   *   The settings that will be passed to the selection handler.
   *
   * @return \Symfony\Component\HttpFoundation\JsonResponse
   *   The matched entity labels as a JSON response.
   */
  public function handleAutocomplete(Request $request, $target_type, $selection_handler, $selection_settings = '') {
    $matches = array();
    // Get the typed string from the URL, if it exists.
    if ($input = $request->query->get('q')) {
      $typed_string = Tags::explode($input);
      $typed_string = Unicode::strtolower(array_pop($typed_string));

      // Selection settings are passed in as an encoded serialized array.
      $selection_settings = $selection_settings ? unserialize(base64_decode($selection_settings)) : array();

      $matches = $this->matcher->getMatches($target_type, $selection_handler, $selection_settings, $typed_string);
    }

    return new JsonResponse($matches);
  }

The key hunk here:

$selection_settings = $selection_settings ? unserialize(base64_decode($selection_settings)) : array();

see http://heine.familiedeelstra.com/security/unserialize

Confirming issue doesn't exist in D7.
Introduced by #1959806: Provide a generic 'entity_autocomplete' Form API element

In D7 and before #1959806: Provide a generic 'entity_autocomplete' Form API element selection settings were part of the field, and the field name was what was passed around.

Possible fixes - use key-value to keep the settings server side and pass that back instead? Key would be a hash.

Another possible fix, use json_encode/decode instead, forcing to associative array

poking

Using json_encode/decode instead as the value is an array.

Manual testing of patch at #9

Makes sense, but that's not really what the original report is about? It's still going to be too long if you put a lot of stuff in your settings? Maybe we need two separate issues, although fixing the original issue might also fix the security problem?

I'm not sure how to fix it however... we could try to depend on form_state again and reference that, but we want to get away from that, or a custom place to store the settings and reference them... ideas?

Right agree separate issues makes sense

Let's fix the security issue first because finding a better place to store those settings should not be release blocking. Nice work, @larowlan!

Is there no way to add test coverage for this issue?

Yep same question as #15.

Test what exactly?

We're not changing anything functional... the base64 encoded string just looks different now.

We could try to manually call it with a base64 encoded unserialize string... but to what point? json_decode() will just fail on that? Maybe we could better handle a scenario where we fail to successfully decode a string, but that's not really the reported problem?

We could try to manually call it with a base64 encoded unserialize string... but to what point?

Well that would pick up if this patch ever got reverted, but yes can't really think of a good test.

We should at least add docs to explain we're using json_encode/decode because this is user-supplied input though.

That makes sense, NW for adding some docs.

@Heine asked if the match operator in $selection_settings could lead to sqli, I think he's right.
The match operator from the settings gets into $query->condition($field, $value, $match_operator)
Then that gets into $conditions_fragment thus ' (' . $connection->escapeField($condition['field']) . ' ' . $operator['operator'] . ' ' . $operator['prefix'] . implode($operator['delimiter'], $placeholders) . $operator['postfix'] . ')
unescaped
which becomes $this->stringVersion = implode($conjunction, $condition_fragments);

@Berdir confirmed there is no validation of operators in $query->condition($field, $value, $match_operator)

Heine+=1000

Let's fix the security issue first because finding a better place to store those settings should not be release blocking

Finding a better place to store the settings is not release blocking, but it would mean we could ignore this problem. If we just have a reference or something that points to the settings, then we don't have to worry about the two security problems anymore?

I'm not sure how we want to do this, but if we can find a nice solution for that, then maybe we can just move forward with that?

Damn, it seems there are too many issues with exposing these settings on the client side. D7 and previous D8 had it easy because it was all stored in the field definition :/ Ok then, let's switch to something like the proposal from #6.

I think its a bad idea to provide such settings via the user input at all. A user should not be able to change that.

Of course we run into the same chicken-egg problem as with $form_state - unless we can cache this by some key?

So can we "cache" this in KV-store, so that one settings_instance is shared across all users?
(If we can't then would need to set max-age=0 anyway as currently it could be cached already in some upper level, hence it needs to provide cache contexts anyway, which means it should be cacheable.)

I would prefer a solution where random users are not able to change ER autocomplete selection settings. Even if the SQL-i and unserialize issues are fixed it might still be possible to extract confidential information from the database if a user can e.g. change the entity property used for matching or sorting. I must say I haven't completely analyzed the flow to see if that's possible, but I assume it is from what I've seen so far.

Ah, that seems doable, yes. hash the settings, put it in key value if it doesn't exist yet and then reference that...

That will fix the original issue too, will take this on tomorrow unless someone beats me to it

Cleaning up my own mess :/ A test-only patch is not needed because we're changing the API here.

Ahem.

Nice...

1. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -112,11 +114,21 @@ public static function processEntityAutocomplete(array &$element, FormStateInter
   +    if (!$key_value_storage->has($selection_settings_key)) {
   +      $key_value_storage->set($selection_settings_key, $selection_settings);
   +    }
   

   expirable without expiration isn't actually expirable :)

   Was wondering about this too, do we want to make it expirable or not. Worst case is that we have a few left-over keys in there that we no longer use...

2. +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -55,21 +70,40 @@ public static function create(ContainerInterface $container) {
   +        if ($selection_settings_hash !== $selection_settings_key) {
   +          // Disallow access when the selection settings hash does not match the
   +          // passed-in key.
   +          throw new AccessDeniedHttpException('Invalid selection settings key.');
   +        }
   

   Could this really happen? We wouldn't have been able to load it if it doesn't match?

expirable without expiration isn't actually expirable :)

lol.. fixed :)

Was wondering about this too, do we want to make it expirable or not. Worst case is that we have a few left-over keys in there that we no longer use...

I'm not sure.. better safe than sorry?

Could this really happen? We wouldn't have been able to load it if it doesn't match?

Yep, it can happen if the user tries to access the url using the selection settings key from another entity_autocomplete element.

Fixed the tests.

If you cache for 6 hours, you should also set $element['#cache']['max-age'] = '6 hours as integer number'; so that the caches will be synchronized properly.

Or rather you would need to set it to whatever the expiration / time left / age on that key-value entry right now is.

I know we have not taken care of that (cache sync) before, but autocomplete is a case, where I think it could even happen on anonymous pages that you want it somehow ...

As this is a critical to not hold this up, just setting 6 hours is probably fine, but would be good to have a way for KV-expirable to 'refresh' a timestamp as follow-up.

If we are not worried about write performance, we could always set it in KV for now freshly - as the hash should be unique anyway.

1. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -112,11 +114,22 @@ public static function processEntityAutocomplete(array &$element, FormStateInter
   +      // Match the 6 hours cache lifetime of forms.
   +      $key_value_storage->setWithExpire($selection_settings_key, $selection_settings, 21600);
   

   What happens if a form is cached in page cache for > 6h? For non-ajax submissions, we just rebuild, for ajax, I'm not sure...

2. +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -55,21 +70,40 @@ public static function create(ContainerInterface $container) {
   +        $selection_settings = $this->keyValueExpirable->get($selection_settings_key);
   +        $selection_settings_hash = Crypt::hmacBase64(serialize($selection_settings), Settings::getHashSalt());
   +        if ($selection_settings_hash !== $selection_settings_key) {
   

   Sorry, but I still don't get this ;)

   If you submit a different key, then you also get different settings.. and the hash will then match again?

   If you want to at least verify that it matches with the target type and selection handler, then you need to include them in the hash. Or we could store that in key value as well and just use the key to identify it all (but then checking also makes no sense again).

Ok, let's not bother with expirable key/value anymore because it just complicates things.

If you want to at least verify that it matches with the target type and selection handler, then you need to include them in the hash.

Uhm, yes, I think that's what I wanted to do :)

Looks good to me

Yeah, that makes more sense now, +1 :)

+++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
@@ -55,21 +70,40 @@ public static function create(ContainerInterface $container) {
+      if ($this->keyValue->has($selection_settings_key)) {
+        $selection_settings = $this->keyValue->get($selection_settings_key);

Why not just do a get($selection_settings_key, FALSE) and then check if !== FALSE. Saves a round trip to the key value storage. Which I think will save a query when using the default KeyValue DatabaseStorage implementation. Asked @Berdir and he thought changing this makes sense too.

Meant to say in #42 that apart from that this looks good to go. Although have we considered how the key value store might explode?

Fixed #42, good point :)

I think @Berdir was not very alarmed about an exploding key value store. The number of items we store should be in the order of hundreds in the worst case. It's basically ($target_type + $selection handler) + every possible combination of selection handler settings (~5-7 settings at most, afaik).

Back to RTBC, looks great to me!

Right, most sites are only going to have a handful of those, one for each entity reference field (or in many cases even less, because the same settings would use the same key, across different fields).

And even if it would be a problem, then it should be easy to update.. we can just switch to storing it somewhere else and empty this key value collection...

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 4a576b0 and pushed to 8.0.x. Thanks!

An important follow-up question to ask:

- Is anything else in core using unserialize() or any other user input data we custom serialize? (either via json or php)

Together with chx & catch (Thank you very much both!) we discussed the SQL-injection here.

For those not familiar with the original patch:

+      $match_operator = !empty($selection_settings['match_operator']) ? $selection_settings['match_operator'] : 'CONTAINS';
+      $entity_labels = $handler->getReferenceableEntities($string, $match_operator, 10);

is the code that in the end can end with a SQL-injection attack.

The problem is that this code seems in no way database related, but in the end it ends up within a ->condition() call. (but you have to follow some layers to get there).

Also nothing in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21... warns you that match_operator MUST not be user input ...

Just put a JS form via bootstrap / other JS framework => ?operator=LIKE => use the $_GET parameter (yes, you should not but real life ...) => Possible SQL injection

=> Conclusion: Our SQL layer is still vulnerable.

Our consensus was to open at least an issue to discuss possibilities to either strip all whitespace or use a white list per database layer or a combination.

Will do so now ...

#2492967: SQL layer: $match_operator is vulnerable to injection attack is the new issue to harden the match operator.

Would it still be possible to get credited for reporting the SQLi (and potential info leak) via larowlan?

@Heine I've added you to the credits on this issue (i.e. the d.o storage) for now at least.