An SA was just published for views:

https://www.drupal.org/node/2854980

The D6LTS vendors backported the patch to D6 which is attached

CommentFileSizeAuthor
#23 views-SA-CONTRIB-2017-022-23.patch8.4 KBcatch
#20 2855030-20.patch8.72 KBjoelpittet
#17 views-db_rewrite_sql.2855030.patch4.51 KBmr.j
#16 2855030-16-name-ambiguous.patch908 bytesjoelpittet
#6 views-SA-CONTRIB-2017-022.patch4.31 KBcatch
views-SA-CONTRIB-2017-022.patch4.19 KBdsnopek
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dsnopek created an issue. See original summary.

dsnopek credited catch.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Fixed

Committed to repo!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented

Thanks for fixing up the patch and posting here!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Looks good for me!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented
Status: Fixed » Needs review
FileSize
views-SA-CONTRIB-2017-022.patch4.31 KB

Two bugs with this, one is trivial, one not so much:

1. db_rewrite_sql() has an $args parameter - dsnopek fixed my original patch which stopped passing $args to db_query(), but they should technically be passed to db_rewrite_sql() too. However I couldn't find a single implementation of db_rewrite_sql() that actually uses that param.

2. More importantly the original views queries are often written like SELECT td.* FROM term_data WHERE tid = %d. This is OK when not rewritten, but tac_lite adds a join, and you end up with 'ambiguous column tid in query' errors, needs to be td.tid = %d instead.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

2. More importantly the original views queries are often written like SELECT td.* FROM term_data WHERE tid = %d. This is OK when not rewritten, but tac_lite adds a join, and you end up with 'ambiguous column tid in query' errors, needs to be td.tid = %d instead.

Ah, crap :-/

However, the patch in #6 doesn't appear to contain the 'tid' to 'td.tid' change, only the extra args from point 1

Jeremy’s picture

Jeremy CreditAttribution: Jeremy at Tag1 Consulting commented

The fix is after the WHERE (you can't fix it in the SELECT as modules often check things like 'n' or 'td' etc).

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

Er, actually, I'm not sure if changing 'tid' to 'td.tid' is right. And in re-reading #6 I can see that you're not actually suggesting that, that was just my misinterpretation. :-)

But here's why that isn't right: looking at some examples of hook_db_rewrite_access(), any that deal with taxonomy are doing if ($field == 'tid') to see if the query deals with taxonomy. So, I think we need to actually fix the query not to use "SELECT td.*"

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

Hm, and looking at the tac_lite code (I haven't actually tested), it shouldn't have this problem:

    $join = "LEFT JOIN {term_data} tac_td ON $primary_table.tid = tac_td.tid";
    $where = "$primary_table.tid IN (" . implode(', ', $tids) .
      ") OR tac_td.vid NOT IN (" . implode(',', $vids) .")";

http://cgit.drupalcode.org/tac_lite/tree/tac_lite.module?h=6.x-1.x#n541

The "$primary_table.tid" should turn into "td.tid" and be not ambiguous, right?

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

Oh, nevermind, please ignore my comments. I now see where the fix is. Thanks, @Jeremy!

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Reviewed & tested by the community

Ok, I tested with tac_lite, was able to reproduce the problem, and tested the patch #6 which fixes the problem. Marking RTBC!

@catch: is there any more testing you want to do before committing this to the repo?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented

@dsnopek I tested with tac_lite and confirmed it fixed both the security issue (404 instead of showing term with empty listing) and didn't throw the warning. I had to head scratch a bit on exactly where the bug was as well.

I think it's ready to go, my testing wasn't 100% comprehensive but definitely caught one place and it's the same change on each query so I think it's OK.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Reviewed & tested by the community » Fixed

Ok, great, thanks! I've committed the new patch to the repo

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented
FileSize
2855030-16-name-ambiguous.patch908 bytes

Not sure if this is related but started seeing this in the logs and relates to tid mentions above being ambiguous.

Column 'name' in field list is ambiguous query: SELECT name FROM term_data td LEFT JOIN term_data tac_td ON td.tid = tac_td.tid WHERE (td.tid IN (0, 0, 0) OR tac_td.vid NOT IN (2)) AND ( td.tid IN (4031)) in views_handler_argument_term_node_tid.inc on line 43.

Attached is a patch.

mr.j’s picture

mr.j CreditAttribution: mr.j as a volunteer commented
FileSize
views-db_rewrite_sql.2855030.patch4.51 KB

Yes this patch broke a few views for me, all of which use taxonomy filters or arguments. There are a few unprefixed "SELECT *" statements that fail when passed to db_rewrite_sql when using a node access module (in my case forum_access) as it can add more tables to the query and you get ambiguous column name errors. The previous patch on #16 doesn't catch all the potential errors. I made a github pull request with patches attached to this post here:
https://github.com/d6lts/views/pull/2/files

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented

Good find, thanks for posting the patch.

There was another issue in the original patch, found by Fabianx. It shouldn't be a bug in practice, but if we're going to update the patch, better to do it once:

- Core uses 't', 'tid'
- The patch uses 'td', 'tid'

Neither tac_lite nor taxonomy_access (the two modules I am aware of) check the table name though.

i18ntaxonomy does check for 't' or 'v' in table, but obviously that is not security critical, but is how I noticed the mismatch.

### Examples

Core:

- https://api.drupal.org/api/drupal/modules%21taxonomy%21taxonomy.module/f...

The docs for db_rewrite_sql() only mention using the full table names vs. the 'usual table aliases' (all one-letter). It's not a requirement but there's the potential for it being used as an implicit API if core's consistent.

Going to mark CNW for that, it's just a change from td to t everywhere for the alias.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Closed (fixed) » Needs work
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented
Status: Needs work » Needs review
FileSize
2855030-20.patch8.72 KB

Addressing 'td' being 't' from @catch.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

The code changes in this look great! I haven't yet had chance to actually test this, though, which is the big thing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented

I also haven't tested this yet, but visually it looks great.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community
FileSize
views-SA-CONTRIB-2017-022-23.patch8.4 KB

#20 is an interdiff against #6.

Here's a combined patch that will apply against views-6.x-3.2

Tested fine for me, so moving to RTBC.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

RTBC +1

It took me way too long to reproduce the bug, but I was finally able to using forum_access and a View that had and exposed filter for TID using the autocomplete. And the patch fixed it!

Thanks, Everyone!

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Reviewed & tested by the community » Fixed

Committed!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.