Implement a tamper-proof hash for the media library state

1. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -76,19 +80,36 @@ public static function create($opener_id, array $allowed_media_type_ids, $select
   +    $hash = static::getHash(
   +      $query->get('media_library_opener_id'),
   +      $query->get('media_library_allowed_types'),
   +      $query->get('media_library_selected_type'),
   +      $query->get('media_library_remaining')
   +    );
   

   Is there any reason for this to be a static method we pass parameters into? Seems to me like we could have a public getHash() method to do that (which would not need any arguments except the private_key service, which could be passed if needed to facilitate unit testing), like so:

   public function getHash(PrivateKey $private_key = NULL) {
     $private_key = $private_key ?: \Drupal::service('private_key');
   ...
   }
   

   And then we could even have an isValid() method which calls it, like so:

   public function isValid() {
     return Crypt::hashEquals($this->getHash(), $this->get('hash', ''));
   }
   

   Overall I think $state->isValid() might be a cleaner encapsulation.

2. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -76,19 +80,36 @@ public static function create($opener_id, array $allowed_media_type_ids, $select
   +    if (!Crypt::hashEquals($hash, $query->get('hash', ''))) {
   +      throw new AccessDeniedHttpException("Invalid 'hash' query parameter.");
   +    }
   

   Having this class throw AccessDeniedHttpException seems like overreach. IMHO, the calling code should be responsible for asking the state object if it is valid, then taking appropriate action.

3. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -206,21 +206,15 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
   +      $link_state = MediaLibraryState::create($state->getOpenerId(), $state->getAllowedTypeIds(), $allowed_type_id, $state->getAvailableSlots());
   

   Whoa. Couldn't we just do $link_state = clone $state; here? (We'll need a comment either way.)

Looking good :)

1. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -76,19 +80,36 @@ public static function create($opener_id, array $allowed_media_type_ids, $select
   +    // The URL needs to contain a valid hash if a state object is created from
   +    // the request.
   

   , to prevent a malicious user modifying the query string to attempt to access inaccessible information.

2. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -76,19 +80,36 @@ public static function create($opener_id, array $allowed_media_type_ids, $select
   +      throw new AccessDeniedHttpException("Invalid 'hash' query parameter.");
   

   🤓 You could even turn this into a \Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException and pass url.query_args as the cache context!

3. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -140,6 +161,28 @@ protected function validateParameters($opener_id, array $allowed_media_type_ids,
   +    asort($allowed_media_type_ids);
   

   🤔 Why do we need these to be sorted? Yes, it affects the hash. But why/how could the order by different?

4. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -140,6 +161,28 @@ protected function validateParameters($opener_id, array $allowed_media_type_ids,
   +    return Crypt::hmacBase64("$opener_id:$allowed_media_types:$selected_type_id:$remaining_slots", \Drupal::service('private_key')->get() . Settings::getHashSalt());
   

   👍 Using hash salt

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -185,6 +185,29 @@ public function testWidgetAccess() {
   +    $assert_session->responseContains('Access denied');
   

   Nit: I personally think ->statusCodeEquals(403); would be clearer?

6. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryStateTest.php
   @@ -256,4 +258,88 @@ public function providerCreate() {
   +    // Assert an exception is thrown when we change the actual hash.
   +    $test_data['changed hash'] = [
   

   You are using descriptive strings as array keys for the data provider, great! But you're also providing comments. The comments should be unnecessary, the strings should capture it all. That makes going through test failures ten times easier. So, in this case: $test_data['a modified (invalid) hash triggers a 403'], for example?

Tagging this as a beta target for 8.7.x because it's a minimally disruptive, non-user facing security improvement for Media Library. The only way this patch will break anything is if you've been tampering with Media Library's query parameters ;)

+++ b/core/modules/media_library/src/MediaLibraryState.php
@@ -76,19 +80,36 @@ public static function create($opener_id, array $allowed_media_type_ids, $select
+    $hash = static::getHash(
+      $query->get('media_library_opener_id'),
+      $query->get('media_library_allowed_types'),
+      $query->get('media_library_selected_type'),
+      $query->get('media_library_remaining')
+    );
+    if (!Crypt::hashEquals($hash, $query->get('hash', ''))) {
+      throw new AccessDeniedHttpException("Invalid 'hash' query parameter.");
+    }

Sean and I discussed this in Slack today. We hashed out a compromise...

We'll make getHash() a non-static protected method. It's okay if it uses \Drupal::service(), since that doesn't actually prevent unit testing (it just means you need to call \Drupal::setContainer() during test setup). This way, we can call it without any arguments. fromRequest() will instantiate the state object, call its protected getHash() method, and throw BadRequestHttpException, not AccessDeniedHttpException, if the hash is invalid. If the calling code wants to return a 403 if the hash is invalid, it can catch and wrap the BadHttpRequestException.

We hashed out a compromise...

😆

I raised another concern with Sean, and wanted to copy/paraphrase our discussion here:

phenaproxima
I think we DO need to hash all legitimate params.
Because consider this
Let’s say we have some opener with an additional param
If an attacker has a valid hash for the required parameters, they can do WHATEVER THEY WANT to that additional param and it will pass validation
And that is a security hole

seanb [9:48 AM]
The problem is, we don’t know which parameters users are allowed to change and which they are not allowed to change.
Changing a views filter is totally fine, and that would break (edited) 

phenaproxima [9:49 AM]
Right.
Should I raise this concern in the issue?

seanb [9:51 AM]
I think in this case the opener need to add their own hash to the URL if they need it

phenaproxima [9:52 AM]
Should we add a convenience method to help with that?

seanb [9:53 AM]
There actually already is a convenience method

phenaproxima [9:53 AM]
Oh?

seanb [9:54 AM]
`Crypt::hmacBase64()` :joy:

phenaproxima [9:54 AM]
I do agree with you that keeping the scope small is a good idea
However, I want to be sure that we are not introducing a security hole here.

seanb [9:54 AM]
If calling code needs to validate their own stuff, there is not stopping them from doing that
After you have created a state, you can add, remove replace everything you want
Just don’t mess with our hash and params, because that will make our code angry

So basically, if an opener wants to add and maintain additional query parameters beyond the ones Media Library already does, that opener is fully responsible for validating those parameters, either by hashing or by some other method. This seems acceptable to me, but I would appreciate a +1 from a framework manager or security team member.

I think

if an opener wants to add and maintain additional query parameters beyond the ones Media Library already does, that opener is fully responsible for validating those parameters

makes sense to me. You can't be responsible for things you don't know about.

Partial review...

1. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -60,12 +64,15 @@ public function __construct(array $parameters = []) {
   +    // Add a hash to the state parameters.
   +    $state->set('hash', $state->getHash());
   

   Is there any reason not to do this in __construct()? If so, let's expand the comment to explain why.

2. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -76,19 +83,32 @@ public static function create($opener_id, array $allowed_media_type_ids, $select
   +    $query = $request->query;
   

   Thanks! This makes the method much more readable.

3. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -76,19 +83,32 @@ public static function create($opener_id, array $allowed_media_type_ids, $select
   +    if (!$state->isValidHash($query->get('hash', ''))) {
   

   I don't think we need to pass the default value to $query->get().

4. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -140,6 +160,42 @@ protected function validateParameters($opener_id, array $allowed_media_type_ids,
   +   * @param \Drupal\Core\PrivateKey $private_key
   +   *   The private key service.
   

   Needs to start with (optional).

5. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -140,6 +160,42 @@ protected function validateParameters($opener_id, array $allowed_media_type_ids,
   +    $hash = implode(':', [
   +      $this->get('media_library_opener_id'),
   +      implode(':', $this->get('media_library_allowed_types')),
   +      $this->get('media_library_selected_type'),
   +      $this->get('media_library_remaining'),
   +    ]);
   

   We should call our own methods here, like $this->getOpenerId(), $this->getSelectedTypeId(), etc.

1. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -140,6 +160,42 @@ protected function validateParameters($opener_id, array $allowed_media_type_ids,
   +  public function getHash(PrivateKey $private_key = NULL) {
   

   Given the fact that isValidHash() doesn't allow us to pass in the private key service, I'm thinking we can just go ahead and use \Drupal::service() without worrying about injecting it. Unit tests can easily call \Drupal::setContainer() if they need to bother with this.

2. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -159,12 +159,28 @@ protected function buildLibraryContent(MediaLibraryState $state) {
   +      catch (BadRequestHttpException $e) {
   +        return AccessResult::forbidden($e->getMessage())
   +          ->setCacheMaxAge(0);
   +      }
   +      catch (\InvalidArgumentException $e) {
   +        return AccessResult::forbidden($e->getMessage())
   +          ->setCacheMaxAge(0);
   +      }
   

   I wish we could use PHP 7.1, then we could catch (BadRequestHttpException | \InvalidArgumentException).

3. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -206,21 +222,15 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
   -    unset($query[MainContentViewSubscriber::WRAPPER_FORMAT], $query[FormBuilderInterface::AJAX_FORM_REQUEST], $query['_media_library_form_rebuild']);
   

   Why is this line removed? Seems like it might be important to maintain it?

4. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
   @@ -101,25 +104,41 @@ public function testMediaLibraryAccess() {
   +  /**
   +   * Checks the media library access for the test opener.
   +   *
   +   * @param \Drupal\media_library\Event\MediaLibraryCheckAccessEvent $event
   +   *   The media library check access event.
   +   */
   +  public function onMediaLibraryCheckAccess(MediaLibraryCheckAccessEvent $event) {
   +    // Always allow access for the test opener.
   +    if ($event->getState()->getOpenerId() === 'test') {
   +      $event->setAccessResult(AccessResult::allowed());
   +    }
   

   This doesn't seem like it belongs...

#14:

1. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -63,13 +64,15 @@ public function __construct(array $parameters = []) {
   +    // Add a hash to the state parameters.
   +    $state->set('hash', $state->getHash());
   

   🤔 We're using object $foo to modify object $foo. Wait, what?!

   Addressed in #16 👍

2. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -162,25 +161,39 @@ protected function validateParameters($opener_id, array $allowed_media_type_ids,
   +  public function getHash(PrivateKey $private_key = NULL) {
   +    $private_key = $private_key ?: \Drupal::service('private_key');
   

   Why is this optional?

3. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -159,12 +159,28 @@ protected function buildLibraryContent(MediaLibraryState $state) {
   +          ->setCacheMaxAge(0);
   ...
   +          ->setCacheMaxAge(0);
   

   🤔 Why?

2. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -76,19 +80,36 @@ public static function create($opener_id, array $allowed_media_type_ids, $select
   +      throw new AccessDeniedHttpException("Invalid 'hash' query parameter.");
   

   🤓 You could even turn this into a \Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException and pass url.query_args as the cache context!

On second thought, this is not worth making cacheable. If anything, we don't want it to be cacheable, because there'd be too many variations to cache, plus it's not an expensive response to generate anyway, so this'd be a waste of caching space.

#20.3: hah! But the access result itself is cacheable. The access checking result is going to stay exactly the same so it shouldn't get max-age=0. I agree that we shouldn't cache the error response. Right now there's a problem with correctness/accuracy of the access result. The resulting response is never going to be cacheable, we don't need to tweak the access result to make the response uncacheable!

+++ b/core/modules/media_library/src/MediaLibraryState.php
@@ -76,19 +82,32 @@ public static function create($opener_id, array $allowed_media_type_ids, $select
+      throw new BadRequestHttpException("The URL contains an invalid 'hash' query parameter.");

Is the hash invalid or the other params? I'm not sure this message is correct. In other situations we do things like throw new BadRequestHttpException('Invalid contextual ID specified.'); I think we should say something like Invalid media library parameters specified.

Otherwise this looks great.

Thanks.

+1 for RTBC. Nice improvement!

Committed and pushed 2ab4c46707 to 8.8.x and f7c0c98edb to 8.7.x. Thanks!

When I attempt to use the views exposed filters in the media library widget I get the invalid hash error due to my hash having '_wrapper_format=drupal_ajax' appended to its end.

Noticed I was getting the exception error on pagination, and someone else did too.

Is there a reason an empty entity_id would appear in one hash and not in another?

Related: https://www.drupal.org/project/drupal/issues/3067041