Issue #1204658 by chx: Fixed SA-CORE-2011-002 - Drupal core - Access bypass.

modules/node/node.module

@@ -3110,7 +3110,7 @@ function node_access_view_all_nodes($account = NULL) {
  * 'update' and 'delete').
  */
 function node_query_node_access_alter(QueryAlterableInterface $query) {
-  _node_query_node_access_alter($query, 'node', 'node');
+  _node_query_node_access_alter($query, 'node');
 }
 
 /**
@@ -3121,7 +3121,7 @@ function node_query_node_access_alter(QueryAlterableInterface $query) {
  * conditions are added for field values belonging to nodes only.
  */
 function node_query_entity_field_access_alter(QueryAlterableInterface $query) {
-  _node_query_node_access_alter($query, $query->getMetaData('base_table'), 'entity');
+  _node_query_node_access_alter($query, 'entity');
 }
 
 /**
@@ -3129,14 +3129,12 @@ function node_query_entity_field_access_alter(QueryAlterableInterface $query) {
  *
  * @param $query
  *   The query to add conditions to.
- * @param $base_table
- *   The table holding node ids.
  * @param $type
  *   Either 'node' or 'entity' depending on what sort of query it is. See
  *   node_query_node_access_alter() and node_query_entity_field_access_alter()
  *   for more.
  */
