[D7] drupal_render_collect_attached() does check neither #access nor #theme (Information Disclosure)

This issue was discussed by the Drupal Security Team, and their decision was that this can be solved in a public issue.

Problem/Motivation

Originally reported by @fabianx:

Core has a Information Disclosure vulnerability when using render caching with the #pre_render / #cache pattern.

The reason is that drupal_render_collect_attached() does not check #access when traversing the tree.

After discussion with David Rothstein, we decided that https://www.drupal.org/node/2706027 → (which has a more extensive patch) can stay public, but I should file a security issue anyway.

Example to reproduce (within a larger #pre_render / #cache pattern:

$build['non-accessible']['#access'] = FALSE;
$build['non-accessible']['#attached']['js'][] = ['do-not-access.js'];

// Creates the cache item.
drupal_render($build); // do-not-access.js is not present

// Retrieves data from cache.
drupal_render($build); // do-not-access.js is present

The same could in theory happen with #theme, when a theme variable uses a children key instead of an argument prefixed with '#', which is perfectly allowed.

$build['non-accessible']['#theme'] = 'foo';
$build['non-accessible']['foo']['#attached']['js'][] = ['do-not-access.js'];

In this case the access would be check by theme_foo() function, which is quite unlikely but possible.

Probably the easiest way to fix this is to only collect data from #printed elements, though this might have side effects and lead to bugs in custom code.

Steps to reproduce

@David_Rothstein contributed some good notes:
As far as I understand this applies to anything in #attached, not just JavaScript files as in the issue summary, so maybe here is a better example:

$build['some_content']['#access'] = user_access('bypass node access');
$build['some_content']['#attached']['js'][] = array(
  'type' => 'setting',
  'data' => array('my_module' => array('somevalue' => 'Some private data that only users with "bypass node access" should be allowed to see')),
);

My understanding of the bug is that if render caching is used, that private setting can appear in the HTML for users without the "bypass node access" permission.

Actually if I'm reading the code correctly, setting cache granularity correctly won't help here, so my example is in fact valid.

Basically, the issue (as Fabian said) is that "drupal_render_collect_attached() does not check #access when traversing the tree" so as long as the #attached code occurs in something other than the top level of the item passed to drupal_render(), I think that means my example would cause a problem even if someone with "bypass node access" never visited the page:

$build = array();
$build['some_content']['#access'] = user_access('bypass node access');
$build['some_content']['#attached']['js'][] = array(
  'type' => 'setting',
  'data' => array('my_module' => array('somevalue' => 'Some private data that only users with "bypass node access" should be allowed to see')),
);

// First visit by a user who doesn't have "bypass node access" permission:
// This won't add the setting to the HTML.
$output = drupal_render($build);

// Second visit by the same user:
// It will add the setting to the HTML?
$output = drupal_render($build);

Definitely this could use some tests and such to verify.

> When asked "how would one exploit this?"

The same way you exploit most access bypass issues - visit the site, look at the HTML output, and see things you weren't supposed to be able to see :) (In this case it would require viewing the HTML source.)

I guess the essential question is what are the conditions where a site would actually be vulnerable to this. I can't really think of a specific example offhand... but it's relatively common to have a page or a form that admins and non-admins can both view but with different levels of access (for example the node form). At that point, all you need is some JavaScript added to the admin version of that page that contains sensitive data that only admins are supposed to see, and this issue can occur. I guess it takes a somewhat complicated set of circumstances to happen, but it's really more of a fundamental security flaw in the render caching system.

@mcdruid provided steps to reproduce:

Initial (ugly) steps to reproduce:

• Implement hook_page_build() in a custom module, and add a render array to one of the regions - e.g.:

function mcdruid_test_page_build(&$page) {                                         
  $page['footer']['example'] = array(                                              
    '#type' => 'fieldset',                                                         
    '#cache' => array('cid' => 'testy'),                                           
    'mctestface' => array(                                                         
      '#type' => 'fieldset',                                                       
      '#attached' => array('js' => array(array('type' => 'setting', 'data' => array('mcdruid_test' => 'TOP SECRET!!!!1!')))),
      '#access' => isset($_GET['bypass_access']) && (bool) $_GET['bypass_access'],
    ),                                                                             
  );                                                                               
}

• Render caching is set on the parent element, and there's a child element with a JS attachment and an #access property, which in this case checks for a GET param to determine access.
• (With a clear cache...) Send a request to the site with a GET param which should mean the child element is not accessible:

$ curl -s http://drupal7x.xp/?bypass_access=0 | grep -o 'mcdruid.*$'

• Now send a request where the GET param should permit access to the JS setting:

$ curl -s http://drupal7x.xp/?bypass_access=1 | grep -o 'mcdruid.*$'
mcdruid_test":"TOP SECRET!!!!1!"});

• Now repeat the request which should not be able to access the setting:

$ curl -s http://drupal7x.xp/?bypass_access=0 | grep -o 'mcdruid.*$'
mcdruid_test":"TOP SECRET!!!!1!"});

That would seem to verify that restricted content is being (render) cached and retrieved inappropriately when access should be denied.

I should note this is on a vanilla site with page cache disabled.

However, is this actually testing the right thing? I'm not yet 100% sure whether this is down to the behaviour of drupal_render_collect_attached().

Fetching the cached item directly:

$ drush ev 'print_r(cache_get("testy")->data);'
Array
(
    [#markup] => <fieldset class="form-wrapper"><div class="fieldset-wrapper"><fieldset class="form-wrapper"><div class="fieldset-wrapper"></div></fieldset>
</div></fieldset>

    [#attached] => Array
        (
            [js] => Array
                (
                    [0] => Array
                        (
                            [type] => setting
                            [data] => Array
                                (
                                    [mcdruid_test] => TOP SECRET!!!!1!
                                )

                        )

                )

        )

)

...next step might be to turn this into a proper test.

Proposed resolution

Summary by @longwave:

We discussed this on the triage call. As in the earlier comments we couldn't think of a case where this was actually exploitable and this seems more like an API bug - the second call to drupal_render() does not appear possible to force from outside without access to the code.

It feels like as this will likely break more code than it fixes, this is probably "works as designed" at this stage. We think this should be closed unless someone objects or there is anything in this issue that could be hardened or fixed in public without disruption.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet