block_block_list_alter() incorrectly assumes blocks are from a contrib module

Comments from security.drupal.org:

#1 Xano commented September 15, 2014 at 2:34pm

This adds an explicit check to see if the module belongs to Block itself or another module.

Comment #2 Xano commented September 15, 2014 at 2:55pm
Status: Active » Patch (code needs review)

I think the code comment here
Comment #3 Dave Reid commented September 15, 2014 at 6:15pm

I think the code comment here is incorrect instead of the code. Should be something more to the effect of "ensure the block is enabled for the current theme"

I'm not sure what the security issue is? If this patch was applied I think it would *cause* a security issue by making contrib block visible when they aren't enabled or assigned to the current theme?

Comment #4 Xano commented September 15, 2014 at 7:43pm

Hmmz, maybe I read the code a bit too quickly. The problem is that this messes up Context, which uses blocks and hook_block_list_alter() to ascertain block visibility, and this code in the Block module can cause blocks to be considered visible by the system while they shouldn't be, just because Block module thinks that if a block isn't used by a theme its visibility does not matter.

Comment #5 larowlan commented September 16, 2014 at 3:41am
Component: » Code
Access: » tekante

Comment #6 Xano commented September 16, 2014 at 9:13am
Status File
new
Review
715 bytesdrupal_security_133993_6.patch
Show 1 file was hidden/shown/deleted

Visibility settings are global per block and have no relationship with the available themes if you look at a block's form at http://gewoon-toegankelijk.tq/en/admin/structure/block/manage/$module/$delta/configure. Therefore they should be applied regardless of block status and theme settings. The attached patch ensures this by removing the entire code block that performs this check.

Comment #7 Xano commented September 22, 2014 at 9:27am
Show 1 file was hidden/shown/deleted

Bump.

Comment #8 greggles commented September 22, 2014 at 4:00pm
Category: » 1

It's hard for me to understand the security architecture/principle that is being ignored here.

@Xano, you said:

can cause blocks to be considered visible by the system while they shouldn't be,

Can you provide an example scenario including the type of site and the code/configuration so that other people can better understand the security factor here?

Comment #9 Xano commented September 22, 2014 at 4:08pm

Example scenario:

Create a block that contains any type of sensitive information that is not meant to be seen by the general public.
In the block's configuration, restrict visibility to users of a certain role and remove it from any themes.
Load the block, perhaps in a context with a block action, run the block list through hook_block_list_alter(). Then render and display the list of blocks.
Behold a displayed block that contains restricted information, because the block's visibility configuration is not applied. The reason is that Block module does not think it's relevant, because the block is not associated with the current theme (or any theme, for that matter).

What goes wrong here is that block_block_list_alter() does not apply blocks' global configuration if the block is not configured for the current theme. However, the global configuration is theme-independent and should therefore never be ignored based on any theme information.

Comment #10 Xano commented September 25, 2014 at 6:05pm

Bump.

Comment #11 Xano commented October 6, 2014 at 3:35pm

Bump.

Comment #12 greggles commented October 6, 2014 at 3:44pm

My thoughts at https://security.drupal.org/node/134008#comment-93013 apply to this issue as well, of course. I posted them there because it has more people to weigh in on the topic.

Comment #13 Xano commented October 9, 2014 at 8:59am

As I said there I'm fine with fixing this in public. I thought it was a security issue, but I am by no means an expert. I just know the current UI and code are very confusing and don't seem to be in sync, and I'd like to fix this as soon as possible.