Arbitrary files can be referenced in file fields even if validation using form API would fail

It sounds like we should start here with one or more test cases to demonstrate the problem?

Here's a rough start on a test class. Doesn't do anything useful yet, just setting up the 2 content types. It passes at least if you run it.

Some code copied over from \Drupal\file\Tests\FileFieldTestBase

I'll try to continue with the test.

Some more work on the tests, this should fail now.

@klausi
Thank you, yeah you are absolutely right, tests should not be too verbose. This should clean the tests up a bit, but still have good test coverage.

+++ b/core/modules/rest/src/Tests/RESTFileValidationTest.php
@@ -0,0 +1,191 @@
+  protected function createFileField($name, $entity_type, $bundle, $storage_settings = [], $field_settings = [], $widget_settings = []) {
...
+  protected function attachFileField($name, $entity_type, $bundle, $field_settings = [], $widget_settings = []) {

Instead of copying, can we factor these 2 identical functions out of FileFieldTestBase into a trait?

Isn't this related to #2577963: Let entity_ref Selection handlers be in charge of the field validation ?

Oh wow, the 'file_extensions' and 'max_filesize' settings are only ever validated by the widgets, i.e only ever enforced in forms submissions.

I would indeed think those restrictions should be enforced by the FileSelection "entity_ref selection handler", and that we should leverage those during validation with #2577963: Let entity_ref Selection handlers be in charge of the field validation

Other than that, maybe FileItem::getConstraints() could add constraints for those, like we did with EntityReferenceItem::getConstraints() and the 'Bundle' constraint as a hotfix in #2064191: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled (was easy because the 'Bundle' constraint validator already existed)

So it seems like there are 2 separate issues here? The case of sending a bogus File ID in a form submission and sending a bogus File ID in a REST request?

Or is the REST code using Form API validation?

@pwolanin :
- The 'file_extensions' and 'max_filesize' restrictions are currently only ever applied by the field widgets, in the shape of validation routines passed as the '#upload_validators' property of the underlying ManagedFile FAPI element (more precisely, ManagedFile::valueCallback() delegates to an old-school function, file_managed_file_save_upload()).
- And those are checked only on *new uploads*, which explains that you can forge fake form values referencing an existing fid, and bypass the checks completely.

So yeah, the bug is probably two-fold :

- At the FAPI level (ManagedFile element), maybe the constraints should be enforced not only on file uploads, but checked on any submitted fid. That can be argued, I guess, since the property is named '#upload_validators', so we can't really say it's lying, it never claimed doing more than checking uploads :-)

- At the entity level, there should definitely be constraints that ensure validation fails if a field references a file that is not eligible according to the field settings, independantly of whether the widget / form element catches them or not (which it currently doesn't, as per the above). Those validations would fix REST, and make sure our entity forms have no holes either.

Then it's the usual problem of "it's better if the widget doesn't spend too much time validating, since the work will be done by entity validation anyway". The way this was approached in the EntityRef "autocomplete" widget and the underlying EntityAutocomplete FAPI element, is that the element has a #validate_reference flag, that defaults to TRUE, but that the widget sets to FALSE knowing that validation will be performed at the entity level.

Opened up another critical for the other bit: #2594565: File extension + max_filesize should be validated using a Constraint on file fields

This seems to be a general entity reference problem:

a) Create a unpublished node with the admin
b) Allow the anonymous user to create nodes with an entity reference
c) Add "a ($nid)" into the entity reference textfield
d) You can save the entity, but not access the label

I guess ideally we would validate on that level?

@dawehner : The more general #18 is what #2577963: Let entity_ref Selection handlers be in charge of the field validation is solving :-)

