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

[QA] Browser session is valid for newly created user of same name #904

Open
jnweiger opened this issue Nov 19, 2020 · 18 comments · Fixed by #6326 or #6338
Open

[QA] Browser session is valid for newly created user of same name #904

jnweiger opened this issue Nov 19, 2020 · 18 comments · Fixed by #6326 or #6338
Labels
Category:Research Research is needed Interaction:Needs-help Asking some hints to engineering when the issue can't be reproduced Priority:p3-medium Normal priority Status:Housekeeping Type:Bug

Comments

@jnweiger
Copy link
Contributor

Setup via docker-compose-eos-test.yml from branch fix-yml-for-rc5 on localhost
From the WEB UI:

  • moss creates user testy
  • in a different browser session, testy logs in, and creates some folders or files.
  • moss deletes user testy
  • the browser session of testy no longer works, error messages appear.
  • moss creates a new testy user
  • the browser session of the original testy works again, the new contents is seen.

This is a corner case, but may lead to access to a wrong account's data.

Expected behaviour: sessions remain invalidated forever, once the user is deleted.

@jnweiger
Copy link
Contributor Author

jnweiger commented Nov 20, 2020

@C0rby maybe exploitable like this:

  • assume an intrudor got control of a user account
  • admin deletes and recreates the user account to kick out the intrudor
  • intrudor changes the accounts password before the real user can access.
    ... don't know what els can be done with that :-)

@C0rby
Copy link
Contributor

C0rby commented Nov 20, 2020

Oooh, that is interesting and tricky since oidc is a stateless mechanism AFAIK and there is no central "session" we could invalidate.
This requires some research.

@exalate-issue-sync
Copy link

David Christofas commented: So after a bit of research I found the following options:

  • We could include the accounts uuid in the jwts (only possible if the access token is a jwt) and on every request check that the affected user's id matches.
  • The idp needs to hold a blacklist with invalidated access tokens. This requires the idp to synchronize the blacklist if it is setup in a distributed manner. Also when an admin deletes a user the admin has no access to the users access token and therefore can not tell the idp which token to invalidate.

Both options are not really satisfying. I think this issue requires some more brainstorming together with the team.

@exalate-issue-sync
Copy link

Jörn Friedrich Dreyer commented: optimal solution: idp sends the ownclouduuid property that we can rely on.
if the idp cannot send arbitrary claims (eg konnectd): when creating accounts we need to store the sub+iss with the account (our accounts have an identities property that can be used for that). then we can double check if the sub+iss still matches on logins.

@exalate-issue-sync
Copy link

exalate-issue-sync bot commented Dec 11, 2020

Ilja Neumann commented: Contacted upstream with the proposal of [~jfd]. Meanwhile we could quickly implement this feature in a fork and use that until the change has landed it upstream. We done this before.

@settings settings bot removed the bug label Jan 12, 2021
@refs refs changed the title [QA] browser session is valid for newly created user of same name [QA] Browser session is valid for newly created user of same name Jan 13, 2021
@refs refs added Category:Research Research is needed Interaction:Needs-help Asking some hints to engineering when the issue can't be reproduced labels Jan 13, 2021
@individual-it
Copy link
Member

individual-it commented Jan 26, 2021

  • ToDo QA-team: think about if we can test this kind of scenario and if yes add tests

@SwikritiT
Copy link
Contributor

SwikritiT commented Dec 14, 2022

This issue still exists

  1. create a user test
  2. as the user test login
  3. delete the user test
  4. re-create the user test
  5. reload the browser of the previously logged-in user test
  6. the page is in loading state but reload again and you're logged in as a newly created user
simplescreenrecorder-2022-12-14_10.36.08.mp4

started ocis with docker
image: owncloud/ocis:latest

