Users could bypass custom field access for specific translations

Originally reported to the Drupal security team by @hchonov → on 18 July 2019 (#170529), which was subsequently considered a case for security hardening and not a vulnerability.

Problem/Motivation

Consider a custom field access preventing edit access for e.g. langcode “de” on a specific field, but allowing it for “en”.

1. Go on node/edit for an english node, change at the same time the langcode to “de” and the field with the custom access logic.
2. Submit and watch how the custom access has been bypassed and the field value has been updated.

The security issue I see here is that if a user does not have access to fields for specific translations, then the user could simply change the langcode, edit those fields in a different translation and then change the langcode back.

----

Maybe we could solve this, if we introduce an entity constraint, which becomes active only if the langcode has changed and in such case checks if the user has edit access for all other changed fields.

----
Some follow-up comments:

@Berdir
Discussed this a bit with @hchonov before he created this issue and I believer that having reliable per-language field access logic is almost impossible to achieve when a user is allowed to change the language anyway.

If you can change the language, you can save in that language, and then edit again and change the language back. at least when it is the default language, and the target language doesn't exist yet :)

I don't think constraints and access should be mixed, but we already do that because we don't have property-level access (e.g. text format)

----

@plach

I can't make my mind about this, to be honest: on one hand per-language access seems definitely a legitimate use case and something worth caring about in core, OTOH the same access bypass issue Hristo described in the OP may be reproduced with any custom access logic depending on an entity field value.

For instance, let's say we have custom module allowing access to field_bar only if field_foo > 3:

1. As an editor, I can set field_foo to 4 and save
2. Change field_bar and save
3. Restore field_foo to its original value and save

So it seems there's really nothing language-specific about this, which makes me think that we should either address this in core globally, i.e. actually requiring people to check access on save, much like we require people to validate entities (and enforce this in Form API workflows), or we just close this, with the assumption that any module implementing this kind of access logic is also responsible for dealing with the consequences. We could maybe have a PSA mentioning this use case.

If we do want to special-case language, IMO the simplest and less impacting way forward is to prevent people from changing language and other field values at the same time, via a constraint, as @hchonov was suggesting above. However this would have BC implications and might not be straightforward to implement, at least in a clean way (think of the changed field for instance).

All and all, I feel an opinion from the Security team would be useful.

----
@catch

I'm not sure this is fixable without some kind of bc break, in general I see this kind of conditional field access as more of a convenience/UX improvement than an actual security measure, and it's almost always custom logic so there is no generic exploit here - you have to be a translator or other kind of content editor, and you'd need to more or less understand the custom access logic in place to then be able to circumvent it - it is more likely that someone does it by accident than on purpose.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Contributors

- hchonov
- greggles
- Berdir
- plach
- catch