DoS image derivatives in Media WYSIWYG

I tested the patch and it works as expected. I did a security review which resulted in Access denied for people who had no access.

When the patch was applied, I could no longer load pages like /media/1/format-form unless the permission was set to allow that. Loading the url for a user that did not have the permission resulted in an Access denied message.

Further, running dbupdate resulted in an update DB patch being applied, and it did give access to the format-form to everybody that had access to view files before the update was run.

I also did not see any side-effects after applying the patch to the current dev.

1. +++ b/modules/media_wysiwyg/media_wysiwyg.install
   @@ -18,3 +18,23 @@ function media_wysiwyg_uninstall() {
   + * Accommodate the introduction of a new permission which restricts access to
   + * use media_wysiwyg by granting it to existing users who were able to access
   + * it.
   + */
   

   Needs a one-line summary first.

2. +++ b/modules/media_wysiwyg/media_wysiwyg.install
   @@ -18,3 +18,23 @@ function media_wysiwyg_uninstall() {
   +  return t('Use Media WYSIWYG permission was granted to: @roles. Visit !link to change these permissions.', array(
   +    '@roles' => check_plain(implode(', ', $roles)),
   +    '!link' => l(t('permissions'), 'admin/people/permissions', array(
   +      'absolute' => TRUE,
   +      'fragment' => 'module-media_wysiwyg',
   +    ))
   +  ));
   +}
   

   Let's leave out the second sentence of this string. Just notifying which roles have new permissions added is the best benefit.

3. +++ b/modules/media_wysiwyg/media_wysiwyg.module
   @@ -31,13 +31,14 @@ function media_wysiwyg_hook_info() {
   +    'access arguments' => array('use media wysiwyg'),
   

   I wonder if we need a custom access callback that also checks file_entity_access()? Is this a regression if we're not checking that in addition to the new permission?

The patch in #8 applies cleanly to the current media-7.x-2.x-dev release and works as expected. It adds a new permission to deny users access to media/%file/format-form

Media WYSIWYG
Use Media WYSIWYG in an editor
Warning: Give to trusted roles only; this permission has security implications.

However, when the patch is applied and creates this new permission it grants access to anonymous users despite the warning. Access should not be granted to anonymous users.

True @izmeez.

I've updated the patch to only grant the permission to roles that have the "access media browser" permission, but there's still a very good chance that that will include anonymous users on many sites, in light of media_update_7226().

I'm torn with this one. If we don't grant it to anons, then we may break some sites, but if we do, then many sites will go needlessly unprotected. And my understanding here of the security concern is not so much strictly a DoS problem as it is a diskspace issue: it makes easy for an attacker to fill your disk with image derivatives. We have to weigh that against the possible damage done by "breaking" sites.

And here's a more conservative version of the patch that won't grant the perm to anons.

This approach seems like a good way to go.

And the change in #11 is less code than that in #10 which is usually a good thing.

Actually #11 has almost exactly the same amount of code as #10 (less code by just 1 character!) :). Thanks for testing, izmeez.

Just a note, similar to what I posted in #1792738-182: Allow custom file view modes for WYSIWYG display, the patch in this thread has some lines overlapping with the patch in the other thread and depending on which is committed first the other will need a re-roll.

Sorry, I misunderstood, I thought the interdiff in both #10 and #11 was from the patch in #8 rather than incremental.

The patch in #11 applies to the current media-7.x-2.x-dev and works as expected. It preserves and uses the access media browser permission and does not allow anonymous users the use media wysiwyg permission.

Thanks. The status is changed to RTBC.

If this patch is committed the patch in #1792738: Allow custom file view modes for WYSIWYG display will need to be re-rolled.

There appears to be a duplicate of this issue at #2337305: Media wysiwyg exposes fields attached to a file entity via media_wysiwyg_format_form callback which maybe could be closed.

Committed to http://drupalcode.org/project/media.git/commit/02b3142.

I have an issue that may be related to this and I will have to open a separate issue. Not sure if it belongs in media or file_entity queue.

Surprised to see on new install anonymous users see content with youtube video as expected but also see above the video a menu with two items, Edit and Delete. These are links to the paths file/%/edit and file/%/delete which since the commit of the patch in this thread returns access denied.

Still it's a surprise to see this menu showing to anonymous users and not showing to admin roles when logged in.

Firebug shows them to be contextual links.

Just looking into this now. Any suggestions are welcome. Thanks.

Follow-up to #21 started a new issue #2455117: Anonymous users see menu to Edit and Delete files