Fix security issue and make release to bring back D6 releases

I've been trying to find a maintainer for VBO 6.x since early 2012. Nobody ever showed up, and VBO 6.x stayed mostly unmaintained.
Now there's been a security issue found, and there was nobody to fix it.
So, the security team had to remove the D6 releases, per protocol.

I'm hoping one of those 20,000 - 30,000 sites will give us a new D6 maintainer :)

(Same thing happened to Services 6.x a year ago, but nobody ever showed up)

That is understandable.

I did a quick glance and it looks like another mitigation step is turning on the "Action Permissions" submodule. You do have to then set permissions for all your roles but this stops anonymous users from firing an action. Would forcing that submodule to be enabled satisfy the security issue for now?

@dmundra
No, the issue needs to be fixed, for both the "role" and the "fields" actions.

I propose the following patch to fix the security issue described here. This patch was made against 6.x-1.16.

Now, if a user tries to modify a user's role using views_bulk_operations_user_roles_action they will fail unless:

• The module actions_permissions is enabled.
• OR The user has the core "administer users" permission.

Please test and let me know if I missed something.

Disregard this post, I thought I got something wrong. #4 seems to be working for me

@fernando-iglesias #4 works for me but there are still other actions that are allowed that are not checked for permissions. If you enable the view with the path 'admin/user/user2' you can the see the available actions. See screenshot.

Updating the original report to focus on the issue at hand.

Aegir 2.x (and therefor devshop) relies on views bulk operations 6.x.

They want to move people to 3.x but it's not released yet, and I won't be able to update devshop front-end to 6.x for a while.

Aegir install depends on a successful drush make so this is breaking it.

I'm going to be forced to try and patch this as the maintainer of devshop so I guess I might as well offer to be the 6.x maintainer in the near term.

I'll offer to do it only on the condition that we keep looking for another 6.x maintainer for the long term. :D

@Jon Pugh
Deal! Thank you.
I've added you as a co-maintainer. You can now contact klausi and/or greggles and ask to be added to the security issue.

I'll keep the issue open so that we can find a more permanent maintainer.

@roball - not sure how fast. Jon Pugh does have access now to the private issue so he can see the details of the issue and hopefully fix it faster than he would just looking at the commit to 7.x. If someone else can create a patch for him to review that would probably help, too.

Given there's a new maintainer, might it be possible to re-open the D6 branch and just don't set a recommended release? Right now it isn't possible to do anything with D6 issues, though D5 issues are still fair game :-P

Applied patch in #4 and following. I use total control for better user management and would really like to continue to use it.

There's another bug with how update module shows releases, so I've republished the 6.x-1.x-dev branch to see if that fixes the update module bug.

In general we unpublish them to make it clear that the code is not secure. Once the update xml at http://updates.drupal.org/release-history/views_bulk_operations/6.x is updated to remove the 6.x-1.13 then will someone please post a screenshot of what the update module inside the site shows?

OK, I removed support from it.

@Jon Pugh - a nice workaround would be a fixed release ;)

@roball - can you provide a patch that improves on the existing one?

Working on this now.

Trying to wrap my mind around the old views UI!

All of the patches here add code to the "validate" step. Wouldn't it be best to hide the option completely from users that don't have that permission?

Attached is a patch that checks for permission before returning the "operations" array in "views_bulk_operations_user_roles_action_info()".

This way the option doesn't even appear unless you have permission to "administer users" or "execute Modify user roles (views_bulk_operations_user_roles_action)".

This way it works with or without actions permissions enabled.

Thoughts?

There are two more operations that we should probably adjust: "delete" (which gives deletion powers to all entities without checking access) and user block and unblock.

Should I go ahead and check permissions for those as well?

Hey Greggles: I don't see any issues for me at https://security.drupal.org/project/issues/user?text=&projects=&status=A...

Not sure what to do next :)

Given that the permission won't exist until the action_permissions module is enabled, is this ok to do? I think you might need to expand that to check if that submodule is enabled first before checking the extra permission.

Thanks, Damien!

It's perfectly fine, actually.

From user_access():

 return isset($perm [$account->uid][$string]);

If the permission string doesn't exist, it safely assumes the user doesn't have it. And actually, based on that $perm array, user_access() can't tell the difference between a missing permission and a missing module with that permission.

Since this has been disclosed publicly there's no need to coordinate on security.d.o further.

Next steps are fix the bug (seems largely done), get a good review that approves the change, and commit/make release and then ping here to get it published.

Who will approve the review and push the patch if there's no maintainer?

Jon was already granted maintainer status, so re-titling this issue.

Yay Jon! :-)

In the name of progress, I am going to commit this patch myself and put out a new release.

Can't wait any longer!

Hiding the whole action even from the admin feels odd, we've traditionally showed it, but done nothing in case permissions are insufficient (D7, for example).

So why not do the check in the actual action, and return if access is insufficient? Feels friendlier.

Would it be hidden from an admin? Doesn't an admin have "administer users" permission?

Not sure what you mean "check in the actual action"? In function views_bulk_operations_user_roles_action() ?

I'm not sure showing the user an option they are not allowed to take would be considered "friendlier", but it's not my module, so... :)

Was about to push out a release. Considering the security hole is fixed can we save user interface tweaks for a next release?

I've always considered the "Drupal way" to only show actions that can be performed, rather than finding out after the fact "sorry, you can't do that". It may also be worth having the permission check at the API level, but that's for someone else to decide.

Sounds like an RTBC to me.

I think this patch would count as "at the API level" since I'm blocking it from the default operations hook.

Thanks, Damien!

Release Pending!

Release created: https://www.drupal.org/node/2545390

Calling it closed!

Thanks, everyone, for your participation.

It will auto-close in two weeks.

Can help people looking for recent commits.

I marked the release as a "security update" so that people will know to upgrade to it. In the future please be sure to do that on any release that fixes a security issue.

Oops!

Thanks, Joel & Greg!

No worries, I'm pulling for you. We're all in this together;)

Thanks guys. The module page needs to be updated to not say that the 6.x release is abandoned (in the compatibility section).

Thanks everyone for working on this! As one of the people who reported the bug with Update module not showing the 6.x update status properly, I'm glad to see that my sites are now properly requesting an upgrade from 6.x-1.13 to 6.x-1.17