@SwikritiT SwikritiT reopened this Dec 14, 2022
@micbar micbar added the Priority:p2-high Escalation, on top of current planning, release blocker label Dec 14, 2022
rhafer added a commit that referenced this issue May 23, 2023
Reconfigure the oidc clients for lico, so that lico adds the "lg.uuid" to
tokens and userinfo by default. That claim will contain the userid. So
we can now use the userid for matching users when using the default
idm/idp configuration. This fixes further problems so that users being
recreated with the same name are correctly treated as differnt users.

Fixes: #904
ownclouders pushed a commit that referenced this issue May 23, 2023
Reconfigure the oidc clients for lico, so that lico adds the "lg.uuid" to
tokens and userinfo by default. That claim will contain the userid. So
we can now use the userid for matching users when using the default
idm/idp configuration. This fixes further problems so that users being
recreated with the same name are correctly treated as differnt users.

Fixes: #904
@C8opmBM
Copy link

C8opmBM commented May 29, 2023

Since my last update of ocis last week I believe, I am no longer able to login (access denied). Im using Authelia, and my setup has been working for a long time. I see no breaking changes, but now I'm being asked for a claim, which I never used.
I've been using this guide since the beginning of the year without problems: https://helgeklein.com/blog/owncloud-infinite-scale-with-openid-connect-authentication-for-home-networks/

Now I'm being spammed with:

`ERR claim not set or empty claim=lg.uuid claims={"amr":["pwd","otp","mfa"],"aud":["xdXOt13JKxym1B1QcEncf2XDkLAexMBFwiT9j6EfhhHFJhs2KM9jbjTmf8JBXE69","xdXOt13JKxym1B1QcEncf2XDkLAexMBFwiT9j6EfhhHFJhs2KM9jbjTmf8JBXE69"],"auth_time":1680888917,"azp":"xdXOt13JKxym1B1QcEncf2XDkLAexMBFwiT9j6EfhhHFJhs2KM9jbjTmf8JBXE69","client_id":"xdXOt13JKxym1B1QcEncf2XDkLAexMBFwiT9j6EfhhHFJhs2KM9jbjTmf8JBXE69","email":"[email protected]","email_verified":true,"iat":1683262123,"iss":"https://auth.xxx.net","name":"status quo","preferred_username":"xxxx","rat":1683262123,"sub":"95527cef-dac1-4407-a3be-4b613e8fc541"} line=github.com/owncloud/ocis/v2/services/proxy/pkg/middleware/account_resolver.go:63 service=proxy

I searched the docs, I've tried to add various options such the ones below, to no avail. Can you please advise?

      PROXY_USER_OIDC_CLAIM: "email"
      PROXY_ROLE_ASSIGNMENT_DRIVER: oidc
      PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM: roles
      PROXY_USER_OIDC_CLAIM: preferred_username

Using ocis web with docker, windows client and the android client. All latest versions.

rhafer added a commit to rhafer/ocis that referenced this issue Jun 1, 2023
This reverts commit 52951b4.

The change broke authentication for at least the desktop client when
using the builtin idp. There seem to be issues in the IDP (lico) which
result in the implicit scoped not being added correctly in some case.
When that scope is missing the `lg.uuid` claim will not be present in
the userinfo and we can correctly match users by id.

This reverts back to the old behaviour of matching users by name. Which
also brings some aspects of owncloud#904

Fixes owncloud#6368
rhafer added a commit to rhafer/ocis that referenced this issue Jun 1, 2023
This reverts commit 52951b4.

The change broke authentication for at least the desktop client when
using the builtin idp. There seem to be issues in the IDP (lico) which
result in the implicit scoped not being added correctly in some case.
When that scope is missing the `lg.uuid` claim will not be present in
the userinfo and we can correctly match users by id.

This reverts back to the old behaviour of matching users by name. Which
also brings some aspects of owncloud#904

Fixes owncloud#6415
rhafer added a commit to rhafer/ocis that referenced this issue Jun 1, 2023
This reverts commit 52951b4.

The change broke authentication for at least the desktop client when
using the builtin idp. There seem to be issues in the IDP (lico) which
result in the implicit scoped not being added correctly in some case.
When that scope is missing the `lg.uuid` claim will not be present in
the userinfo and we can correctly match users by id.

This reverts back to the old behaviour of matching users by name. Which
also brings some aspects of owncloud#904

Fixes owncloud#6415
rhafer added a commit that referenced this issue Jun 1, 2023
This reverts commit 52951b4.

The change broke authentication for at least the desktop client when
using the builtin idp. There seem to be issues in the IDP (lico) which
result in the implicit scoped not being added correctly in some case.
When that scope is missing the `lg.uuid` claim will not be present in
the userinfo and we can correctly match users by id.

This reverts back to the old behaviour of matching users by name. Which
also brings some aspects of #904

Fixes #6415
ownclouders pushed a commit that referenced this issue Jun 1, 2023
This reverts commit 52951b4.

The change broke authentication for at least the desktop client when
using the builtin idp. There seem to be issues in the IDP (lico) which
result in the implicit scoped not being added correctly in some case.
When that scope is missing the `lg.uuid` claim will not be present in
the userinfo and we can correctly match users by id.

This reverts back to the old behaviour of matching users by name. Which
also brings some aspects of #904

Fixes #6415
fschade pushed a commit that referenced this issue Jul 10, 2023
This avoids that recreating the user with the same name will create the
same "sub" claim. Even though it gets a new UUID

Fixes: #904
fschade pushed a commit that referenced this issue Jul 10, 2023
Reconfigure the oidc clients for lico, so that lico adds the "lg.uuid" to
tokens and userinfo by default. That claim will contain the userid. So
we can now use the userid for matching users when using the default
idm/idp configuration. This fixes further problems so that users being
recreated with the same name are correctly treated as differnt users.

Fixes: #904
fschade pushed a commit that referenced this issue Jul 10, 2023
This reverts commit 52951b4.

The change broke authentication for at least the desktop client when
using the builtin idp. There seem to be issues in the IDP (lico) which
result in the implicit scoped not being added correctly in some case.
When that scope is missing the `lg.uuid` claim will not be present in
the userinfo and we can correctly match users by id.

This reverts back to the old behaviour of matching users by name. Which
also brings some aspects of #904

Fixes #6415
@S-Panta
Copy link
Contributor

S-Panta commented Jul 19, 2023

Although a newly created user with the same name couldn't access the data of the previously created user of the same name, the browser session still remains logged in even after you change the entire credential (including password). Only when you change the username will the session redirects to logout.
When every detail of new user are changed except username

preview.mp4

When username is changed while keeping every detail same

preview2.mp4

@S-Panta S-Panta reopened this Jul 19, 2023
@SwikritiT SwikritiT removed their assignment Jul 20, 2023
@rhafer rhafer removed their assignment Dec 11, 2023
@rhafer
Copy link
Contributor

rhafer commented Jan 10, 2024

To really fix this, I think we need to address libregraph/lico#98 first. After that we can bring back the original fix for the issue: 52951b4

@dragonchaser
Copy link
Member

@micbar does it make sense to tackle this, since we are planning to move to another idp?

@micbar micbar added this to the Release 5.0.0 milestone Jan 22, 2024
@butonic butonic self-assigned this Feb 8, 2024
@DeepDiver1975
Copy link
Member

As just discussed and agreed on:

  • the access token has a life time of 5 minutes
  • the user cannot perform critical operations like change password
  • the user has no access to any space

-> the old user has at most 5 mins to view personal files of the new user and these files first need to be uploaded
-> this is a very unlikely attack vector and even if the information leakage is pretty low

BUT: this scenario has to be retested when the idp is changed

@micbar micbar added Priority:p3-medium Normal priority and removed Priority:p2-high Escalation, on top of current planning, release blocker labels Feb 8, 2024
@micbar micbar removed this from the Release 5.0.0 milestone Feb 8, 2024
@butonic butonic removed their assignment Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Research Research is needed Interaction:Needs-help Asking some hints to engineering when the issue can't be reproduced Priority:p3-medium Normal priority Status:Housekeeping Type:Bug
Projects
Status: Prio 3 or less