file_file_download() passed bogus $field to field_access()

Ew. This is bad :)

Confirmed, the part of the code of file_file_download() that handles field-level permission is just totally bogus.

Subscribe

Subscribe

Can some one test this patch I came up with for this issue?
http://drupal.org/node/1182338#comment-4868078

Here is the patch. It applies cleanly to 7.7 and 7.x-dev.

Here is a slightly better version that could prevent a null from being passed to hook_field_access.

Seem like when we originally assign $field, that could just use $field = field_info_field($field_name) rather than assigning it twice?

It looks like the function could be cleaned up a bit otherwise too, but not sure if we should do that here or open up another issue.

works fine!

Needs work per #8.

This should be what catch was suggesting in #8.

~

#11 works for me.

#11 works for me

#11 works for me to fix issue described here: http://drupal.org/node/1182338 - where, private file fields were not viewable by other organic group members.

Thanks!

I'm sure it will be fixed. I've noticed that the general procedure is to first fix the issue for Drupal 8 and then backport to D7. So now the question is how is that Backport going? Does anybody have a patch for Drupal 7? Thanks!

Patch #11 is for Drupal 8. Is there a version for Drupal 7.8? Thanks

Let's get a test for this.

The #11 seems to cause the following error in Drupal 7.9.
EntityMalformedException: Missing bundle property on entity of type node. in entity_extract_ids() (line 7405 of /home4/bawiecco/public_html/drupal-7.9/includes/common.inc).
It worked fine in 7.8, but some changes in the new version are causing this error.

After Entity API, Views and cTools (among others) got updated errors have disappeared and patch applies correctly again.
Lets get this one tested out. :)

Ended up here from: http://drupal.org/node/1182338

Path #11 solved the issue for me in Drupal 7.9

Same here, in Drupal 7.10. Can we backport this to Drupal 7 as well?
The patch in #11 solved the problem for me.

Here is a rerolled patch with a test-only version to show that the bug is actually captured. It applies just fine to D7 with -p2.

The patches in #25 are not getting tested, uploading them again.

Still postponed :(

Trying to post the patches separately.

Re-uploading the patches will not help, unfortunately. 8.x tests failed to run earlier. I'll requeue the last patch if needed, but there is a backlog at the moment (and the more patches we add, the bigger the backlog). :)

@xjm:

When I first uploaded the patches the queue was empty, thus I thinked about a problem while forwarding them to the testbot. I did not spot the HEAD was failing, sorry.

Anyway, the test results are ok, so if the test looks good this should be RTBC. Code reviews welcome :)

Yep, this looks RTBC to me. Sneaking in a docs fix here--I clarified the inline comment and fixed a grammatical error. No commit credit please.

The interdiff I meant to attach in #32 in place of that second copy of the patch. Sorry testbot. ;)

Committed/pushed to 8.x. Moving back to 7.x for backport.

Nearly.

Just applied #32 with -p2 and rerolled. Back to RTBC.

So... I unpublished this issue for a while, because the security team was already in the process of working on a fix for the same problem in Drupal 7. (It turns out that under some circumstances, this bug led to security issues.) I didn't notice there was an issue here for it also until after it had been committed to Drupal 8.

