Observed that while trying to create a project , project issue drop down is listing all the project irrespective of the user permission on the project .

CommentFileSizeAuthor
#20 project-node_access_restrictions_followup-1890906.patch5.94 KBMustangGB
#2 18ef9b14ebb9360f9114d2b4c69ef221d72724a4.patch1.25 KBjosejayesh
Screen Shot 2013-01-17 at 12.37.44 PM.png19.86 KBjosejayesh
Screen Shot 2013-01-17 at 12.37.19 PM.png24.51 KBjosejayesh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

josejayesh’s picture

josejayesh CreditAttribution: josejayesh commented

By modifying the following function , we can fix this issue

File name = sites/all/modules/contrib/project/project.module

function project_projects_select_options(array $conditions = array()) {
  $projects = array();
  global $user;

  $project_node_types = project_project_node_types();
  if (empty($project_node_types)) {
    // If there are no project node types, just return the empty array here.
    return $projects;
  }

  $query = new EntityFieldQuery();
  $query->entityCondition('entity_type', 'node', '=');

  // Restrict the query to the node types that can be projects.
  $query->propertyCondition('type', $project_node_types);

  foreach ($conditions as $condition) {
    $query->fieldCondition($condition['field'],
      isset($condition['column']) ? $condition['column'] : 'value',
      isset($condition['value']) ? $condition['value'] : '1',
      isset($condition['operator']) ? $condition['operator'] : '='
    );
  }

  $result = $query->execute();
  foreach ($result as $entity_type => $project_entities) {
    $project_entities = entity_load($entity_type, array_keys($project_entities));
    foreach ($project_entities as $project) {
      $node = node_load($project->nid);
      //checking whether logged in user has the node access 
      if (node_access("view", $node, $user) === TRUE){
	      $project_entity_ids = entity_extract_ids('node', $project);
	      $projects[$project_entity_ids[0]] = $project->title;
      }
    }
  }
  return $projects;
}

Please verify

josejayesh’s picture

josejayesh CreditAttribution: josejayesh commented
FileSize
18ef9b14ebb9360f9114d2b4c69ef221d72724a4.patch1.25 KB
dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Title: Project Submit Issue page is listing down all the projects irrespective of the user permission on that page » Information disclosure: Project* is not properly allowing node_access to work on listing queries in D7
Priority: Normal » Critical

I haven't verified, but if this is true, it's an information disclosure security bug, which should normally be submitted to security.drupal.org not here. However, since the D7 version of this code hasn't been officially released yet, we can deal with this here publicly.

We don't have the same bug in the D6 version since we use db_rewrite_sql() for the query generating this listing, and that handles node access checks for us.

In D7, we're just using EntityFieldQuery to build the query for this, and from what I can tell, deep down that calls db_select() to create a dynamic query, which is how you're supposed to handle node access for listing queries in D7 and later.

If this really is a bug in Project* we need to fix it, but not by individually loading each project node and calling node_access() on it. That would be a gigantic number of queries on a site the size of Drupal.org with over 23K project nodes (and counting).

OH, I see the problem. For node access to work for altering dynamic queries, we need to add a 'node_access' tag to our query before we execute it. Wow, I'm ashamed to admit I didn't already know that. ;) Whoops. We need to go through the whole Project* D7 code base and audit all our listing queries to make sure we're adding something like this to all of them:

  $query->addTag('node_access');
drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

http://api.7.devdrupal.org/api/drupal/includes%21entity.inc/class/Entity... says

Also note that this query does not automatically respect entity access restrictions. Node access control is performed by the SQL storage engine but other storage engines might not do this.

The mechanism for this appears to be modules/field/modules/field_sql_storage/field_sql_storage.module:

      if (!isset($query->tags['DANGEROUS_ACCESS_CHECK_OPT_OUT'])) {
        $select_query->addTag('entity_field_access');
      }

And in modules/node/node.module

function node_query_entity_field_access_alter(QueryAlterableInterface $query) {
  _node_query_node_access_alter($query, 'entity');
}

Project module does assume standard SQL storage in a few places, so I think it may be okay to rely on this. node.module itself does use ->addTag('node_access') so it wouldn't hurt to use it ourselves.

josejayesh, are you using any special field storage?

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

Thanks for the research, drumm.

Project module does assume standard SQL storage in a few places

Where's that? I wasn't aware of any such assumptions (other than the d.o Project* migration code). I *thought* we were being good citizens and using the proper APIs as necessary. If we're not, I'd consider that a bug we should fix (obviously in a separate issue).

Thanks,
-Derek

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

At some point last year we became more sloppy about shortcuts to just get Drupal.org upgraded. A few things that stick out from git grep -nE 'db_(select|query).*node':

