Enforce node access permissions - SA-CONTRIB-2015-104

According to the stats, around 16,000 sites are still using this module. Do you want to follow option 2 of the module unsupported notice?

"File an issue in the queue with a patch to fix the module and then contact the security team to have your version reviewed and the project handed over to you following the unsupported project process."

Thank you!

I was the original reporter of this issue to the security team.

Here's a patch for Drupal 7 I included along with my report (and a Drupal 6 backport of it by benjy).

This uses a different technique than the patch above - it modifies the database query directly to respect node access there.

One other difference is that my patch does not modify _ddblock_get_content_array() also, but the patch posted here does. I haven't looked into it to see if that's necessary or not.

Hope this helps...

Posted patches on the original issue

So David Rothstein's patches are the ones to use? Where's the original issue?

Patches which were posted in the original issue:

Are these patches going to be committed so that the module isn't flagged as unsupported anymore? I'd rather wait until that happens to modify code further.

@ edminn As soon as the issue is reviewed & tested by community the patches will be comitted

Tested, and working fine for me. #9

So patch at #9 should be committed to 7.x-1.x, right?

Unfortunately, I don't have command line access for this client. Is there an alternate method for applying the patches?

Patch looks good to me too. Let me know when you create the security release and I'll remove the red banner from the project page, publish the release and update the advisory.

Created releases and updated the advisory

Created releases and updated the advisory

Confused because still, no commits are shown.

I've checked that the commits went into the release and published them. Everything should be ok now.

Thanks

I don't see them at https://www.drupal.org/node/293705/commits nor as comments in this issue.
I don't see them in the 7.x-1.x log in git: http://cgit.drupalcode.org/ddblock/log/ddblock.module?h=7.x-1.x

From the summary page at http://cgit.drupalcode.org/ddblock/ I do see new tags that seem to be associated with the commit.

So, it seems like there's a bit of cleanup necessary to ensure that these patches will be in future 7.x-1.x releases.

I don't see them at https://www.drupal.org/node/293705/commits nor as comments in this issue.
I don't see them in the 7.x-1.x log in git: http://cgit.drupalcode.org/ddblock/log/ddblock.module?h=7.x-1.x

From the summary page at http://cgit.drupalcode.org/ddblock/ I do see new tags that seem to be associated with the commit.

So, it seems like there's a bit of cleanup necessary to ensure that these patches will be in future 7.x-1.x releases.

The patch went into the 7.x-1.1 release, I checked the source before publishing it.

Probably the tag was pushed but not the commits.

Actually, this only pushes the tags, not the branches:
git push --tags

This should be done instead:
git push
git push --tags

Maybe we should consider option 1 when fixing a vulnerability in private. That way the commits would be less discoverable (the branches could be pushed a bit later, after the release and the advisory have been published)

Yes, I agree it appears to be in the release. Thanks for confirming it!

Strange that two days after 6.x-1.0-rc7 has been released, the update module still reports that the installed version 6.x-1.0-rc6 is not supported and that the module should be removed, instead of recommending updating to rc7.

@roball there's a bug in updates.drupal.org right now - should be fixed in a few days. I asked someone to clear the cache on this url manually to help get this update out.

Thank you @greggles, now 6.x-1.0-rc7 came through the update process.

6.x-1.0-rc7 is completely messed up! If you update 6.x-1.0-rc6 to rc7, the dynamic blocks will no longer work! Thus, I had to revert to rc6 and manually apply the patch that has been applied with pushing the tag 6.x-1.0-rc7:

[root@server modules]# cd ddblock
[root@server ddblock]# wget -O SA-CONTRIB-2015-104.patch http://cgit.drupalcode.org/ddblock/patch/?id=52cf04de9104da8231f2091b2452d799c62e769c
[root@server ddblock]# patch -p1 < SA-CONTRIB-2015-104.patch

This is unlikely to have happened by the patch. Probably the patch was applied to a branch that had many other changes and was not fully tested, that's unfortunate. Maybe an easy way to fix that would be creating a 6.x-1.0-rc8 release just applying the patch against 6.x-1.0-rc6 code?

Anyway, please can you create a follow-up and post the link here? So we can close this.

I agree to release 6.x-1.0-rc8 which just has the patch applied against 6.x-1.0-rc6 code. Follow-up issue created at #2506573: 6.x-1.0-rc7 is broken. Thanks.

So, it seems like there's a bit of cleanup necessary to ensure that these patches will be in future 7.x-1.x releases.

Yes, for Drupal 6 too.

Running either of these:

git diff 7.x-1.x..7.x-1.1
git diff 6.x-1.x..6.x-1.0-rc7

shows that the patch is not present on 6.x-1.x or 7.x-1.x, only on the latest tagged releases.

I don't know how to create a new release from the rc6 version of the module, see my question at:
#2506573: 6.x-1.0-rc7 is broken

From comment #31, I understand I should first update the changes to 6.x-1.x and 7.x-1.x and then create a new release from dev.

I think there where changes in the 6.x-1.x version a long time ago and I mistakenly adjusted the code to that version to create the rc7 release.

Will try to find out what are the difference in the different branches and tags and update code when I know how to do it the good way.

Created the new rc8 release.

I just did a git pull and the issue is still present in the latest code on the 6.x-1.x and 7.x-1.x branches. You basically just need to commit the patches to the 6.x-1.x and 7.x-1.x branches directly - then it should be all set for any future release.

ppblaauw has created a 6.x-1.x-temp branch, which contains the 6.x-1.x commits up to the (last working) 6.x-1.0-rc6 release, and the patch directly applied to that, tagged and released as 6.x-1.0-rc8. Looks good to me. Maybe the current 6.x-1.x branch should be renamed to 6.x-2.x and 6.x-1.x-temp to 6.x-1.x, so the fix is actually also in 6.x-1.x. The new features committed to 6.x-1.x years ago could then be developed further as 6.x-2.x (if that ever happens).

The patch in #9 fails for our case because of ambiguity in the sql statement.
Upgraded to rc8 and ddblocks stopped showing.

The line:
$sql = db_rewrite_sql("SELECT n.nid FROM {node} n WHERE status=1 AND type='%s' AND nid='%s'");

Has ambiguiity in the last WHERE clause.
Error in log:
Column nid in where clause is ambiguous query: SELECT DISTINCT n.nid FROM node n INNER JOIN node_access na ON na.nid = n.nid WHERE (na.grant_view >= 1 AND ((na.gid = 0 AND na.realm = 'all') OR (na.gid = 0 AND na.realm = 'content_access_author') OR (na.gid = 1 AND na.realm = 'content_access_rid'))) AND ( status=1 AND type='home_carousel' AND nid='61') in ****/modules/ddblock-6.x-1.0-rc8/ddblock.module on line 1917.

It should be
$sql = db_rewrite_sql("SELECT n.nid FROM {node} n WHERE status=1 AND type='%s' AND n.nid='%s'");

This could be because of the extra modules were using for access control.
However, if you are going to alias {node} as "n" then you should refer to its child fields by the same alias in the query, thus n.nid