File module security fixes from SA-CORE-2011-001 not yet applied to Drupal 8

Related documentation issue #993082: file_get_file_references() @return doc totally wrong.

Are there any test that could be added to node.test (building on the existing node_access_test.module) to test for this vulnerability?

I agree that tests here would be really welcome. We definitely dont' want to break this EVER again.

I tried to write a test but it isn't as easy as just enabling node_access_test.module and trying to view a private file, because that passes.
I read the code, which is affected by the patch. And the patch in itself seems to make a lot of sense, but I don't see how that is related to node access modules.

Test is here. It's very weird that I am getting a 404 for logged out user instead of a 403. Very strange. Edit: I am running D7 and the patch above is applied.

Well that just proves that the above patch is necessary -- tests passes for me but it's not committable because I do not see why I am getting a 404 vs a 403.

OK figured it out. tl;dr: file module can't deny access because it has no idea the file is used by a file field.

Let's say we have private://foo.txt. This file is used in one place, in a file field attach to one entity only, a node denied by the node_access table to the current user.

1. file_file_download checks the URI in the database.
2. Then it calls file_get_file_references.
3. That function runs EntityFieldQuery.
4. node_query_entity_field_access_alter does its elaborate dance and completely hides the fact that the file is used by a node.
5. EntityFieldQuery returns empty handed and so with the array_filter, file_get_file_references also comes back empty handed.

6.   // If there are no references, stop processing, to avoid returning headers
     // for files controlled by other modules.
     if (empty($references)) {
       return;
     }
   

That's the end of the story. While a 404 is not as good as a 403 it's still a lot better than a 200 and so I kept assertNonResponse(200) as we have nothing better. Patch attached is just the concat of #2 and #7.

I strongly recommend committing this and handle the 403-404 issue in separately. #1221214: file_download returns 404 instead of 403 filed.

#10 is fun.

+
+
+class FilePrivateTestCase extends FileFieldTestCase {

One too many line breaks. And I think the test classes themselves need a docblock. Apart from that looks good.

Reroll with corrections suggested in #12.

+++ b/modules/file/tests/file.test
@@ -1041,3 +1043,48 @@ class FileTokenReplaceTestCase extends FileFieldTestCase {
+      'description' => 'Uploads a test to a private node and check access.',

Typo: ...and checkS access.

+++ b/modules/file/tests/file.test
@@ -1041,3 +1043,48 @@ class FileTokenReplaceTestCase extends FileFieldTestCase {
+  function setUp() {
+    parent::setUp('node_access_test');
+    node_access_rebuild();
+    variable_set('node_access_test_private', TRUE);
+  }

Also a nitpick, but I wonder if we shouldn't merge with func_get_args() here as well. I don't know if there are that many use-cases extending this class, but it seems somewhat best practice.

+++ b/modules/file/tests/file.test
@@ -1041,3 +1043,48 @@ class FileTokenReplaceTestCase extends FileFieldTestCase {
+   * Tests upload and remove buttons, with and without Ajax, for a single-valued File field.

This seems leftover from copy-paste. Could maybe mention mention the node_access_test module, which is subtle but important here (right?).

Tentatively marking needs work, but this is really RTBC except docs, which could also be dealt with in a follow-up.

Also a nitpick, but I wonder if we shouldn't merge with func_get_args() here as well. I don't know if there are that many use-cases extending this class, but it seems somewhat best practice.

You mean for parent::setUp('node_access_test');, right?

I'll reroll it with these changes since I still have that clone set up.

Reroll with corrections from #14.

Ah, too many levels of array_merge. Are we using this pattern elsewhere for child classes?

There's a better pattern in FieldUITestCase we can probably use. However, considering that most child classes are not yet extended, I think maybe we should leave it as-is until there's the need to extend it? What do you think?

Edit:

    // Since this is a base class for many test cases, support the same
    // flexibility that DrupalWebTestCase::setUp() has for the modules to be
    // passed in as either an array or a variable number of string arguments.
    $modules = func_get_args();
    if (isset($modules[0]) && is_array($modules[0])) {
      $modules = $modules[0];
    }
    $modules[] = 'field_test';
    parent::setUp($modules);

func_get_args here is not necessary only on classes that will be extended (ie non-test classes, just helpers).

Reusing the pattern from field UI tests in the parent class, and dropping the unnecessary arg merge from the child.

RTBC if it comes back green.

Also, the test should probably be backported.

The test is known to work with D7 -- I developed it on D7 so the commit is just that, patch -p1 the patch, the file.inc hunk will recognize it's been applied already it will ask "Assume -R" answer n and then apply anyways answer n.

There is no point in creating the field first then changing it. Speedier test, yay.

Edit: the interdiff is backwards. Bother.

git apply is very picky about what it accepts. patch licked this up.

+++ b/modules/file/tests/file.test
@@ -1041,3 +1051,47 @@ class FileTokenReplaceTestCase extends FileFieldTestCase {
+    $field = field_info_field($field_name);

Just noticed that: It seems like the $field variable is never used below.

Leaving at RTBC.

Argh. Dreditor...

That seems valid. Re-uploading with that line removed.

Out of things to complain. Definitely RTBC.

Committed and pushed #30 to 8.x, then manually removed the file.module hunk and committed and pushed to 7.x as well. Yay!