Now that the fix is in Drupal 7 (http://drupal.org/node/1425084), there is some work to do for Drupal 8:

1. The Drupal 8 patch committed here actually made an API change to hook_file_download_access() and hook_file_download_access_alter(). We avoided doing that in the Drupal 7 security release, but rather just fixed the hook documentation so that it correctly documented what had always been passed in. Here, we need to decide what we actually do want to pass in for Drupal 8, and write a change notification if the API is going to change... In the security team discussion (which a few people here participated in) there seemed to be some thought that maybe $field wasn't the correct thing to pass in here, but rather the field item. Maybe we should just pass both?
2. We should make sure the terminology is precise everywhere ("field" vs "field item" vs "file item"), since that confusion is basically what led to this bug in the first place.
3. There's a test that was committed to Drupal 7 that should be forward-ported to Drupal 8.
4. When we're done, we should probably backport the test that was added here (or some version of it) to Drupal 7, since it's complementary to the one that already went in with the security fix.

Although I don't think there's a security issue in Drupal 8 anymore, the above tasks probably add up to critical on their own, so I'm marking this issue as such.

For reference, I'm attaching a copy of the patch that was already committed to Drupal 7.

Hmm, I was bitten by the confusion when porting the issues in #1425330: Apply Aggregator and OpenID fixes from DRUPAL-SA-CORE-2012-001

tagging Security Advisory follow-up

There is another issue to be dealt with here:

- drupal_alter() now only accepts exactly 3 arguments, we have 4 which means that the alter hook actually never receives the fourth argument and any attempt in implementing that hook as he is documented will fail with errors.

The only way out there is to convert $entity_type and $entity into a single $context array with the two variables as array values.

@berdir -- yep, and I found this independently and created #1143460: hook_file_download_access_alter missing entity argument.

There's also #1462538: Change drupal_alter() to use only one context variable --- which makes sense as well.

I'm not sure if it's better to work here or in one of the other issues. either way we should pick one place and redirect the other issues to the correct location.

i can't get d8 to install atm to test it out, but I'm thinking something like the attached.

Tagging.

Since this is where the history and activity is, reposting dave reid's patch here (and marked the other issue as a dupe).

Merged the patches from #44 and #37 and went a step further. I merged all additional things into a single $context variable (entity, entity_type, field_info, field_item) for both hooks.

The advantage is that the both are almost equal now (except the $grants argument in the alter hook, obviously).

It's a bigger change this way, though.

Let's try that again.

Another one that I missed.

I'm not able to implement my own hook ;).

Ok, this one should now be green. Sorry for spaming the issue.

#53: single_context5.patch queued for re-testing.

Feedback. Anyone? ;)

Re-rolled.

I read the comments, looked at the tests and the approach. Looks fine to me. Marking this rtbc.

Patch generally looks fine, but a couple of questions:

- I'm assuming we don't need the $entity_type parameter once all entities are classed objects, that's very close with only files left, so could we even remove it now?

- while drupal_alter() takes two $context arguments, that's a heavily loaded term in Drupal, would be good to think of another parameter name if possible (although no single word springs to mind to include entities, fields and field items). Also if we remove the $entity_type then can we actually skip the $context change in general?

We can't remove it now because the file entity isn't converted yet which is blocked on the other file issue that doesn't get proper reviews for unknown reasons.

/me is raising pressure :p

Yes, the removal of entity_type wold make this unnecessary. What would you suggest then, postponing until the the file conversion patch is in? EDIT: Actually, to include the field *and* field item, we'd still need $context.

However, we'd still need to do something about 7.x. The only fix that would not break backwards compatibility would be doing a manual $function() call with the given arguments instead of using drupal_alter(). I can't think of anything else.

1. + *   - field: The field info of the field the field_item belongs to.
   + *   - field_item: The field item that is being requested.
   

   This "field item" terminology looks like a bit of a regression compared to what was committed in the D7 security patch. I think we should use something more like "file item" as was done there (to make it easier to understand the difference between the two variables); e.g., where we had:

   * @param array $file_item
   *   The array of information about the file to check access for.
   

