Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Privilege escalation #336

Open
ojeytonwilliams opened this issue Nov 15, 2023 · 2 comments
Open

Privilege escalation #336

ojeytonwilliams opened this issue Nov 15, 2023 · 2 comments
Labels
bug Something isn't working MVP scope: backend

Comments

@ojeytonwilliams
Copy link
Contributor

ojeytonwilliams commented Nov 15, 2023

It's currently possible to update your own user data via PUT requests to api/users/me. The only requirement is that you have the Contributor role and be signed in. Once you meet those requirements you can PUT { "role": "a-new-role-id" } and give yourself whichever role you would like.

Mitigations

We don't currently have any need for users to be able to update their role, so we should prevent that entirely.

Also, do we need to create the api/users/me route at all? It might be simpler if we just used the api/users/{id} route and forbade access if the user id did not match the current user's id.


Alternatively we could take the opposite approach: rather than restricting how a user can modify their account, we could remove all existing permissions and build a route that allows them to a) modify only their user data and b) only make specific modifications.

I don't really know strapi well enough, though, so it's unclear to me if there's a canonical way to handle this (though we can't be the first users that want to!)

@sidemt
Copy link
Member

sidemt commented Nov 20, 2023

Thank you for catching this issue, Oliver!

I guess there should be some ways to override the behavior of PUT api/users/me endpoint so that we can prevent updates to specific fields when the request is made by Contributor users, something similar to what I did in here:

module.exports = createCoreController("api::post.post", ({ strapi }) => ({
async create(ctx) {
if (!isEditor(ctx)) {
// don't allow publishing or scheduling posts
delete ctx.request.body.data.publishedAt;
delete ctx.request.body.data.scheduled_at;
}
// call the default core action with modified data
return await super.create(ctx);
},

The difficulty is that PUT api/users/me endpoint is provided by users-permissions plugin so it's different from other core controllers as I commented in #138

@ojeytonwilliams
Copy link
Contributor Author

The users-permissions plugin does provide the PUT /users/{id} endpoint, but the PUT /users/me endpoint is a custom one that we've created. It's up to us how it behaves.

I think the simplest approach is to allow everyone to make specific changes to their own user record, but forbid all changes to your own role. Since it still needs to be possible to change roles, we can have an Administrator role that grants you access to PUT /users/{id}. That way even Administrators have to use PUT /users/{id} if they want to change their own role.

The long and short of it is your role is so important that we should only have one way (one endpoint) to change it and be very careful about which role(s) are allowed to use that endpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working MVP scope: backend
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants