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

[Security] Generic OAuth allows anyone to authenticate as a user if map_user_id is not configured correctly #469

Closed
PeterCxy opened this issue Jun 21, 2021 · 5 comments
Assignees
Labels
Milestone

Comments

@PeterCxy
Copy link

@PeterCxy PeterCxy commented Jun 21, 2021

Describe the bug

I was attempting to configure Generic OAuth for my WriteFreely instance with my Mastodon instance. Initially, I followed what's described on https://writefreely.org/docs/latest/admin/config, and everything seemed to be working. I could link my Mastodon account on WriteFreely, and I could log into my WriteFreely account with my Mastodon account just fine... until I noticed that everybody that has an account on my Mastodon instance now authenticates as me on the WriteFreely instance.

A close inspection on the database revealed that the remote_user_id field in the database was empty (null). This resulted in the single record being matched for every OAuth login request, thus resulting the aforementioned behavior.

After digging around in the source code, I realized that Mastodon does not return the user id in the field user_id, but instead in id, and WriteFreely needs a special configuration map_user_id for that (and also a few other map_* options). These are mentioned nowhere in the documentation.

So basically, the issue here is:

  1. The OAuth code should not appear as being working fine when in fact being horribly broken (i.e. when remote_user_id cannot be fetched). There should be some big red error / warning in the logs to notify the administrator of the situation, instead of having to dig around in the source code to find the solution.
  2. The map_* options should be included somewhere noticeable in the documentation.

Steps to reproduce (if necessary)

  1. Set up OAuth authentication with Mastodon without providing map_user_id
  2. Link any Mastodon account with any WriteFreely account
  3. Notice that now any Mastodon account registered on the linked instance can authenticate with said WriteFreely account

Expected behavior

  1. Step 2 in "Steps to reproduce" should have failed in the first place.
  2. Step 3 in "Steps to reproduce" should also fail (it should not match records with remote_user_id being null)

Version or last commit: 0.13.0

@PeterCxy PeterCxy changed the title Generic OAuth allows anyone to authenticate as a user if map_user_id is not configured correctly [Security] Generic OAuth allows anyone to authenticate as a user if map_user_id is not configured correctly Jun 21, 2021
@thebaer thebaer modified the milestones: 0.13, 0.13.1 Jun 21, 2021
@thebaer thebaer added the bug label Jun 21, 2021
@thebaer
Copy link
Member

@thebaer thebaer commented Jun 21, 2021

Thanks for the report, @PeterCxy -- this is something we definitely need to fix. To help us replicate and debug this quicker, can you share your generic OAuth config?

@PeterCxy
Copy link
Author

@PeterCxy PeterCxy commented Jun 21, 2021

@thebaer Sure. My config is quite basic

[oauth.generic]
client_id = <redacted>
client_secret = <redacted>
host = https://sn.angry.im
display_name = Mastodon on SN.Angry.Im
token_endpoint = /oauth/token
inspect_endpoint = /api/v1/accounts/verify_credentials
auth_endpoint = /oauth/authorize
scope = read:accounts
allow_disconnect = false

(sn.angry.im is just a Mastodon instance I run)

This configuration results in said bug because /api/v1/accounts/verify_credentials of Mastodon returns user id in the id field, not the default user_id that WriteFreely expects, resulting in remote_user_id being null. Adding map_user_id = id fixed it, as mentioned in the original issue.

thebaer added a commit that referenced this issue Jun 23, 2021
...on the OAuth access token inspection call. This returns an error and
privately (via logs) prompts the admin to add a `map_user_id` config value.

Fixes #469
@thebaer thebaer self-assigned this Jun 23, 2021
@thebaer
Copy link
Member

@thebaer thebaer commented Jun 23, 2021

Thanks! I configured one of my instances for Mastodon, tested this all out, and fixed this in #474. Now we'll return an error and a helpful log message when there's no user_id at Step 2 of your "steps to reproduce." Would love to have your thoughts / review on that fix, if you don't mind.

@PeterCxy
Copy link
Author

@PeterCxy PeterCxy commented Jun 23, 2021

@thebaer The fix looks good to me. BTW, maybe the admin documentation on writefreely.org should also be updated with those mapping options?

@thebaer
Copy link
Member

@thebaer thebaer commented Jun 24, 2021

@PeterCxy yes, I'll open a PR for that on our documentation repo soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants