TrustedProperties don't include the uuid value

[neos-bot]

Jira issue originally created by user econic:

If you render an edit/delete form, you can just replace the hidden **identity input to another entity's uuid that you have read access on and voilà, you can edit this entity.

Therefore i suggest the trustedProperties to contain also the **identity's value.

Since as a developer you would expect this to be checked, i regard this bug as critical because this opens up security vulnerabilities in all flow applications with edit/delete forms.

Jira-URL: https://jira.neos.io/browse/FLOW-138

[neos-bot]

Comment created by cerlestes:

Can confirm this is the case. You can override any accessible other entity of the same kind by simply swapping the identifiers.

A form shouldn't allow to edit another entity, even if the user potentially would be allowed to do so. This, indeed, is a critical bug.

[neos-bot]

Comment created by akii:

Fix it. Please.

Edit: added a please :)

[neos-bot]

Comment created by @bwaidelich:

IMO security on that level should be enforced with a proper Policy.yaml

BTW: [~akii] Please don't assign tickets to others. If you want feedback you can mention people (like I did here ;) or post to the mailing list

[neos-bot]

Comment created by akii:

Haha sorry [~bwaidelich]. I didn't know about this feature.

The problem here is when you render forms where the form elements are dependent on the individual entity. In this case you can generate a form for an entity where you have rights, then change the ID to also apply those rights onto other entities.

This, afaik, cannot be enforced properly by a policy.

[neos-bot]

Comment created by econic:

Hi [~bwaidelich], you can not use the policy for that, because you can only restrict read access, not update or delete access. This is exactly why i think this issue is critical, because if you grant read rights on a resource to someone who has access to an edit form of the same entity type, you implicitely give him full access to this entity.

[neos-bot]

Comment created by akii:

Additionally, I'd suggest to, by default, include all input[type="hidden"] values in the hash, unless op'ed out manually (or inverse and at least offer the functionality to hash). Something like .

[neos-bot]

Comment created by @bwaidelich:

You can enforce that with content security and with the new ACL foundation this will even be easier.

But apart from that: I agree that it could be discussed whether hidden field values should be protected (not just identity fields IMO) and in fact we discussed that before (but I can't find the result/ticket atm).. Let's collect some real-life scenarios, pros and cons here

[neos-bot]

Comment created by econic:

Real life scenario from us: We have file entities that everyone has read access to. Additionally, there's plenty of reasons why you could have write access to a file and several places where you can edit files. Opening a file edit form and replacing the uuid now enables you to update any file that you have the uuid of, even if you shouldn't be able to edit that file.

"plenty of reasons why you could have write access to a file" means that i had to write a really long list of content security resources which i'd have to update regularly and which would slow the queries incredibly down.

[neos-bot]

Comment created by cerlestes:

I don't think that it's a good idea that the server is delivering a form and the client is free to change the edited entity at will.

Although the mentioned problems could be solved via ACL/content security, I don't see it as a problem of this component. I rather think that if the server sent out a form containing a specific entity, only this entity should be allowed to be returned to the server and not any other one the ACL/content security allowed write access to.

[neos-bot]

Comment created by econic:

Another reason would be that if i use the FormViewHelper with the object argument, i give the user access to THIS entity, not to all entities of this type. [edit: just what [~cerlestes] wrote above]

[neos-bot]

Comment created by afoeder:

TBH I am more on Bastian's side to use (proper) ACL settings. A Form should not and even must not define the security settings for an followup-action which not necessarily must have anything to do with the (sending) form. Think RESTful: you create/modify a resource which is represented by the URI and you should be able to "anonymously" (i.e. without hashing stuff) send data to it; end the end point has to decide whether you're allowed to do that. Eventually you will end up with ACLs defined by a sending form and/or ACLs defined by the backend which is not good IMO. One might consider the hashed form data as kind of "authorization" but that'll be really a moloch to tackle.

What if your data changes during runtime, i.e. via JavaScript? If you exchange identifiers on purpose, for example with the result of an XHR fetch? Could be handled with the disableHash idea ofc, but that might introduce problems we're not aware of ATM.

[neos-bot]

Comment created by cerlestes:

[~afoeder]: You're missing the point of using the FormViewHelper though ;)

Edit: Thinking about it: What about splitting it into two VHs? A BasicFormViewHelper which does the basic stuff like handling the properties correctly and does not have any security-related features like trusted properties and CSRF, and a FormViewHelper or even an EntityFormViewHelper, which adds trusted properties, CSRF token and binds the form to the entity's identifier.

Edit2: Checking the abstraction of the VHs (namely AbstractFormViewHelper), doing this split would probably clean up the code base a lot.

[neos-bot]

Comment created by econic:

As long as you use the trustedProperties at all, i would suggest you should protect the form as much as possible. That includes your javascript situation, because currently you're also not able to add properties dynamically.

Additionally you could of course be able to just not protect anything, which would be more RESTy, sure. But that's not the case when using trustedProperties imo.

[neos-bot]

Comment created by @skurfuerst:

Hey everybody,

First off -- thanks everybody for being involved on the discussion, I definitely see the points of both opinions and have thought about this topic previously as well; though I did not arrive at a conclusion yet.

• This change, if we do it, would be a rather big behavioral change for users. Thus I'd only change the default in Flow 3.0; and if we backport to Flow 2.X, we need to keep the old behavior; as it is difficult for users to determine if their application still works after this change as expected.
• I tend to agree that we should, by default, both protect hidden fields and **identity of the form. It would be possible to disable that on a field-by-field basis (although then, the information that it is disabled must be of course part of the Hash).
• For REST APIs, you will want to disable the HMAC altogether anyways. So for me, REST APIs are handled differently anyways. * To open a side-discussion: For select boxes, we do not verify whether the thing the user chose was actually in the list of the select. Do we want to handle that as well? I'd say NO, because this does not modify the content security

Curions on what you think :-)

Greets, Sebastian

[neos-bot]

Comment created by econic:

Thanks for your thoughts.

First on your side discusstion: i have relevant points for and against validation of values in selects, so it could be handy if you were able to do both. It can be really helpful with security and really annoying with ajax/clientside stuff. User should decide, default is protect i'd say.

I also think that REST APIs shouldn't be protected like this in the first place, and also that the hidden fields should be protected by default.

But, i don't see where this change is not backwards compatible. Let's say you add this check in 3.0 and backport it as an optional argument in 2.x OR make it a setting defaulting to false, so everyone can enable that for his own environment. Don't see how someone can't be happy with this.