-function _node_query_node_access_alter($query, $base_table, $type) {
+function _node_query_node_access_alter($query, $type) {
   global $user;
 
   // Read meta-data from query, if provided.
@@ -3160,14 +3158,61 @@ function _node_query_node_access_alter($query, $base_table, $type) {
     return;
   }
 
+  $tables = $query->getTables();
+  $base_table = $query->getMetaData('base_table');
+  // If no base table is specified explicitly, search for one.
+  if (!$base_table) {
+    $fallback = '';
+    foreach ($tables as $alias => $table_info) {
+      if (!($table_info instanceof SelectQueryInterface)) {
+        $table = $table_info['table'];
+        // If the node table is in the query, it wins immediately.
+        if ($table == 'node') {
+          $base_table = $table;
+          break;
+        }
+        // Check whether the table has a foreign key to node.nid. If it does,
+        // do not run this check again as we found a base table and only node
+        // can triumph that.
+        if (!$base_table) {
+          // The schema is cached.
+          $schema = drupal_get_schema($table);
+          if (isset($schema['fields']['nid'])) {
+            if (isset($schema['foreign keys'])) {
+              foreach ($schema['foreign keys'] as $relation) {
+                if ($relation['table'] === 'node' && $relation['columns'] === array('nid' => 'nid')) {
+                  $base_table = $table;
+                }
+              }
+            }
+            else {
+              // At least it's a nid. A table with a field called nid is very
+              // very likely to be a node.nid in a node access query.
+              $fallback = $table;
+            }
+          }
+        }
+      }
+    }
+    // If there is nothing else, use the fallback.
+    if (!$base_table) {
+      if ($fallback) {
+        watchdog('security', 'Your node listing query is using @fallback as a base table in a query tagged for node access. This might not be secure and might not even work. Specify foreign keys in your schema to node.nid ', array('@fallback' => $fallback), WATCHDOG_WARNING);
+        $base_table = $fallback;
+      }
+      else {
+        throw new Exception(t('Query tagged for node access but there is no nid. Add foreign keys to node.nid in schema to fix.'));
+      }
+    }
+  }
+
   // Prevent duplicate records.
   $query->distinct();
 
-  // Find all instances of the {node} table being joined -- could appear
+  // Find all instances of the base table being joined -- could appear
   // more than once in the query, and could be aliased. Join each one to
   // the node_access table.
 
-  $tables = $query->getTables();
   $grants = node_access_grants($op, $account);
   if ($type == 'entity') {
     // The original query looked something like:

modules/node/node.test

@@ -968,6 +968,156 @@ class NodeAccessRecordsUnitTest extends DrupalWebTestCase {
   }
 }
 
+/**
+ * Tests for Node Access with a non-node base table.
+ */
+class NodeAccessBaseTableTestCase extends DrupalWebTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Node Access on any table',
+      'description' => 'Checks behavior of the node access subsystem if the base table is not node.',
+      'group' => 'Node',
+    );
+  }
+
+  /**
+   * Enable modules and create user with specific permissions.
+   */
+  public function setUp() {
+    parent::setUp('node_access_test');
+    node_access_rebuild();
+    variable_set('node_access_test_private', TRUE);
+  }
+
+  /**
+   * Test the "private" node access.
+   *
+   * - Create 2 users with "access content" and "create article" permissions.
+   * - Each user creates one private and one not private article.
+
+   * - Test that each user can view the other user's non-private article.
+   * - Test that each user cannot view the other user's private article.
+   * - Test that each user finds only appropriate (non-private + own private)
+   *   in taxonomy listing.
+   * - Create another user with 'view any private content'.
+   * - Test that user 4 can view all content created above.
+   * - Test that user 4 can view all content on taxonomy listing.
+   */
+  function testNodeAccessBasic() {
+    $num_simple_users = 2;
+    $simple_users = array();
+
+    // nodes keyed by uid and nid: $nodes[$uid][$nid] = $is_private;
+    $this->nodesByUser = array();
+    $titles = array(); // Titles keyed by nid
+    $private_nodes = array(); // Array of nids marked private.
+    for ($i = 0; $i < $num_simple_users; $i++) {
+      $simple_users[$i] = $this->drupalCreateUser(array('access content', 'create article content'));
+    }
+    foreach ($simple_users as $this->webUser) {
+      $this->drupalLogin($this->webUser);
+      foreach (array(0 => 'Public', 1 => 'Private') as $is_private => $type) {
+        $edit = array(
+          'title' => t('@private_public Article created by @user', array('@private_public' => $type, '@user' => $this->webUser->name)),
+        );
+        if ($is_private) {
+          $edit['private'] = TRUE;
+          $edit['body[und][0][value]'] = 'private node';
+          $edit['field_tags[und]'] = 'private';
+        }
+        else {
+          $edit['body[und][0][value]'] = 'public node';
+          $edit['field_tags[und]'] = 'public';
+        }
+
+        $this->drupalPost('node/add/article', $edit, t('Save'));
+        $nid = db_query('SELECT nid FROM {node} WHERE title = :title', array(':title' => $edit['title']))->fetchField();
+        $private_status = db_query('SELECT private FROM {node_access_test} where nid = :nid', array(':nid' => $nid))->fetchField();
+        $this->assertTrue($is_private == $private_status, t('The private status of the node was properly set in the node_access_test table.'));
+        if ($is_private) {
+          $private_nodes[] = $nid;
+        }
+        $titles[$nid] = $edit['title'];
+        $this->nodesByUser[$this->webUser->uid][$nid] = $is_private;
+      }
+    }
+    $this->publicTid = db_query('SELECT tid FROM {taxonomy_term_data} WHERE name = :name', array(':name' => 'public'))->fetchField();
+    $this->privateTid = db_query('SELECT tid FROM {taxonomy_term_data} WHERE name = :name', array(':name' => 'private'))->fetchField();
+    $this->assertTrue($this->publicTid, t('Public tid was found'));
+    $this->assertTrue($this->privateTid, t('Private tid was found'));
+    foreach ($simple_users as $this->webUser) {
+      $this->drupalLogin($this->webUser);
+      // Check own nodes to see that all are readable.
+      foreach ($this->nodesByUser as $uid => $data) {
+        foreach ($data as $nid => $is_private) {
+          $this->drupalGet('node/' . $nid);
+          if ($is_private) {
+            $should_be_visible = $uid == $this->webUser->uid;
+          }
+          else {
+            $should_be_visible = TRUE;
+          }
+          $this->assertResponse($should_be_visible ? 200 : 403, strtr('A %private node by user %uid is %visible for user %current_uid.', array(
+            '%private' => $is_private ? 'private' : 'public',
+            '%uid' => $uid,
+            '%visible' => $should_be_visible ? 'visible' : 'not visible',
+            '%current_uid' => $this->webUser->uid,
+          )));
+        }
+      }
+
+      // Check to see that the correct nodes are shown on taxonomy/private
+      // and taxonomy/public.
+      $this->assertTaxonomyPage(FALSE);
+    }
+
+    // Now test that a user with 'access any private content' can view content.
+    $access_user = $this->drupalCreateUser(array('access content', 'create article content', 'node test view', 'search content'));
+    $this->drupalLogin($access_user);
+
+    foreach ($this->nodesByUser as $uid => $private_status) {
+      foreach ($private_status as $nid => $is_private) {
+        $this->drupalGet('node/' . $nid);
+        $this->assertResponse(200);
+      }
+    }
+
+    // This user should be able to see all of the nodes on the relevant
+    // taxonomy pages.
+    $this->assertTaxonomyPage(TRUE);
+  }
+
+  protected function assertTaxonomyPage($super) {
+    foreach (array($this->publicTid, $this->privateTid) as $tid_is_private => $tid) {
+      $this->drupalGet("taxonomy/term/$tid");
+      $this->nids_visible = array();
+      foreach ($this->xpath("//a[text()='Read more']") as $link) {
+        $this->assertTrue(preg_match('|node/(\d+)$|', (string) $link['href'], $matches), 'Read more points to a node');
+        $this->nids_visible[$matches[1]] = TRUE;
+      }
+      foreach ($this->nodesByUser as $uid => $data) {
+        foreach ($data as $nid => $is_private) {
+          // Private nodes should be visible on the private term page,
+          // public nodes should be visible on the public term page.
+          $should_be_visible = $tid_is_private == $is_private;
+          // Non-superusers on the private page can only see their own nodes.
+          if (!$super && $tid_is_private) {
+            $should_be_visible = $should_be_visible && $uid == $this->webUser->uid;
+          }
+          $this->assertIdentical(isset($this->nids_visible[$nid]), $should_be_visible, strtr('A %private node by user %uid is %visible for user %current_uid on the %tid_is_private page.', array(
+            '%private' => $is_private ? 'private' : 'public',
+            '%uid' => $uid,
+            '%visible' => isset($this->nids_visible[$nid]) ? 'visible' : 'not visible',
+            '%current_uid' => $this->webUser->uid,
+            '%tid_is_private' => $tid_is_private ? 'private' : 'public',
+          )));
+        }
+      }
+    }
+  }
+}
+
 /**
  * Test case to check node save related functionality, including import-save
  */

modules/node/tests/node_access_test.module

@@ -12,8 +12,10 @@
  */
 function node_access_test_node_grants($account, $op) {
   $grants = array();
+  // First grant a grant to the author for own content.
+  $grants['node_access_test_author'] = array($account->uid);
   if ($op == 'view' && user_access('node test view', $account)) {
-    $grants['node_access_test'] = array(888);
+    $grants['node_access_test'] = array(8888);
   }
   if ($op == 'view' && $account->uid == variable_get('node_test_node_access_all_uid', 0)) {
     $grants['node_access_all'] = array(0);
@@ -26,14 +28,27 @@ function node_access_test_node_grants($account, $op) {
  */
 function node_access_test_node_access_records($node) {
   $grants = array();
-  $grants[] = array(
-    'realm' => 'node_access_test',
-    'gid' => 888,
-    'grant_view' => 1,
-    'grant_update' => 0,
-    'grant_delete' => 0,
-    'priority' => 999,
+  // For NodeAccessBaseTableTestCase, only set records for private nodes.
+  if (!variable_get('node_access_test_private') || $node->private) {
+    $grants[] = array(
+      'realm' => 'node_access_test',
+      'gid' => 8888,
+      'grant_view' => 1,
+      'grant_update' => 0,
+      'grant_delete' => 0,
+      'priority' => 0,
     );
+    // For the author realm, the GID is equivalent to a UID, which
+    // means there are many many groups of just 1 user.
+    $grants[] = array(
+      'realm' => 'node_access_test_author',
+      'gid' => $node->uid,
+      'grant_view' => 1,
+      'grant_update' => 1,
+      'grant_delete' => 1,
+      'priority' => 0,
+    );
+  }
 
   return $grants;
 }
@@ -142,3 +157,62 @@ function node_access_entity_test_page() {
 
   return $output;
 }
+
+/**
+ * Implements hook_form_node_form_alter().
+ */
+function node_access_test_form_node_form_alter(&$form, $form_state) {
+  // Only show this checkbox for NodeAccessBaseTableTestCase.
+  if (variable_get('node_access_test_private')) {
+    $form['private'] = array(
+      '#type' => 'checkbox',
+      '#title' => t('Private'),
+      '#description' => t('Check here if this content should be set private and only shown to privileged users.'),
+      '#default_value' => isset($form['#node']->private) ? $form['#node']->private : FALSE,
+    );
+  }
+}
+
+/**
+ * Implements hook_node_load().
+ */
+function node_access_test_node_load($nodes, $types) {
+  $result = db_query('SELECT nid, private FROM {node_access_test} WHERE nid IN(:nids)', array(':nids' => array_keys($nodes)));
+  foreach ($result as $record) {
+    $nodes[$record->nid]->private = $record->private;
+  }
+}
+
+/**
+ * Implements hook_node_delete().
+ */
+
+function node_access_test_node_delete($node) {
+  db_delete('node_access_test')->condition('nid', $node->nid)->execute();
+}
+
+/**
+ * Implements hook_node_insert().
+ */
+function node_access_test_node_insert($node) {
+  _node_access_test_node_write($node);
+}
+
+/**
+ * Implements hook_nodeapi_update().
+ */
+function node_access_test_node_update($node) {
+  _node_access_test_node_write($node);
+}
+
+/**
+ * Helper for node insert/update.
+ */
+function _node_access_test_node_write($node) {
+  if (isset($node->private)) {
+    db_merge('node_access_test')
+      ->key(array('nid' => $node->nid))
+      ->fields(array('private' => (int) $node->private))
+      ->execute();
+  }
+}