[BUG] Security issue when resolving $owner role

[pierreclr]

Bug

considering the following model definition :

{
  "name": "myModel",
  "base": "PersistedModel",
  "idInjection": true,
  "options": {
    "validateUpsert": true
  },
  "strict": true,
  "properties": {
    "myProperty": "string"
  },
  "relations": {
    "user": {
      "type": "belongsTo",
      "model": "user",
      "foreignKey": "userId"
    },
    "sender": {
      "type": "belongsTo",
      "model": "user",
      "foreignKey": "senderId"
    }
  },
  "acls": [
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "$everyone",
      "permission": "DENY"
    },
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "admin",
      "permission": "ALLOW"
    },
    {
      "principalType": "ROLE",
      "principalId": "$owner",
      "permission": "ALLOW",
      "properties": "findById"
    }
  ]
}

If a user tries to access this ressource as a sender or as a user, it should be able to access it.

Problem : In that case, the $owner role will be resolved if the current logged in user (in the access context) has the same id that the one in userId BUT the senderId role will not be resolved which should not be the case.

No we can speak about security issue :
considering the following model :

{
  "name": "myCompromisedModel",
  "base": "PersistedModel",
  "idInjection": true,
  "options": {
    "validateUpsert": true
  },
  "strict": true,
  "properties": {
    "myProperty": "string",
    "userId": "string"
  },
  "relations": {
    "sender": {
      "type": "belongsTo",
      "model": "user",
      "foreignKey": "senderId"
    }
  },
  "acls": [
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "$everyone",
      "permission": "DENY"
    },
    {
      "accessType": "*",
      "principalType": "ROLE",
      "principalId": "admin",
      "permission": "ALLOW"
    },
    {
      "principalType": "ROLE",
      "principalId": "$owner",
      "permission": "ALLOW",
      "properties": "findById"
    }
  ]
}

In that case the only real owner is (in theory) the one contained in the senderId property.
BUT, because of how the roles are resolved (see loopback/common/models/role L210), the framework first try to check the userId property even if the relation to the owner is not defined on this property and will resolve (and ALLOW) the role anyway.
That's why in this case if someone with a userId tries to access the model even if he is not the owner, he could access to it ... and the sender role will be DENY ...

To fix this issue, you should consider to never arbitrary check a possible userId property but fetch the belongsTo relations properties only and resolve the role that way. First trying to look at the inst.userId || inst.owner to define the ownerId may cause some serious security issues in the case that a model has a property userId / owner not related to any belongsTo relation.

Do you want me to create a PR to fix Role.isOwner in loopback/common/models/role ?

Thank you in advance and however, what a great job you're doing with this framework !

Additional information (Node.js version, LoopBack version, etc)

Node version : 6.X LTS
Loopback version: ^2.36.0

[pierreclr]

I understand that's not a real security problem but I insist because of the functional aspect !

Indeed in the first case I describe : only the owner role on userId will be resolved :/