json-patch major security holes [DATAREST-1032]

Ezra Epstein opened DATAREST-1032 and commented

Given a domain model with related entities such as:

@Entity
public class Article {
...
    @OneToMany(mappedBy = "article")
    private List<Comment> comments;
...
}

@Entity
public class Comment {
...
    @ManyToOne(optional = false)
    private Article article;
...
}

We proceed to perform a JSON-Patch request against a comment but (sneakily?) change the associated article entity in that request:

PATCH http://some.domain.foo/api/comments/1234
Content-Type: application/json-patch+json

[
    { "op": "replace", "path": "/article/title", "value": "foobar2" }
]

And... it works.

THUS, ANY USER who can edit their comments could change the article itself. This is a security hole waiting to be exploited.

BUT: the none of the expected hooks get called. So, for example:

org.springframework.data.rest.core.annotation.HandleBeforeSave

on the Account is not called.

MOREover, there is no clear way to prevent this sort of thing directly via Spring and there is no way to scope the security checks.

Something about this naive use of the powerful SpEL to do Json-Patch is semantically inconsistent with the needs of securing a REST API.

I've written a custom implementation to disallow the "dangerous" paths, but it occurs to me that a general purpose solution would be pretty easy and is something Spring should support.

AND, in the meantime, the docs should caution against using JSON-PATCH on any endpoints that are part of a graph where different entities have different security profiles.

Possible general solutions:

1. Add an annotation parameter that allows one to enumerate disallowed path regexes to check before processing the patch; along with a setting for whether to proceed and process the remaining valid paths or to abort the entire operation if an invalid path is encountered in the patch;
2. a content-type specific call-back method that allows custom code to pre-process the patch and/or indicate whether subsequent processing should proceed or be aborted.

My own current (custom) implementation calls such a "validate" method before applying the patch:

public abstract class ControllerBase<T, ID extends Serializable> {
...
    protected ResponseEntity<PersistentEntityResource> patchEntity(CrudRepository<T, ID> dao,
                                                                   ID entId,
                                                                   JsonNode patchReq,
                                                                   PersistentEntityResourceAssembler pera) {

        log.trace("{}: serverBody: {}", domainType.getName(), patchReq);

        HttpStatus status;

        Patch p = null;
        try {
            p = jsonPatchConverter.convert(patchReq);
            status = validatePatch(p);
        } catch (IllegalArgumentException | PatchException ex) {
            log.info("{}: Malformed patch request: {}", domainType.getName(), ex);
            status = HttpStatus.BAD_REQUEST;
        }
...

The simplest form of validation for my case of modifying related entities via the json-patch is just to disallow any path that has a "/" after the first position in the string. Crude, but effective.

A more standards-based approach might be useful.

---

Affects: 2.6.1 (Ingalls SR1)

Reference URL: http://stackoverflow.com/questions/42590027/securing-json-patch-paths-in-spring-boot-data-rest-application/42830785?noredirect=1#comment72944475_42830785