Consumers should not be able to grant roles beyond those already assigned to user

Initial patch shows promising results.

This are the tests with some fixes.

I have implemented the fix for the claimed security concern. However I don't feel comfortable with this change.

I consider this to be a useful feature, maybe it could be better communicated, but it is documented here https://youtu.be/-xRBJvmg0qQ

The idea is that Simple OAuth site builders should create an ad-hoc role (or collection of roles) when creating a consumer, that any user can gain access to provided they know both the client ID and client secret. If a user wants to request additional roles they can do so.

In summary, with the current feature set (without this issue's patch), a user request can gain access to these roles:

- Any role the user already has configured in Drupal. This is protected with Drupal's username/password (or session).
- Any role the consumer has configured in Drupal. This is protected with the consumer's id/secret. While this is the same for all the users of a given app, this is distributed with the app and the user has no knowledge of it.

The only place where client secret is not required is when using the implicit grant (https://oauth2.thephpleague.com/authorization-server/implicit-grant/). However this is marked as insecure in Drupal and disabled by default. In order to enable it you have to declare you understand the security implications.

The question for the reporter and the security team is: do you think this is a sufficiently secure system? (you may have different expectations, but the question is about security)

Bear in mind that shipping the patch above implies a new major version + unpublishing old versions + code changes to the 3rd party apps that integrate with this module.

#8 greggles commented

re #6: I had the same feeling (that this "works as designed") when reading the initial report, but defer to the maintainers judgment on it. So, I agree this is a well documented, intentional feature that can remain in the module. One question is if it needs to be more documented somehow or more obvious somehow?

#9 JKerschner commented

I was kind of suspicious that this might be the intended functionality of the module (hence why I hadn't sent it straight to the security team from the get-go, and I had actually asked the module maintainer directly about it a few weeks ago but did not ultimately receive any explanation of how this was supposed to be used). The video documenting this I had missed in part because the many videos for this module all seem to be about various often out-of-date versions of the module (e.g., even that same video show "Administrator" as a selectable role when editing a client, which it no longer is). So, there isn't much in the way of written docs for this module, and the videos are out of date. This definitely seems like it needs better documentation, since the intuitive way of setting the module up seems to lead to some pretty big security concerns.

As to the functionality itself, I suppose my question is how is this module able to be used in the sort of use case I have described? The site I am working on has potentially 15+ roles on it, and users could have them in pretty much any combination. Given that, it isn't viable to create a separate Client for each potential combination of roles. I suppose it would work to have no roles at all configured on the Client and to have the frontend just send a list of all the available roles as the "scopes" to the consumer - it's just that then the frontend needs all the roles listed out, which feels like an unnecessary step.

I'm also curious, @e0ipso... you note that the "Implicit Grant" is disabled by default. Where would one go to enable/disable that? I'm not seeing anything in the module settings related to that sort of concept, and it looks like my code isn't currently sending along the client secret, so it seems like the Implicit Grant seems like it may already be disabled.

Finally, could we get Drupal.org user "camprandall" added to this issue? He's on the same developer team I am and is the one who first brought this issue to my attention, so it would be cool if we could loop him in on this.

Thanks for your help and for looking into this.

Hmm, I feel that the documentation is pretty accurate. There have been several minor improvements along the way after the videos have been recorded, but all the major concepts still stand accurate.

@greggles we could add more extra verbose documentation in the checkbox on the form explaining the implications of assigning a role to a consumer.

Also, based on the conversations. Can we make this issue public again?

#14 greggles commented

We should wait a bit for anyone on the team to object to making it public first (2 weeks is our standard).

#16 mlhess commented

As no one has objected, we can make this public and close the private issue. @ e0ipso do you mind doing that?

In this case it also means if you have role based oauth consumer, you will allow users to have roles which they dont have in reality.

This feature set is consistent with the OAuth 2 specification. You can read more at https://tools.ietf.org/html/rfc6749#section-3.3.

Fur sure the intended way cannot be, if user is not assigned to have x role, he will still get it if he just logins by using exact consumers.

This behavior is by design. It's so consumers can ensure access to certain necessary resources. The question is why are you giving more access to the consumer than what you intend?

In any case, this kind of feature cannot be in a design as it makes the authentication process insecure!

The team of security experts that looked at this do not agree with that statement. I do not agree either. I just think that your setup needs to be fixed. It's like granting administer site configuration to anonymous users, that's not a security issue with Drupal core it's a configuration choice.

I hope this clarifies things. I also see you are new in the community that's why I took part of my free time to respond to your concerns in more detail. Please, remember that mindfulness in communication style is appreciated when dealing with vonluneers.

And I think we should do this because it's intuitively what you would expect based on how Drupal roles are normally used.

This may be a common opinion, however this issue has been posted for a long time but there is no real traction to change things.

I am of the opposite opinion, I think that you should be able to specify the required permissions for the application to work. If you want that to be 0 permissions, you can do so very easily by configuring your consumer with a role empty of permissions.