Potential Security Issue (User model -> change-password/reset-password endpoints)

[sebastianfelipe]

Problem Description

Hi, I'm writing this post because I think the new functionalities change-password and reset-password doesn't have the expected security considerations.

When a user model is created (or used as a base model), it creates 2 potential issued endpoints: /Users/change-password and /Users/reset-password.

Both of them, belongs to a particular or specific user, let's say a userId 1 is authenticated and use /Users/change-password to change its password, BUT, this works when exists only one TYPE of User account. It uses the access_token to determine the userId, but when exist a polymorphic relation (multi-user model), it could be tricky. For example a user from another user-based model that has the same userId (I wrote a bug in Loopback 2.0 about a problem like that in other situations).

Possible Solution

To this case, I suggest enable that functionality without relaying in a authenticated access_token, instead use an owner access_token. The change I propose is generate those 2 endpoints, but in this way:

• /Users/{id}/change-password
• /Users/{id}/reset-password

That is, using explicitly the id of that user model.

I hope you consider this possible inconvenience to multi-users implementations. Also, it could be very useful when you're developing with a team, setting the endpoints free (without the server.enableAuth()), saving time in reproduce the use cases, that maybe involve those functionalities. From my point of view, a change like that could worth it.

I'd really appreciate the improvement of these functionalities.

Thanks, Sebastián.

[kjdelisle]

@sebastianfelipe If the routes were to be bound to the ID of the user model, it would involve giving away the user's unique identifier as a part of the URL of the request, rather than as a part of the payload.

If you could provide a working demonstration of how to exploit this vulnerability, that would be great, but I would ask that you send it to me in private using the email address listed on my GitHub profile, and not make it public at this time.

[sebastianfelipe]

I tested with polymorphic relations and it is working well (I tested with the scope [reset-password]). It is ok, is working. So I think there won't be a major problem using the url without bound to the ID. I had my doubts because https://github.com/strongloop/loopback/issues/2516, issue I opened a year ago. I think that issue is not solved, I tried again and the $owner permission has problems. I posted at last what it needs to be fixed. I hope we can get a solution to that issue (2516).

Thanks again, Sebastián.

[sebastianfelipe]

Another thing, using the point you made, the /User/{i}/verify, maybe should be /User/verify with an option (like the /User/reset), that needs an email, and then send the verification email. What do you think?

[kjdelisle]

@sebastianfelipe In your example, you need to have a unique AccessToken implementation for each user type. The CustomAccessToken example is not meant to be recycled between multiple User types (as that would cause the same problem!).

Using the example you posted in #2516, you would want a CustomerAccessToken and a separate BackOfficeAccessToken, each with the appropriate hasMany/belongsTo relations to link them to their corresponding user representations.

This way, if someone were to generate a reset token with a CustomerUser, it would be tied to the CustomerAccessToken model type, which would not work when attempting to call the /BackOfficeUser/{id}/reset-password route.

EDIT: This might be wrong, I'm double-checking now. EDIT 2: I can confirm that separating the AccessToken implementations resolves the issue.

[guillob]

you would want a CustomerAccessToken and a separate BackOfficeAccessToken, each with the appropriate hasMany/belongsTo relations to link them to their corresponding user representations.

@kjdelisle after I read your comment, how can you set both custom tokens in middleware.json or server.js ? Thanks!

[kjdelisle]

{
  "name": "CustomerUser",
  "base": "User",
  // ...
  "relations": {
    "accessTokens": {
      "type": "hasMany",
      "model": "CustomerAccessToken",
      "foreignKey": "userId",
      "options": {
        "disableInclude": true
      }
    }
  },
  // ...
}

You'd just change your user models to use the separate access token implementations, like the above.

[sebastianfelipe]

In this moment that's not possible @guillob, it could be great if it exists someday. The architecture would change a lot. It would be necessary OR send the principal_type also from the access_token by query (or header) OR generate a duality access_token, for example -> new_access_token = <hash(principalType)>+access_token. Where the first 16bits for example, could be used to extract the principalType. That means, we should define in the middleware.json file something like.

"auth": {
    "loopback#token": {
      "params": {
        "models": ["CustomAccessToken1", ..., "CustomAccessTokenN"]
      }
    }
  }

Well, that is to avoid sending the principalType named as it is. Having this perspective, we could extract the principalType hash from the new_access_token (probably it still have the name of access_token, but is to differentiate it in this thread). Then, when the new_access_token arrives at the server, there are N possibilities (probably less than 5), to relate the CustomAccessTokenX model and make the search. That would need a little computational memory resource, but it won't need too much because generally are just a few CustomAccessTokens.

To test the hashed principalType part in the new_access_token, would be similar to the User.hash validator when a user logged in.

Pseodocode:

function getAccessToken(newAccessToken, cb) {
    var hashedPrincipalType, accessTokenId, models, customModel;
    hashedPrincipalType = getHashedPrincipalTypeFromNewAccessToken(newAccessToken);
    accessTokenId = getAccessTokenIdFromNewAccessToken(newAccessToken);
    // Step 1: Get the model hashed
    models = auth["loopback#token"].params.models;
    for (var i = 0; i < models.length; i++)
    {
        // For example
        if (hashedPrincipalType.isModel(models[i]))
        {
            customModel = models[i];
            break;
        }
    }
   if (!customModel) cb(new Error("Invalid access token"));
   // Step 2: Now that we now the model, then we need to find
   // the accessToken register from the accessTokenId
   Server
    .models[customModel]
    .findById(accessTokenId)
    .then(function (res) {
        cb(null, res);
    })
    .catch(function (err) {
        cb(err);
    })
   return cb.promise;
}

Well, or something like that. Is just to illustrate what I think it could be.

[sebastianfelipe]

@kjdelisle, time ago I tried that kind of thought but it is not possible do it right now, because Loopback search the accessToken in just one model, and no in the related ones. A possible solution is change how it get the model to search and the login function in the User model. Something like the comment above.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.