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

User Can Read Token Material for Other Users When Creating Token #11091

Closed
bryanculver opened this issue Dec 2, 2022 · 10 comments
Closed

User Can Read Token Material for Other Users When Creating Token #11091

bryanculver opened this issue Dec 2, 2022 · 10 comments
Assignees
Labels
status: under review Further discussion is needed to determine this issue's scope and/or implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@bryanculver
Copy link

NetBox version

v3.3.9

Python version

3.8

Steps to Reproduce

  1. Assume a user (user1, with id of 1) has superuser permissions
  2. Have a user (user300) with the add_token permission without constraint, no other permissions (not staff or superuser)
  3. user300 issues themselves a token (either via the UI, or endpoint discussed below)
  4. user300 sends a POST to ://netbox/api/users/tokens/ with the payload below:
    • {"user": 1}
  5. user300 is presented with a token issued for user1, with entire key material data
  6. user300 will only be able to see the key material once, as the key is not associated to their account the query for /api/users/tokens is restricted to their user
  7. If user1 doesn't exist, or does not have superuser permissions, user300 can continue incrementing the user ID until one is found.

Expected Behavior

At the very least user300 with the add_token may be able to create a token for other users who may have a higher privilege than themselves but should not be able to receive the response payload as they do not have view_token permissions.

Ideally, the add_token permission would not be able to allow a user to create tokens for users with higher permissions than themselves.

Observed Behavior

user300 is able to receive the token for user1 and then impersonate that user who may be a superuser, including changing the password.

This can be confirmed in the demo environment.

@bryanculver bryanculver added the type: bug A confirmed report of unexpected behavior in the application label Dec 2, 2022
@bryanculver
Copy link
Author

This is following up from a disclosure to the security email group which redirected us here.

A user having the add_token permission is less likely if the first install/use of tokens was post Netbox 3.0. Netbox 3.0 no longer requires this permission to be applied to users as a bugfix was implemented to address the perceived expected behavior that users should not need a permission to issue their own tokens (see: https://github.com/netbox-community/netbox/issues/6073). The add_token permission may still exist for users from pre-3.0 installations.

A permission can be scoped to limit a user’s ability to only create tokens owned by themselves (constraint like {"user__username": "theuser"}). Until Netbox 3.3 there was no way for a permission to reference the requesting user when evaluating the permission, which would require a permission created uniquely for each user. No best-practice documentation has been identified showcasing the danger of applying this permission without constraints (current user interface does not highlight this as a potential security concern in the API).

TokenViewSet restricts users from seeing other users tokens unless they are superuser: https://github.com/netbox-community/netbox/blob/v3.3.9/netbox/users/api/views.py#L48-L65 so the create method could be overloaded to apply the same restriction before returning the response to the requested user.

@kkthxbye-code
Copy link
Contributor

At the very least user300 with the add_token may be able to create a token for other users who may have a higher privilege than themselves but should not be able to receive the response payload as they do not have view_token permissions.

But that's the entire purpose of the feature. Some other system having the possibility of provisioning tokens for other users and then either providing the token or using the token to perform actions as the user. I agree that it's an inherently dangerous functionality. As such I'm having a hard time seeing this as a bug, seems more like a feature request to suggest removing the feature.

No best-practice documentation has been identified showcasing the danger of applying this permission without constraints

Are you interested in providing a PR for the updated documentation?

For reference this is what's currently there:

All users can create and manage REST API tokens under the user control panel in the UI. The ability to view, add, change, or delete tokens via the REST API itself is controlled by the relevant model permissions, assigned to users and/or groups in the admin UI. These permissions should be used with great care to avoid accidentally permitting a user to create tokens for other user accounts.

@kkthxbye-code kkthxbye-code added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Dec 18, 2022
@moonrail
Copy link

I am a little shocked to be honest, by how easy it is for admins to misuse this edge-case-feature.
I as an admin was not aware of this models CRUD handling being any different from other models and have to say, that I think this "dangerous functionality" - as you've called it yourself - is implemented fairly poorly in comparision to best practices NetBox itself, REST and other applications apply.

I mean - a permission for a non superuser that grants the user the permission to imitate a superuser. At least this has to be a red flag for this feature or lead to a requirement, that superusers can configure it in any way other than create a single object permission definition for token endpoint per user with restriction to allowed users.

Either a seperate endpoint or a seperate permission set would've been valid and avoided any misunderstanding.

Additionally the UI actually currently does require token permissions for users to be able to not only create, but also manage their tokens, see Screenshots below.

Without CRUD on token model:
Screenshot_20230113_154238

With CRUD on token model:
Screenshot_20230113_154348

And here the superuser imitation from a user with token Create-Permission:
Screenshot_20230113_155151

@jeremystretch
Copy link
Member

Additionally the UI actually currently does require token permissions for users to be able to not only create, but also manage their tokens, see Screenshots below.

To clarify, users can create tokens for themselves via the UI without the assignment of any specific permissions. This is intentional per #6073/#8436 and noted in the REST API documentation. You're correct in that explicit permission assignment is needed for a user to edit/delete his or her own tokens via the UI, and I've opened #11491 to capture this as a bug.

As @kkthxbye-code points out above, the ability to enable a user to create tokens on behalf of other users is intentional and desired in certain use cases. I'm happy to consider potential safeguards against the accidental misuse of this feature, but so far no specific implementation has been proposed.

@moonrail
Copy link

Proposal:

  • by default all token-endpoint usage is restricted to the user of the used API-Token
    • NetBox could apply a simple permission constraint (user__username) to the permission by default
  • if an administrator explicitly defines a constraint on the permission, the default permission constraint is dropped by NetBox and the administrator has to define appropiate constraints to which users this may apply
    • the administrator could just compare any user attribute via regex and a wildcard and achieve the same functionality as the default is now

@jeremystretch
Copy link
Member

@moonrail we'll have to dig into the implementation details to determine feasibility, but IMO that sounds reasonable. Would you mind opening a feature request to capture this please?

@kkthxbye-code
Copy link
Contributor

An alternative might be requiring a different action to create tokens for other users, like impersonate or something, like we require the run action for running scripts/reports.

Haven't looked at the current implementation though, so not sure what is more feasible.

@jeremystretch
Copy link
Member

@kkthxbye-code ooh I like that idea, it would be a very natural use of custom permission actions.

@moonrail
Copy link

Separate permission set sounds perfectly fine to me - this also cannot be used by accident, as the Admin UI does not show these as checkboxes.

@jeremystretch
Copy link
Member

@kkthxbye-code's proposal above has been captured under #12207. Closing this out as no further action is required.

@jeremystretch jeremystretch closed this as not planned Won't fix, can't repro, duplicate, stale Apr 7, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: under review Further discussion is needed to determine this issue's scope and/or implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

No branches or pull requests

5 participants