Inclusion filter present security issue

[offlinehacker]

So let's say i have following relation user <-groupMembers-> group, now if i allow group owner to query users, he can also include all user tokens, and login as user, nice.

[raymondfeng]

Hmm, this is serious. I think the ACL will have to check the permissions for the included relations.

[fabien]

@raymondfeng I believe this has been fixed, no?

[grosem]

@fabien & @raymondfeng

I just tested it with the latest version and it is still possible. For example like this (manager is extending the User model):

http://localhost:3000/api/events?filter={"where":{"nr":1},"include":{"relation":"manager","scope":{"include":[{"relation":"accessTokens"}]}}}

https://groups.google.com/forum/#!topic/loopbackjs/8XmsoaHpNP4 seems to be the same problem.

[fabien]

ping @raymondfeng @bajtos @ritch

[bajtos]

@raymondfeng mentioned in the mailing list that one can disable includes in model/relation settings using the option disableInclude: true. Does that fix the problem?

Is it enough to change configuration of the built-in relation "User hasMany accessTokens" to prevent inclusion? Or is that just a workaround and we need to implement @raymondfeng proposal "the ACL will have to check the permissions for the included relations"?

[grosem]

From my point of view this should definitely covered by ACL.

[raymondfeng]

@bajtos User.hasMany accessTokens probably should have disableInclude set to true by default.

[bajtos]

Closing the issue as fixed by #1318.

[erikverheij]

The ability to access related objects via include is a large security hole. Essentially it makes the whole ACL useless as long as users can use the include filter.

I suggest to add a big warning on the include documentation page. In fact I think filter.include should be disabled by default until ACL deals with it.

[fabien]

@erikverheij agreed, this should be handled with caution.

@raymondfeng @bajtos what do you think?

A workaround for now (from the top of my head) would be:

Model.observe('access', function(ctx, next) {
  ctx.query.include = {}; // clear
  next();
});

[Shyri]

I totally agree with @erikverheij . Include feature is a nice way to avoid multiple requests from the frontend but it is a must to be handled by ACL in order to have decent security. At the moment the only solution I can see is to use "disableInclude" for all models that needs to be blocked to at least one role (other than $everyone or $unauthorized) and access them with specific requests only allowed for specific roles

[ben-girardet]

Any chance that the ACL would handle this security issue in the future ?

[dmisdm]

Has this issue essentially moved to #1362 ??

[sebastianfelipe]

Can this "disableInclude" feature affect the client calls only and not the back-end side? Thanks.

[bajtos]

Can this "disableInclude" feature affect the client calls only and not the back-end side? Thanks.

@sebastianfelipe I believe this flag affects all calls, including JavaScript API. If you suspect it's not working that way, then please open a new issue and provide us with a small app reproducing the problem - see http://loopback.io/doc/en/contrib/Reporting-issues.html#bug-report

[sebastianfelipe]

I think it works it that way pretty well. But, I think it shouldn't be as well. It should block the include calls from the client calls, but not from the internal use. It could facilitates some operations a lot!