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..
Comment | File | Size | Author |
---|---|---|---|
#17 | rest-security-filter-format-2064181-17.patch | 6.56 KB | klausi |
#13 | rest-security-field-access-2064181-13.patch | 16.5 KB | klausi |
#13 | hal-field-cardinality-2063571-12-interdiff.txt | 1.48 KB | klausi |
#11 | rest-security-field-access-2064181-11.patch | 16.28 KB | klausi |
#11 | rest-security-field-access-2064181-11-interdiff.txt | 923 bytes | klausi |
Comments
Comment #1
klausiIntermediate 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.
Comment #2
Dave ReidI'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?
Comment #4
klausi@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.
Comment #5
wodenx CreditAttribution: wodenx commentedFYI - D7 issue is at https://drupal.org/node/2060237.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedI added this issue to #2028027: [META] Missing access control for base fields
Comment #7
klausiThe 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
Comment #9
klausiThis 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.
Comment #11
klausiFixed node namespace.
Comment #13
klausiLooks like a random test fail, could not reproduce locally.
Merged in changes from #2063571: Node denormalization for fields with limited values is broken.
Comment #14
BerdirCan 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.
Comment #15
klausiYou 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.
Comment #16
jibran#1758622: Provide the options list of an entity field is in.
Comment #17
klausiOk, so the patch is now a lot shorter, I basically just copied the test case additions and the changes for the entity resource plugin.
Comment #18
klausi#17: rest-security-filter-format-2064181-17.patch queued for re-testing.
Comment #19
klausiTagging for Crell.
Comment #20
Crell CreditAttribution: Crell commentedI think this makes sense to me. I trust Klausi to know the security implications, and the code looks good.
Comment #21
catchLooks good. Committed/pushed to 8.x, thanks!
Comment #22.0
(not verified) CreditAttribution: commentedremoved unnecessary step