The additional step for it to solve this file issue here as well (or rather the entity validation part of the issue, meaning #2594565: File extension + max_filesize should be validated using a Constraint on file fields), would then be to make FileSelection respect the 'file_extensions' and 'max_filesize' field settings.

Yes, the bug part of this is indeed is present in the exact same way in Drupal 7. The only mitigation is that you need to know the fid of the secret file, but guess attacks might still work.

To reproduce on D7:

- Upload a txt file somewhere
- Save
- Note down the File ID

- Create an article
- Select the image field
- Inspect Element and change the hidden input fid value to the new fid.
- Click upload and voila there is your txt file on your image field!

So this is in no way rest specific, but the way value callbacks do work.

However file_managed_file_value is suitably protected against referencing a private file in D7, which indeed makes this a rest only scope.

The relevant code from D7 should probably be used for rest as well:

      if (isset($input['fid']) && ($file = file_load($input['fid']))) {
        // By default the public:// file scheme provided by Drupal core is the
        // only one that allows files to be publicly accessible to everyone, so
        // it is the only one for which the file access checks are bypassed.
        // Other modules which provide publicly accessible streams of their own
        // in hook_stream_wrappers() can add the corresponding scheme to the
        // 'file_public_schema' variable to bypass file access checks for those
        // as well. This should only be done for schemes that are completely
        // publicly accessible, with no download restrictions; for security
        // reasons all other schemes must go through the file_download_access()
        // check.
        if (in_array(file_uri_scheme($file->uri), variable_get('file_public_schema', array('public'))) || file_download_access($file->uri)) {
          $fid = $file->fid;
        }
        // If the current user doesn't have access, don't let the file be
        // changed.
        else {
          $force_default = TRUE;
        }

IMO we shouldn't focus on the access control to the private file, but rather the fact that a private file can be used in a field which is configured to use public files.

Do we really want to prevent that? I think it might actually be a feature, e.g. of the Media module, that you can open up the media browser and choose files to reference regardless of where they came from.

If you could reference a private file that you don't have access to that would of course be a security hole, but both D7 and D8 already have protection against that. I tested now with Drupal 8, for example, and regardless of whether I try to create the node via the admin interface or via the REST module, in both cases I am prevented from saving it and am shown the following error message: "You do not have access to the referenced entity (file: 1)".

So I don't see where a critical access bypass issue has been identified here.

No file restrictions like file extension, file size etc. is validated.

The same problem happens when editing entities in the browser/form API and manipulating file IDs when the form values are POSTed.

This part is more of a bug, although again need to think about the impact on something like the Media module. If you want to avoid annoying usability problems, you would need to make sure that when the media browser is opened, the media library only shows files that actually are going to be allowed in the field (otherwise the user can choose a file, close the browser, and only find out later that it wasn't allowed).

I wrote some custom code once that made the media library only show images that met the minimum resolution restrictions of the field that the browser was opened from. The code did work, but it was pretty messy... not sure how the Media module would actually handle that in general.

The IS is a bit out of whack with discussion here in terms of the proposed solution.

+++ b/core/modules/rest/src/Tests/RESTFileValidationTest.php
@@ -0,0 +1,191 @@
+    // Create a file which should be secret.
+
+    // Create a test entity type that contains the secret file.
+    $this->drupalCreateContentType(['type' => 'page']);
+
+    // Create a test content type that anonymous can post.
+    $this->drupalCreateContentType(['type' => 'article']);
+    $this->createFileField('field_public_file', 'node', 'article', ['uri_scheme' => 'public'], ['file_extensions' => 'txt']);
+    // Enables the REST service for 'node' entity type.
+    $this->enableService('entity:node', 'POST');
+    $permissions = ['create article content', 'restful post entity:node'];
+    user_role_grant_permissions(RoleInterface::ANONYMOUS_ID, $permissions);
+
+    // Create a private file and attach it to a node.
+    $this->createFileField('field_private_file', 'node', 'page', ['uri_scheme' => 'private'], ['file_extensions' => 'txt']);
+    $private_files = $this->drupalGetTestFiles('text', NULL, FALSE);
+    $private_file_author = $this->drupalCreateUser([]);
+    $file = File::create([
+      'uri' => $private_files[0]->uri,
+    ]);
+    $file->save();
+    $node = Node::create([
+      'title' => 'Test private title',
+      'type' => 'page',
+      'uid' => $private_file_author->id(),
+      'field_private_file' => [
+        0 => [
+          'target_id' => $file->id(),
+        ],
+      ],
+    ]);
+    $node->save();

Since the node that the private file is attached to can be viewed by anonymous, doesn't that mean they should also have access to the file, even if it is in the private file schema? Only if the page were to be unpublished should access to the file be denied?

Updating steps to reproduce. I'm at a BADCamp sprint with sgt1stklass working on this and found that we needed to get specific about the steps to reproduce before we could begin to solve the problem. Adding some clarity and background to steps to reproduce and next we'll explore the possible solutions.

Given #21 and the current issue summary, I'm curious whether:

From the issue title:

access bypass

Per #21, is there an access bypass, and if so, what is it? And if not, is there anything else that justifies a Critical priority on this issue?

Arbitrary files

Again, per #21, it's not arbitrary files, it's only files you can access. So then "arbitrary" is more like "files that don't comply with the field settings", and if that's all this is about, then per #19, is there anything here not covered by #2577963: Let entity_ref Selection handlers be in charge of the field validation plus #2594565: File extension + max_filesize should be validated using a Constraint on file fields?

From how I understood the original post, the vulnerability lies in how the REST module handles the request referenced by the curl:
curl --include --request POST --header 'Content-type: application/hal+json' http://YOURLOCALHOST/entity/node --data-binary '{"_links":{"type":{"href":"http:\/\/YOURLOCALHOST\/rest\/type\/node\/article"}},"title":[{"value":"test title"}],"type":[{"target_id":"article"}],"field_image":[{"target_id":"1"}]}'
which issues a request to an Article content type when the original file was uploaded to a Basic page. Trying to access the private file via the Basic page content type would return access denied, but accessing it via the curl would display the private file free and clear. This could be used by an attacker to access private files arbitrarily, which does seem like a critical issue to me.

The fix would need to include protection against cross-referencing files uploaded by target_id.

After a long talk with @timplunket and @dawehner at the BADCamp sprint, we agree that this issue is a plan/meta issue and that the critical issues are actually #2594565: File extension + max_filesize should be validated using a Constraint on file fields and #2577963: Let entity_ref Selection handlers be in charge of the field validation with foundational work happening in #1927648: Allow creation of file entities from binary data via REST requests.

The steps to reproduce and the ways that this exploit manifests can be critical, but those issues have been promoted to get the specific bug fixing tickets necessary visibility.

I'm a bit confused.

#18/#19, while also a problem are IMHO mostly irrelevant to files, because file/image fields do *not* use entity reference handlers. While you can technically use a generic entity reference field with files where that would then apply, nobody really does that, not without additional contrib modules. So, #2577963: Let entity_ref Selection handlers be in charge of the field validation doesn't help us with this problem as far as I see.

I'm going to work a bit on this/ #2594565: File extension + max_filesize should be validated using a Constraint on file fields, as commented in that issue, I don't understand the difference between the two. We already have a ReferenceAccess constraint on files that prevents that you can reference files that you don't have access to, as it was commented above. So I don't think there's a problem there as asked by @effulgentsia above.

We can make this a meta, but then it's no longer about files but any kind of reference and #2594565: File extension + max_filesize should be validated using a Constraint on file fields basically becomes this issue ;)

As a first step, a tiny patch that disables the existing reference access validation. Just wondering what tests will fail with this.

As @jhedstrom said in #22, the existing test here is not correct because the node is accessible and therefore so is the file. So using that file is perfectly valid.

Updating the test to use an unpublished node shows that this indeed is validated and works just fine, I just have to update the test to give the "private" node author the access own unpublished content permisssion and change the 403 on POST to 422.

The test passes now on HEAD and fails combined with the change from #28. I'm also pretty sure that we will have test fails there, I worked on exactly that together with @pwolanin in the Amsterdam code sprint IIRC. And back then, we explicitly applied the ReferenceAccess validation constraint only to files and images, we had plenty of test fails when trying to do that on normal entity reference fields, there's the problem with node authors that you might not have view access to but can still reference them. See also #2471154: Anonymous user label can't be viewed and auth user labels are only accessible with 'access user profiles' permission for a related problem there.

So, as far as I can see, the only bug related to files is #2594565: File extension + max_filesize should be validated using a Constraint on file fields. I'm going to update the test now to specifically test that and not private/public, assuming that we will indeed have test fails with #28.

I'm sorry, the patches above were just interdiffs, not the full patch.

And #28 isn't being tested, so re-uploading that too :)

Grml...

And, here is the validation, taken from #1927648: Allow creation of file entities from binary data via REST requests (with a small change to pass the actual error along instead of a generic message, including a rewritten test. Moved to file module and converted to a kernel test like we do elsewhere with validation tests. We have existing tests that rest does call entity validation.

This currently on tests the file extension on file fields, not sure how much additional test coverage we need since it's all part of the same API function that we call.

Make sure to credit @marthinal (we need to get him to comment here), who spent a huge amount of his time at the last few DrupalCons and DevDays trying to make #1927648: Allow creation of file entities from binary data via REST requests happen.

I don't think the backport tags apply anymore assuming we agree that the problem space is limited to what I'm testing/fixing in #34. ReferenceAccess was added in #2304969: Port private files access bypass from SA-CORE-2014-003, which is the 8.0.x implementation for https://www.drupal.org/SA-CORE-2014-003 (which I think is exactly the problem that most of this issue was discussing, which already had a public SA and was fixed then).

@berdir : file/image fields do use ER Selection handlers, which is where #2577963: Let entity_ref Selection handlers be in charge of the field validation would be relevant.

But one drawback is that the max filesize and allowed extensions are directly field settings, rather than settings of the FileSelection (unlike "referenceable bundles", for example, which is a setting of DefaultSelection). So FileSelection doesnt have access to those settings atm, and cannot validate them.

So yeah, I still think the selection handler would be the best place for checking those, but that might be out of reach now, because that would mean reshuffling a couple settings in config.

A dedicated constraint is less covering (since the selection process, used by widgets to present possible choices, would be left out), but also works as a stopgap.

@berdir : file/image fields do use ER Selection handlers, which is where #2577963: Let entity_ref Selection handlers be in charge of the field validation would be relevant.

I see. The parent settings are inherited and we also have the schema definition now. However, at runtime, we never actually invoke them at the moment, as far as I can see (I had a breakpoint in the parent constructor of FIleSelection and only user was instantiated while saving a node with an image.

I guess you're issue will change that, but right now, I don't think more or less accidentally inheriting the settings means the *use* them :)

But anyway, all your issue will do AFAICS is change how the constraint is invoked. I'm not sure that part is critical? I do believe that my patch is enough to solve the security and therefore critical part here?

Yes, it's validation that then wouldn't apply to files referenced with a normal entity reference field, but that wouldn't have he settings for the validation either at the moment as you said yourself and there are plenty of other bugs too (like no file usage handling). So we should probably just prevent that from being a possibility (hide the file target type).

Just to clarify :

However, at runtime, we never actually invoke Selectors [during validation] at the moment

Yes, that's the whole point of #2577963: Let entity_ref Selection handlers be in charge of the field validation: make Selection handlers the unique source for "what is referenceable ?". As such, its main change is to make ValidReferenceConstraintValidator (which atm merely checks the referenced ID exists) call the Selection handler for "are those entities really referenceable by the field ?"

Image/file fields don't "accidentally" inherit the 'selection handler' setting, they really do have a selection handler, and it is used by the implementation of the AllowedValuesProvider interface, inherited from ERItem, which would be used by advanced media selection widgets.

So the point of #2577963: Let entity_ref Selection handlers be in charge of the field validation is to make the results of the "allowed values" API consistent with the validation results, by making both fetch from a single source, the Selection handler. Currently, they differ, and the inconsistency sucks (allowed != will pass validation...)

Adding an adhoc constraint is fine to fix validation, but widgets that present selectable options then need to duplicate the logic to not present invalid choices to begin with. Putting the logic in the selection handler centralizes the logic in the one place where it's supposed to live.

The drawback is that doing so would mean moving max size and allowed ext from field settings to settings of the FileSelection handler (like "referenceable bundles" is a setting of DefaultSelection, and "referenceable roles" is a setting of UserSelection). That would mean config changes.

If there is no bug in referencing a private file, then this is not a critical regression, but a feature that exists since Drupal 6/7.

If this is truly a plan, this is not critical as behavior has not changed (not a regression) and there is no data loss and there is no security problem either. (A private file cannot be referenced)

This is a feature that media module uses quite heavily in Drupal 7.

Hence demoting to major until someone can show any criticalness of the bug / feature that should be release blocking or any security part of this that was not considered.

@yched: Right. Except the core file/image widgets aren't doing any selection at all, they just create new files :) I don't really know how those widgets would work with the selection plugins other than channeling the validation through them.

@Fabianx:
The issue summary here still correctly describes the original valid, possibly security relevant issue about missing validation that has been moved to #2594565: File extension + max_filesize should be validated using a Constraint on file fields, we just moved away from that in this issue and I still don't understand why ;)

Anyway, we can also move the patch in #34 to that issue. Doesn't really matter where we fix that.

But if we do that, I don't see what's left here. The current behavior is correct, there's no security improvements to be made in regards to private/public file behavior.

@berdir

the core file/image widgets aren't doing any selection at all, they just create new files :)

Sure, but contrib widgets let you pick existing files, and then the "allowed values" API we propose to them is lying.

I don't really know how those [core] widgets would work with the selection plugins other than channeling the validation through them.

They wouldn't do any specific validation at FAPI level, they would just leave ValidReferenceConstraint run at entity validation time.

#43: That is correct, but not critical, because it is possible in D7 core as well, so not even a regression.

I agree with #34 and think it is a great improvement for Drupal 8 and would be great to get in during the RC phase, but it does not satisfy any requirements for criticalness.

That all said I don't see this as a meta at all here and I think this one is more complete than #2594565: File extension + max_filesize should be validated using a Constraint on file fields.

To #34:

I have a question to the patch in #34:

- What happens if I use the following scenario to change the validation requirements:

- Given I upload a test.txt file to a file field on a node accepting "*.txt" files
- When I change my constraint to only accept "*.doc" files
- And I edit the title of the node
- I expect the node to save, because the file is not newly attached
- And I expect to be able to remove the .txt file to upload a .doc file.

Would that work with #34 or would even already attached files be subjected to the file constraints?

And would the same hold true if I manually change via Javascript the hidden 'fid' field in the upload field?

I tested #46 on simplytest.me and the "test" fails.

This certainly needs way more work - especially as its impossible to remove files that fail validation - so one is essentially stuck then with a no-longer-editable-node.

Edit:

Also tested changing hidden 'fid' manually and I can attach arbitrary files that way, but they are validated with #34.

#43: That is correct, but not critical, because it is possible in D7 core as well, so not even a regression.

What is possible in D7? We did not have a validation API, but we have now and this issue is about bring our validation constraints on the same level as the form validation already is. Unlike D7, we rely on the validation API being complete so that rest validation works as expected.

Re #46/#47:
That's not how any of our entity validation works. It might work that way at the moment for files but it doesn't for any other configurable field. If you previously had a value that is no longer valid, you must fix that if you can see that field.

Why can't you remove the file though, that should be possible?

@yched:

They wouldn't do any specific validation at FAPI level, they would just leave ValidReferenceConstraint run at entity validation time.

The problem is that the file validation on FAPI level is mostly built into the managed_file form element, so I don't think we can or should remove that completely.

@Berdir

The problem is that the file validation on FAPI level is mostly built into the managed_file form element, so I don't think we can or should remove that completely.

The ER autocomplete widget had to solve the same concern wrt the underlying ERAutocomplete FAPI element #type - from #16 :

Then it's the usual problem of "it's better if the widget doesn't spend too much time validating, since the work will be done by entity validation anyway". The way this was approached in the EntityRef "autocomplete" widget and the underlying EntityAutocomplete FAPI element, is that the element has a #validate_reference flag, that defaults to TRUE, but that the widget sets to FALSE knowing that validation will be performed at the entity level.

i don't think we have an agreement that the current behavior is a bug?

As proved by existing test coverage and also the test written in this issue, referencing a private file is only possible if the user has access to the private file. Given that he can also just download that file and re-upload it, I really don't see why we need to prevent against that?

The existing setting doesn't limit the *allowed* file schemes, just what is used for new files. This scenario was always possible.

Core doesn't have any widgets that allow to select existing files (I don't think that using a normal entity reference autocomplete widget counts?) and then it's a problem in e.g. entity_browser to show that a file is private and what implications that referencing it would have?

Also, keep in mind that referencing a private file doesn't make it public. It's still private, and a user still needs to have to at least one of the nodes that reference it. Again, the setting is just about newly uploaded files, not existing ones.

To clarify klausi's comment... In HEAD right now, if you have a filefield with public storage, create a few nodes, then switch the field to private, it just leaves the existing data where it is and the field is mixed-source thereafter? That's the current behavior?

The only hole here is that, from the UI, an admin can cause a field to end up mixed-source. From REST, any user with edit rights can cause a field to end up mixed-source. It still feels like the validation of the current storage location belongs in the entity validation, not form validation, which would then handle both cases.

That said, if doing that would break a bunch of other things then I'm not opposed to leaving it as is.

#59 With Javascript or inspecting and modifying HTML source, I can cause a field to be mixed source on D8 as well as any user. This is not rest only, but a property of Form API.

If there was somewhere on drupal.org here a .exe attached in {files}, I could attach it to my comment here ...

That's what #2594565: File extension + max_filesize should be validated using a Constraint on file fields fixed, you can only do that if that file respects the file validation of the given field.

This is only about the ability to use private:// (or something else, like akamai:// or whatever) files on a field that by default uses public://.

I agree with #58 to won't fix this.