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

Block users from editing important fields in user settings docs #8886

Closed
garethbowen opened this issue Feb 20, 2024 · 4 comments
Closed

Block users from editing important fields in user settings docs #8886

garethbowen opened this issue Feb 20, 2024 · 4 comments
Assignees
Labels
Type: Security Affects security
Milestone

Comments

@garethbowen
Copy link
Member

garethbowen commented Feb 20, 2024

What feature do you want to improve?

Currently users can edit their own user settings doc in the medic db. This is usually not a big issue because on the server we only trust the data in the _users database, but it would be easy to regress and use the wrong one by mistake, but also it would be useful to allow read only access at times (recent example). Also if the medic version gets out of sync with the users version then the application can behave in unexpected ways.

Describe the improvement you'd like

Disallow editing of the user settings doc except by API or db admin users.

There are two ways I can think of that this doc may get modified - either directly to couchdb by a malicious user with curl, or unintentionally by a well meaning developer storing the doc in pouchdb and replicating.

The final gatekeeper for the database is validate_doc_update which will block both approaches, but I think this will permanently block upwards replication for anyone who has an update (needs testing). If we can couple this with a client side check to stop pouchdb getting in an inconsistent state then it may work.

Describe alternatives you've considered

Long term we should block direct access to the db, and use APIs for everything which would allow us to perform custom validations on writes, but this would be a breaking change so doesn't solve the problem in the near term. It also doesn't solve for accidental updates by well intentioned developers.

Additional context

There may be legitimate reasons for updating this doc (eg: "known" and "privacy policies") either in use, or historically used. The historical part is important because if the server is upgraded and gets the new gate, but there's an old doc sitting on a phone, we need to make sure they can recover.

Severity

Low because the user can only edit the shadow of the permissions. We should only use the permissions in the _users db on the server so the medic copy isn't especially important.

@garethbowen
Copy link
Member Author

Replication steps for the two methods. Use a "normal" (not db admin) user that shouldn't have elevated rights.

curl

curl -H 'Content-Type: application/json' --data @./usersettings.json http://<USERNAME>:<PASSWORD>@<API_URL>/medic

usersettings.json:

{
  "_id": "org.couchdb.user:<USERNAME>",
  "_rev": "<REV>",
  "name": "<USERNAME>",
  "type": "user-settings",
  "roles": [
    "chw","SUPER_ADMIN_ROLE"
  ],
  "facility_id": "474bc59c-b38f-47d2-9b2e-3de5ac96dd31",
  "contact_id": "ec7918fa-f150-4d79-b15d-022f6b95d321",
  "phone": "+64274622223"
}

Expected: The request is blocked
Actual: The doc is updated

Pouch

  1. Login as a normal user.
  2. Open the dev console.
const db = new window.PouchDB('medic-user-<USERNAME>');
db.get('org.couchdb.user:gareth').then(console.log);
db.put(<THE DOC FROM THE PREVIOUS LINE WITH ROLE UPDATED>).then(console.log);

Expected: The put command throws an exception
Actual: The command succeeds and after replication the updated role exists on the server.

@mrjones-plip
Copy link
Contributor

@garethbowen - added a "severity" section above - feel free to edit if it's not accurate!

@garethbowen
Copy link
Member Author

@mrjones-plip Updated. The first line of the issue description explains my perspective.

@mrjones-plip
Copy link
Contributor

ah - thank you! I've updated the release notes accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Security Affects security
Projects
None yet
Development

No branches or pull requests

2 participants