CSRF in update module manual check links

This affects Drupal 8 too (though without the "?q=" in the URL).

It should be as simple as this (plus some test fixes to change code that visits the URL directly to click the appropriate link instead).

However, when I tested this manually it didn't work, even after a cache clear. The token is added to the link, but when you click on it the token is treated as invalid and you get "access denied". I debugged a bit but then stopped for now. Looking in the issue queue perhaps it's due to #2630920: _csrf_token is broken due to cacheability metadata integration, results in rendered links without valid CSRF tokens (or more likely one of the issues linked therein).

Although the impact of this CSRF is low, I think it still is a security improvement - tagging accordingly.

Please forgive me, I'm not sure the process when backport patches are required. I just read through https://www.drupal.org/core/backport-policy and am still a bit confused in this situation.

For this issue it looks like a simple change to the update.routing.yml file obviously not present in D7. I have a patch that I would like to submit for D7. Please let me know if I need a separate D7 issue for this or not.

Thanks,

@aaronott - thanks for working on it! I think you should go ahead and post your patch here in this issue. Because of the backport policy we make sure to focus on fixing the issue in Drupal 8 first (and then Drupal 7 afterwards), but it still will be useful to have a Drupal 7 patch now, especially if you already wrote it.

Also if it turns out the proper fix for this in Drupal 8 is blocked on the other issue I linked to (which it might be) then we may need an interim solution for Drupal 8 which would probably be similar to the Drupal 7 one.

Thanks @David_Rothstein, Here is the patch I mentioned for D7.

Attaching a patch for 8.x. We add the CSRF requirement in the router, so all links generated should automatically have the token.

I'm sorry I went ahead and even submitted the patch, and then realized there is an exactly same patch already. I could reproduce the same access denied error.

It turns out Url::fromRoute() gives the proper URL. _update_no_data() functions run-cron link is broken as well, so it is definitely the other issue David mentioned.

Since the fix for D7 is unrelated to the fix for D8, should I create a separate ticket for the D7 issue?

In what way are they unrelated?

Given that #2630920: _csrf_token is broken due to cacheability metadata integration, results in rendered links without valid CSRF tokens doesn't look like it will be fixed soon, and that it appears to block doing this issue in the "proper" way for Drupal 8, I think what we should do here is what I suggested in #8 - make the Drupal 8 patch use the old-style CSRF protection method (in other words, make it do manual token checking just like the Drupal 7 patch), with a @todo to replace that with the correct method once the linked issue is fixed.

That way we can still fix it in both Drupal 7 and 8 without it getting blocked on other issues.

We can just fix the preprocess so that the metadata bubbles properly and the _csrf_token works. #2630920: _csrf_token is broken due to cacheability metadata integration, results in rendered links without valid CSRF tokens wouldn't fix this anyway.

So the issue with going to admin/reports/updates to click on the tokenised link is that it'll refresh the update information because we call update_get_available(TRUE) in UpdateController::updateStatus(). This changes some tests. So we can fix it by going to the url at the right time in the test. However this means that admin/reports/updates is also a path for a brute force attack described in the summary.

Nice, I guess it was the "or more likely one of the issues linked therein" that I mentioned in #4, then. Switching this to point to the correct issue (I think?).

Based on a quick glance the patch looks like a reasonable workaround to me.

So the issue with going to admin/reports/updates to click on the tokenised link is that it'll refresh the update information because we call update_get_available(TRUE) in UpdateController::updateStatus().... However this means that admin/reports/updates is also a path for a brute force attack described in the summary.

My recollection is that update_get_available(TRUE) does a lot less than the "Check manually" link though. I think the former only fetches project data if it's not in the local cache, whereas the latter always fetches the project data no matter what.

Overall the brute force attack here is quite weak either way though.

@David_Rothstein yeah I agree that we probably shouldn't worry about admin/reports/updates as this just processes a queue.

Let's make it more explicit what we're doing to make the tests pass.

Ideally we would enforce a router rebuild, as its required to make the path secure. I don't remember exactly whether update.php does one, when there are no pending updates.

Its really nice IMHO that core makes it really easy to add CSRF protection to specific pages.

Oh well just going back to the approach in #20 for \Drupal\update\Tests\UpdateContribTest::testUpdateBrokenFetchURL().

Discussed with @catch and @xjm. We agreed this is a major bug as an admin could be set an email with image links in to do such a check.

Finally got round to adding the update.

Can actually just have an empty update otherwise we'll rebuild the routes twice :(

+++ b/core/modules/update/update.module
@@ -567,7 +567,12 @@ function _update_project_status_sort($a, $b) {
-  $variables['link'] = \Drupal::l(t('Check manually'), new Url('update.manual_status', array(), array('query' => \Drupal::destination()->getAsArray())));
+  $variables['link'] = [
+    '#type' => 'link',
+    '#title' => t('Check manually'),
+    '#url' => Url::fromRoute('update.manual_status', [], ['query' => \Drupal::destination()->getAsArray()]),
+  ];

This should not be necessary. The code original code should work.

This is indeed related to #2575519: Twig template variables containing result of Drupal::url() and Drupal:l:() don't bubble up their cacheability and attachment metadata (e.g. token placeholder).

Rather than making this change, we should fix the root cause.

ThemeManager does this:

    if (isset($info['preprocess functions'])) {
      foreach ($info['preprocess functions'] as $preprocessor_function) {
        if (function_exists($preprocessor_function)) {
          $preprocessor_function($variables, $hook, $info);
        }
      }
      // Allow theme preprocess functions to set $variables['#attached'] and
      // $variables['#cache'] and use them like the corresponding element
      // properties on render arrays. In Drupal 8, this is the (only) officially
      // supported method of attaching bubbleable metadata from preprocess
      // functions. Assets attached here should be associated with the template
      // that we are preprocessing variables for.
      $preprocess_bubbleable = [];
      foreach (['#attached', '#cache'] as $key) {
        if (isset($variables[$key])) {
          $preprocess_bubbleable[$key] = $variables[$key];
        }
      }
      // We do not allow preprocess functions to define cacheable elements.
      unset($preprocess_bubbleable['#cache']['keys']);
      if ($preprocess_bubbleable) {
        // @todo Inject the Renderer in https://www.drupal.org/node/2529438.
        drupal_render($preprocess_bubbleable);
      }
    }

I.e. we allow preprocess functions to explicitly set #attached and #cache. In this case, our preprocess function is not doing that. Instead, it is assigning a value to one of the variables that itself contains cacheability metadata and attachments!

So, where these variables are being used, we must update Twig to do the necessary bubbling, just when those variables are actually printed. The attached patch does that.

And here is it integrated with the patch in this issue.

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -462,6 +466,24 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
+      if ($arg instanceof CacheableDependencyInterface) {
+        $arg_bubbleable['#cache']['contexts'] = $arg->getCacheContexts();
+        $arg_bubbleable['#cache']['tags'] = $arg->getCacheTags();
+        $arg_bubbleable['#cache']['max-age'] = $arg->getCacheMaxAge();
+      }
+      if ($arg instanceof AttachmentsInterface) {
+        $arg_bubbleable['#attached'] = $arg->getAttachments();
+      }
+      if ($arg_bubbleable) {
+        // @todo Inject the Renderer in https://www.drupal.org/node/2529438.
+        drupal_render($arg_bubbleable);
+      }

Note: We have the renderer already. I'm also wondering whether a dedicated method which just allows to add bubbleable metadata would make sense.

I'm also wondering whether a dedicated method which just allows to add bubbleable metadata would make sense.

I don't understand what you mean. You dislike the protected helper method I added?

I don't understand what you mean. You dislike the protected helper method I added?

Something like $renderer->addBubbleableMetadata, but yeah nevermind about it.

Rerolled now #2575519: Twig template variables containing result of Drupal::url() and Drupal:l:() don't bubble up their cacheability and attachment metadata (e.g. token placeholder) landed.

Looks perfect!

+++ b/core/modules/update/update.install
@@ -148,3 +148,20 @@ function _update_requirement_check($project, $type) {
+ * Rebuild the router to ensure admin/reports/updates/check has CSRF protection.

Supernit: missing leading slash.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Opened the 7.x backport issue here: #2731187: CSRF in update module manual check links.