usage/includes/pages.inc:124:  $query = db_select('node', 'n')
usage/includes/pages.inc:245:  $query = db_select('node', 'n')
release/metrics/ProjectReleaseMetric.class.php:27:    $projects = db_query("SELECT DISTINCT(nn.nid) FROM {node} n ...

This does look a lot better than I had assumed.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

Discussion of SQL field storage is now at #1894404: Project* should not assume SQL field storage. ;)

However, the point that's still relevant to this thread is this:

I was under the impression that even if you're using a non-SQL field storage engine, that {node} itself was always supposed to live in SQL. So, I'm not sure these are even examples of assuming SQL field storage.

So, it still seems like we want to add the 'node_access' tag to all queries we're generating that should be respecting node access, whether those are directly querying {node}, are coming from EntityFieldQuery, or whatever. Right?

Meta: this issue makes me scared. A couple of very senior members of the Drupal community, both of us who sit on the security team, can't easily figure out how to make node access work during a D7 port. Yikes. ;) Seems like a combination of things getting too complicated for their own good, coupled with insufficient documentation...

-Derek

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

So, it still seems like we want to add the 'node_access' tag to all queries we're generating that should be respecting node access, whether those are directly querying {node}, are coming from EntityFieldQuery, or whatever. Right?

Sounds good. Looks like core does this, and we should too.

josejayesh’s picture

josejayesh CreditAttribution: josejayesh commented

#4 @drumm i am not using any special field storage.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Project: Project issue tracking » Project

I went ahead and committed the addTag for the original problem in this issue: http://drupalcode.org/project/project.git/commitdiff/e09ddcd?hp=5eb46db3...

josejayesh - can you confirm that it works for you?

(Leave the issue open, there are other places this needs to be added.)

josejayesh’s picture

josejayesh CreditAttribution: josejayesh commented

It is working for me .Thanks drumm.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

@josejayesh: Excellent, glad to hear it. ;)

@drumm: Thanks for getting the conversion started!

Cheers,
-Derek

josejayesh’s picture

josejayesh CreditAttribution: josejayesh commented

@drumm + dww
Whether we need to apply $query->addTag('node_access'); every where in the project module or only in the 'function project_projects_select_options'

Thanks
~Jayesh

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

@josejayesh: Not just project_projects_select_options(), no. But not necessarily everywhere, either. We need to review all the queries and see which ones need to respect node access. Generally, anything touching {node} needs this tag, although not necessarily (e.g. internal queries to compute results that aren't going to be directly displayed to end users, etc). That's why drumm left the issue active, since there's still (a lot?) more work to do here.

Thanks,
-Derek

josejayesh’s picture

josejayesh CreditAttribution: josejayesh commented

@Derek thanks for the update . I will try to review the complete project module , once it is done i'll post my findings here .

Thanks
~Jayesh

josejayesh’s picture

josejayesh CreditAttribution: josejayesh commented

Guys
I am not observing information disclosure security issue any where else

Thanks
~Jayesh

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

Great, thanks for the update! I still think we should just grep the code and make sure there aren't any places we're missing, but it's good to hear you haven't found anything yet yourself.

Cheers,
-Derek

hass’s picture

hass CreditAttribution: hass commented

Re #7:

Meta: this issue makes me scared. A couple of very senior members of the Drupal community, both of us who sit on the security team, can't easily figure out how to make node access work during a D7 port. Yikes. ;) Seems like a combination of things getting too complicated for their own good, coupled with insufficient documentation...

I fully support this... :-))). Nearly zero documentation, no useful examples...

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

FYI: sort of related but separate bug (now fixed): #1950484: Not checking node access on the project node when creating new issues

MustangGB’s picture

MustangGB CreditAttribution: MustangGB commented
Issue summary: View changes
Status: Active » Needs review
FileSize
project-node_access_restrictions_followup-1890906.patch5.94 KB

No sanity checking or testing has been done, I just greped and added tags.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs review » Needs work

Some sanity checking is needed. For example, ….drush.inc should not do access checking; if something is running packaging, it should be able to access everything.

  • dww committed 8ef1969 on 7.x-2.x 
    [#1890906] by dww, MustangGB: Enforce node_access in list queries.
dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Assigned: Unassigned » dww
Status: Needs work » Fixed

I took a new stab at this. Unfortunately, the patch from #20 was wrong in almost all cases:

  • Various internal APIs do *not* want to be enforcing node_access.
  • The drush commands also do not want to enforce node_access.
  • In a few places, #20 was adding the tag to queries that already had the tag.
  • In one case (project_release_query_releases_by_branch()), there's an argument to the function that determines if we want to enforce access or not, and we need to only add the node_access tag to the query if that function argument says so.
  • To validate a project title, we have to enforce uniqueness over the potential info disclosure. I spoke to drumm @ the DrupalCon LA sprint about this, and we both agreed that even though it's still technically info disclosure, we have no alternative but to have the validation fail on a non-unique project title (since so many other things would break).

In the commit I pushed, I tried to add code comments in the places where we don't want node_access tagging. I also commented on the places where we really do want the node_access, and why. Hopefully it's clear.

Thanks everyone!
-Derek

Status: Fixed » Closed (fixed)

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