Follow-up from SA-CONTRIB-2013-062

Steps to reproduce:
* Create a test user account with the permission to create nodes and access POST on the resource node, but no access to the full_html filter format.
* Enable the POST on the node resource, clear caches
* Send a POST request that is authenticated as test user with a body field like this: {"_links":{"type":{"href":"http:\/\/drupal-8.localhost\/rest\/type\/node\/page"}},"title":[{"value":"New node title"}],"body":[{"value":"<script>alert(\"XSS\");<\/script>","format":"full_html"}]}
* The node gets created with a full_html body field, although the user should not have access. If the PHP module is enabled this open a remote code execution vulnerability as arbitrary PHP code can be injected into the node body with the php_code text format.

Proposed resolution

* Run field level access checks AFTER values have been set on a field
* Make sure that text format access works on the field API level, see also #2060237: Text fields should enforce input format permissions in field access..

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

Status: Active » Needs review
FileSize
7.61 KB

Intermediate patch that starts with an access implementation for text.module. Includes patch from #2063571: Node denormalization for fields with limited values is broken. This is far from finished, just dumping my work here. Test currently fails with Notice: Undefined index: args in core/modules/text/lib/Drupal/text/TextField.php on line 27, so I must be doing something wrong when creating the field class.

The idea is to create a separate Field class for text.module that implements the access checks.

Dave Reid’s picture

I'm still unsure on if we should actually validate if the user has access to the text format or not. What happens if we try to do an altered POST request on the node form to do the same thing?

Status: Needs review » Needs work

The last submitted patch, rest-security-field-access-2064181-1.patch, failed testing.

klausi’s picture

@Dave Reid: if i tamper with the form POST data on a POST request then of course the form API will prevent me from doing that with "An illegal choice has been detected. Please contact the site administrator.". Text formats that are not present in the selectbox cannot be submitted, that is the point of automatic form data validation.

On REST POST requests there is no form API involved, we use the field access API and the entity/field validation API to detect illegal values.

wodenx’s picture

FYI - D7 issue is at https://drupal.org/node/2060237.

moshe weitzman’s picture

klausi’s picture

Status: Needs work » Needs review
FileSize
4.49 KB
10.53 KB

The access implementation for text module should now be complete (including test cases).

TODO:
* Fix REST create test
* Re-visit PATCH/update operation and where field access is called
* Fix REST update tests

Status: Needs review » Needs work

The last submitted patch, rest-security-field-access-2064181-7.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
6.36 KB
16.29 KB

This should now be ready for review. Note that the patch still contains #2063571: Node denormalization for fields with limited values is broken, reviews there would be appreciated, too.

I added text format test cases to REST module's create and update tests.

Status: Needs review » Needs work

The last submitted patch, rest-security-field-access-2064181-9.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
923 bytes
16.28 KB

Fixed node namespace.

Status: Needs review » Needs work

The last submitted patch, rest-security-field-access-2064181-11.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
16.5 KB

Looks like a random test fail, could not reproduce locally.

Merged in changes from #2063571: Node denormalization for fields with limited values is broken.

Berdir’s picture

Can we somehow rely on the AllowedValuesInterface (which is still not committed :( ) to validate the format, instead of a custom access implementation? Instead of access, it would then fail validation.

klausi’s picture

Status: Needs review » Postponed

You are absolutely right! I justed tested this with #1758622: Provide the options list of an entity field and the REST test cases work as expected, with the only difference that 422 is returned instead of 403.

The question is now if we really want this. Field access checking modules must be aware that text formats are not taken into account in the access implementation (or, at least not for core text fields). This is nothing new, in D7 this was not implemented either. So field access means whether the user can create/update the field in general regardless o the value(s), while validation means the semantically correct values allowed for a user. This might be a bit confusing for developers where they should implement their access check, but it also does not make sense to implement the text format access check twice if we already have it in the AllowedValuesInterface validation in #1758622: Provide the options list of an entity field.

This is now postponed on #1758622: Provide the options list of an entity field.

#2063571: Node denormalization for fields with limited values is broken has now been closed as well, so that can be removed from the patch.

jibran’s picture

Status: Postponed » Active
klausi’s picture

Status: Active » Needs review
FileSize
6.56 KB

Ok, so the patch is now a lot shorter, I basically just copied the test case additions and the changes for the entity resource plugin.

klausi’s picture

klausi’s picture

Issue tags: +WSCCI, +Stalking Crell

Tagging for Crell.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I think this makes sense to me. I trust Klausi to know the security implications, and the code looks good.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks good. Committed/pushed to 8.x, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

removed unnecessary step