Wrong session auth handler order

[pboguslawski]

Description

According to #18452 (comment) session auth handler should probably be called before all other auth handlers (i.e. basic auth, reverse proxy, etc.) to avoid costly auth on every web request (i.e. LDAP flooding when used together with reverse proxy auth).

Seems root of the #27821 problem is not auth handler order; session should be simply destroyed on user logout (or page close in case of SSO scenarios) to disallow reusing same session by another user.

Gitea Version

1.23+

Can you reproduce the bug on the Gitea demo site?

No

Log Gist

No response

Screenshots

No response

Git Version

No response

Operating System

No response

How are you running Gitea?

compiled from source

Database

None

[wxiaoguang]

According to #18452 (comment) session auth handler should probably be called before all other auth handlers (i.e. basic auth, reverse proxy, etc.) to avoid costly auth on every web request (i.e. LDAP flooding when used together with reverse proxy auth).

Thank you for pointing out. #18452 is from an org's account. Due to GitHub's limit, it can't be edited by maintainers, so no maintainer could help to update or improve.

Would you like to re-target that PR from a individual's account?

---

Seems root of the #27821 problem is not auth handler order; session should be simply destroyed on user logout (or page close in case of SSO scenarios) to disallow reusing same session by another user.

Could you elaborate or show an example? A related case in my mind is:

• User A login SSO, gets "sso session a"
• Then they login Gitea via "sso session a", and get "gitea session x"
• User A logout SSO, and User B login SSO, gets "sso session b" (in the same browser)
• Then User B visit Gitea with "gitea session x" and "sso session b"
  • Since Gitea sees "gitea session x", so it still use that User A's session

Does it make sense?

[pboguslawski]

Due to GitHub's limit, it can't be edited by maintainers, so no maintainer could help to update or improve.

Feel free to copy/fork this mod to your own PR and adjust to your needs.

Could you elaborate or show an example?
Then User B visit Gitea with "gitea session x" and "sso session b"

Browser must not allow user B to reuse "gitea session x" created by user A. Session data should be accessible only to its owner (user A in case of "gitea session x") and if user B can access it - it's security hole and should be fixed there not in auth handler order.

Session should be created after successful user auth with other methods (i.e. basic auth, reverse proxy auth, 2fa, etc.) and allow to avoid such costly user auth on every other request in this session so probably should be placed on top of auth handler list.

Session should be automatically purged when not active for X hours to force user reauthentication (new session).

[wxiaoguang]

Due to GitHub's limit, it can't be edited by maintainers, so no maintainer could help to update or improve.

Feel free to copy/fork this mod to your own PR and adjust to your needs.

TBH I don't need it, and if there are no enough interests and you don't need it, I might not spend time on it.

---

Browser must not allow user B to reuse "gitea session x" created by user A.

That's the problem. We can't control browser or SSO.

In this step:

User A logout SSO, and User B login SSO, gets "sso session b" (in the same browser)

User A only logout from SSO site, but the browser still store "gitea session x" in its cookie. Unless the SSO provider could help to revoke the "gitea session x", the following visits to Gitea instance will still use the old gitea session.

Yes, it is a security problem, need many methods to ensure correct.

[pboguslawski]

That's the problem. We can't control browser or SSO.

It's not web application job to teach user to use secure client software (SSO, OS, browser) and gitea should not waste user resources for quirks like this IHMO (LDAP auth flooding in this case).

User A only logout from SSO site, but the browser still store "gitea session x" in its cookie.

Session cookie should be accessible only from users A account (regardless of if it's SSO or any other technology). If such cookie is accessible for other users also - consider posting an issue to such buggy SSO/OS/browser provider (to fix it and warn their users) and don't try to pollute web application code with workarounds.

[wxiaoguang]

My understanding of the problem is: although the current approach (session handler last) is not efficient, at least the result is right. Because "reverse auth proxy" can't clear Gitea's session. Gitea's auth handler should be able to handle the account switch.

But if you'd like to change the session handler's order, then it would become the wrong result I described above.

If we'd really like to improve it, at the moment the only feasible approach in my mind is: refactor the whole auth system, and make each auth handler could detect whether there is a account switch (session revoked), before the "verify" step.

[pboguslawski]

Gitea's auth handler should be able to handle the account switch.

Why? Do you know any professional environment that allows one user to access session cookie of other user and does not see point in fixing it ASAP?

[pboguslawski]

BTW: consider using session cookies by default to destroy session cookie when user closes browser (must reauthenticate on next application use). Persistent cookies should be optin IMHO.

[wxiaoguang]

Gitea's auth handler should be able to handle the account switch.

Why? Do you know any professional environment that allows one user to access session cookie of other user and does not see point in fixing it ASAP?

Do you know any professional reverseproxy auth provider could clear Gitea's session cookie?

BTW: consider using session cookies by default to destroy session cookie when user closes browser (must reauthenticate on next application use). Persistent cookies should be optin IMHO.

The problem is that some users just switch account without closing the current browser (that's what I meant "We can't control browser or SSO")

[pboguslawski]

The problem is that some users just switch account without closing the current browser

That's interesting. Any example of real environment that allows changing reverse auth login without closing browser?

[wxiaoguang]

But there is no standard/RFC/spec saying that "switching account must close browser".

Actually end users don't care closing or not, they only follow their intuition: we don't close browser when switching GitHub account, gmail account, facebook account, AWS account, and even back account, etc. So "requiring end users to close browser to re-login Gitea" is quite counter-intuitive and seems more likely a bug.

[pboguslawski]

So "requiring end users to close browser to re-login Gitea" is quite counter-intuitive and seems more likely a bug.

My understanding is that reverse proxy auth is used in professional environments (i.e with kerberos or client certs) when users do not leave their devices unattended (i.e. unlocked, logged in).

If we'd really like to improve it, at the moment the only feasible approach in my mind is: refactor the whole auth system, and make each auth handler could detect whether there is a account switch (session revoked), before the "verify" step.

If you see risk of stealing session cookie such improvement makes sense. What about removing

group.Add(&auth_service.Session{})

from buildAuthGroup and passing session object in every Verify call to let handlers decide what to do (i.e. reverse proxy auth handler may compare uid from header with uid from session object and create new session if different)?

[wxiaoguang]

If you see risk of stealing session cookie such improvement makes sense. What about removing

group.Add(&auth_service.Session{})

from buildAuthGroup and passing session object in every Verify call to let handlers decide what to do (i.e. reverse proxy auth handler may compare uid from header with uid from session object and create new session if different)?

Agree, I think that's more or less similar to my proposal (#34174 (comment)):

If we'd really like to improve it, at the moment the only feasible approach in my mind is: refactor the whole auth system, and make each auth handler could detect whether there is a account switch (session revoked), before the "verify" step.