2. The D7 patch also contained the following change, not present in the above patch:

   -        // Check that $entity and $field were loaded successfully and check if
   -        // access to that field is not disallowed. If any of these checks fail,
   -        // stop checking access for this reference.
   -        if (empty($entity) || empty($field) || !field_access('view', $field, $entity_type, $entity)) {
   +        // Check that $entity, $field and $field_item were loaded successfully
   +        // and check if access to that field is not disallowed. If any of these
   +        // checks fail, stop checking access for this reference.
   +        if (empty($entity) || empty($field) || empty($field_item) || !field_access('view', $field, $entity_type, $entity)) {
   

   To be honest I'm not sure how much it matters, but for consistency we should probably include it in D8 too.

3. However, we'd still need to do something about 7.x. The only fix that would not break backwards compatibility would be doing a manual $function() call with the given arguments instead of using drupal_alter(). I can't think of anything else.

   Not sure I understand this comment... Besides possibly backporting the D8 test, what is left to change in D7, given that the security fix already went out?

afaik, d7 is still broken as described by berdir above. And since the argument that's never passed is the $entity itself, besides the errors, it really hampers doing any kind of entity based access control for files.

If we pass the $entity, then isn't the $field_item included as part of that anyway?

@catch It is included, but you don't know which one it is, if the entity has multiple items for the given field.

Changing to file item would be fine with me, I thought that field item was a common term for this. The problem that I'm seeing with file item is it's a new term (and not the same thing as file object/entity as it is an array and not a full file entity. there is, e.g., no uri)

I prefer 'field item' here, since that's actually what it is.

Makes sense with field item not knowing which one it is, that's annoying but don't see a way around it.

In that case it feels like the only remaining thing is to figure out whether we need this in 8.x:

+        // Check that $entity, $field and $field_item were loaded successfully
+        // and check if access to that field is not disallowed. If any of these
+        // checks fail, stop checking access for this reference.
+        if (empty($entity) || empty($field) || empty($field_item) || !field_access('view', $field, $entity_type, $entity)) {

If we keep "field item", let's at least improve the documentation to make clear what it is; after all, confusion between $field and $field_item is pretty much the entire source of this bug.

On the security.drupal.org discussion, @dww argued (pretty convincingly I think) that "field item" was confusing here because what you receive in this variable has nothing to do with the field it came from at all; the information is limited to the file itself. Which is how we wound up with the description I quoted above for D7 currently (which makes clear this is an array of information about the file, not an array of information about the field).

afaik, d7 is still broken as described by berdir above. And since the argument that's never passed is the $entity itself, besides the errors, it really hampers doing any kind of entity based access control for files.

Thanks, sorry I missed that above. Maybe after this issue is fixed in D8 it would be a good idea to reopen #1143460: hook_file_download_access_alter missing entity argument for D7 and deal with that separately there? (I'm almost wondering if simply adding a $context3 parameter to drupal_alter() isn't the best way to solve this in D7.)

A field consists of field items which contain whatever is defined in the field schema (and attached during load etc.). That is IMHO standard terminology, see for example http://api.drupal.org/api/drupal/modules%21field%21field.module/function....

file item indicates that it is a file object, which it is *not*.

so, maybe "file field item"? ;)

I'm almost wondering if simply adding a $context3 parameter to drupal_alter() isn't the best way to solve this in D7

That's actually what I'm recommending/doing with one of my contributed modules until this is fixed-- it's a tiny change and doesn't break any api's that way changing it just about any other way would. I just didn't think that was an acceptable option for a core bug fix.

Only drupal_alter() has a limited number of parameters, so there's no reason to use $context in hook_file_download_access(). This patch adjusts accordingly.

while drupal_alter() takes two $context arguments, that's a heavily loaded term in Drupal, would be good to think of another parameter name if possible

$context is consistent with other usage of drupal_alter(), especially entity/field related groupings, as in hook_field_attach_view_alter(). If given the other "context" work happening in D8, we want to change the argument name for this kind of usage, let's do so in a separate issue that covers all drupal_alter()s. That should not hold this issue up though.

My reasoning for using the same $context argument to both hooks was to make them similar but I agree that it's not necessary.

So why isn't this rtbc yet? It's a critical with a working patch.

1. I think we do need to include the empty($field_item) check (see #61). In most cases it shouldn't matter, but we don't know what kinds of changes people are making to the entity in hook_entity_load()... and if a NULL field items slipped through to the hook, that wouldn't be good.
2. Regardless of what variable name we go with ($file_item vs $field_item), we still need to improve the docs so they're on par with Drupal 7.

However, it occurred to me, why don't we just make our lives simpler in Drupal 8 and pass $file (i.e. the actual file entity) in to the hook, rather than this bizarre array-that-is-similar-to-a-file-entity-but-isn't? Then we don't have to worry about what to call it, or how to document it. Plus, it would make a lot more sense for the hooks to receive this.

Is there a reason I'm not seeing why we shouldn't just do that? (Note that this also probably allows us to skip all the searching for $field_item in the first place.)

At that point, the hooks would receive something like this:

$file: The file entity whose access is being checked.
$entity: The entity which contains a reference to the file.
$field: An array of information about the field on the entity which references the file, as returned by field_info_field().

Much simpler, I think.

Still not sure what's bizarre about $field_item. But I agree that passing a File entity makes more sense, especially in the light of #1446464: Get rid of file_field_load() - shouldn't load all file object data with field load - only file ID, which will remove the additional file properties from that array and just leave us with fid.

The thing is, that we will loose certain information that is *only* in there, like the size of images or the alt text of a file. No idea why you'd need that in a access hook, but who knows.

Ok, re-rolled with the $file_item => $file changes. If we want to pass $file_item as well then we need to add yet another argument to or $context for both hooks.

I guess better test coverage for all these checks and special cases and to verify that all arguments are passed correctly wouldn't hurt...

#74: file-file-download-context-1245220-73.patch queued for re-testing.

Removed $entity_type from both hooks and instead typed $entity.

Forgot to convert field_module_test. And yay, entity_extract_ids(), begone from that function! ;)

#79: file-file-download-context-1245220-79.patch queued for re-testing.

Made patch applying to current 8.x branch.

I have reviewed this several times and it makes sense and it is a good one.

Yes this looks great to me as well. Committed/pushed to 8.x, thanks!

This needs a change notification, right? (Actually, there were API changes in this issue twice, since the original commit made one too. But we only have to document the final change compared to Drupal 7.)

I think the backport to D7 is not critical (as far as I remember, at most it's backporting a test, if anything needs to be done at all?), but the change notification is, of course.

Actually, some of the hook_file_download_access_alter() documentation introduced here doesn't make sense, since it's still referencing "field item".

The attached patch fixes that, and does a tiny bit of extra docs cleanup also (basically to keep the documentation of hook_file_download_access() and hook_file_download_access_alter() the same, given that I have to change the latter in this patch).

I forgot that another thing which needed backport to Drupal 7 is #1143460: hook_file_download_access_alter missing entity argument, but I went ahead and reopened that now (it was previously closed as a duplicate), since for Drupal 7 I think that will be simpler to handle in its own issue - the backport might have to be very different from the change that went into Drupal 8.

Er, that definitely wasn't my patch which did that. HEAD is broken.

(In other news, this issue title just keeps getting longer and longer...)

OK, looks like the fix for that is simple.

I'm keeping my docs changes in this patch also, but it's committable without them if truly necessary.

By the way, it looks like what happened here (with the test failures) is that this issue and #1184272: Remove deprecated $conditions support from entity controller were committed around the same time, but they didn't play nicely together.

Oh DOH.

Oops. :)

Committed and pushed to 8.x. Thanks!

Ok, I seriously messed up :)

The test implementation has not been updated.

Confirmed and RTBCd.

Committed and pushed the test fix to 8.x.

Back to active for the change notice.

Yay!

Change notice is here: http://drupal.org/node/1744812

We need to remember to update it (the alter part) once #1143460: hook_file_download_access_alter missing entity argument is changed.

The change notice looks fine to me.

Per #86.

I don't think this needs a backport. The tests are the same as the ones that already exist in 7.x, the only thing is the broken alter hook, for which there is a separate issue.