Framework: Attribute shadowing with join() is a security risk

[jonashaag]

The fact that attributes of join()ed models are in the same namespace as the "base" query model provides a security risk.

For example say I have this query in my program:

function getUsers() {
  return User::whereNot("disabled");
}

A few months after coding this the project requirements change and we now have user groups as well, and we change the query to all users having a primary group :

function getUsers() {
  return User::join("groups", "groups.id", "=", "users.primary_group_id");
}

Also we always had some code that fetches users' orders which worked like this:

Order::where("user_id", $someUser->id);

Now with the old version of the getUsers function this would be fine, because ->id is the correct user id. However with the added join the ->id field may now be the group id, which breaks the logic and may result in wrong joins. This is a possible security risk!

Coming from other ORMs such as Django, SQLAlchemy, Active Record, ... this was a huge surprise for us and it actually caused other people's data being shown to people. For now we have implemented a custom checker for "->join() without ->select()", which does not really solve the problem but minimises the chances that you simply forget about adding a ->select() clause, as pictured above.

I believe this to be a severe security problem that should be fixed. I'm unaware of any well-known ORM that inlines related objects' attributes into the base object namespace.

[fernandobandeira]

When joining the groups table the generated query is something like:

SELECT *
FROM `users`
INNER JOIN `groups` ON `groups`.`id` = `users`.`primary_group_id`

Since both columns have an id column the one from the groups table will override the one from the users table when turning this into an array...

Changing this will probably create a BC, i'll ask around what's the opinion of ppl about this and if it's worth introducing a big BC to fix this, i've thought about solutions for this already...

[fernandobandeira]

I think we should close this IMO

This is normal SQL behavior when doing a join. Use a select clause, that is what they are meant for.

Maybe we just need to warn people about this in the join's section?

[jonashaag]

Maybe we just need to warn people about this in the join's section?

If you don't care about having severe security problems in your code base, then this is the way to go. Otherwise, from a pragmatic point of view, this problem has to be solved in Laravel. There will always be a good amount of people who don't read up on all the hints in the documentation and all of these people will have severe security problems in their code. Personally I would not want to be responsible for that.

Btw, SQL does not have this problem because you are forced to explicitly tell which ambiguous column you want to retrieve.

[jonashaag]

@taylorotwell ref #16914

Note my comment above; the problem is that you are adding an implicit select clause, which does not exist in SQL and which is the cause of the security issue.

I just had a look at #16895 which was also referenced here. While these issues are usually trivial to fix, it's not trivial to see them. They may even go unnoticed for a long time, causing seemingly meaningful data processing (see my initial post in this issue). This seems to be a fundamental problem in the way SQL is generated by Laravel. The Python web framework Django for example always prefixes columns with the table name. It never produces queries like SELECT id from table, but always SELECT table.id from table, so it doesn't get all the small potential bugs and security issues that Laravel potentially has.

[fernandobandeira]

I do agree with you I really think that we should address this somehow, however it's not an ordinary fix and will introduce quite a BC, and in the end this can be avoided by properly calling the select method when using join's...

IMO we should address this at least for Eloquent since it's an ORM I think that it should handle this at least, there's many ppl having this problem and as you stated most of them didn't realize whats happening...

However this is argueable some may say it's something the ORM should handle other's will say it's something that the developer should take into account...

I think that laravel/internals is a better place for this discussion since it's being considered as a proposal currently, maybe it'll get more attention there...

[GrahamCampbell]

Maybe we just need to warn people about this in the join's section?

Yes please. :)

[hailwood]

To add another real world example of where this happens, I reported this at laravel/framework#23548 as happening with hasManyThrough which is a lot less "manual" than the joins above yet is still an issue.

I can understand why this happens but personally was not aware at all that eloquent would actually select the id from the join table at all so didn't account for it.

This is actually a situation where simply saying "add a select clause" isn't really a valid answer because it's a relationship query. But I'd settle at least for there simply being a warning

When using the hasManyThrough relationship, if the foreign key on the joining table matches an column on the target models table this will be replaced with the value of the joining table. tl;dr: Do not use a foreign key column that matches a column on your target models table.