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

feat: Refined session handling #40543

Closed
wants to merge 1 commit into from

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Sep 20, 2023

Summary

Before

Remember me is handled with persisted tokens stored in preferences. At cookie login (session expired, cookie is still alive), the token is compared with the database values, token is replaced on success. The session id is also the password/token of the session token in the password. This process is falling apart with concurrency. First, only one process can win the token race. Rarely there are two, because tokens are not read and replaced in a transaction. But then there are also two new PHP sessions and the database can only be updated once.

sequenceDiagram
    Browser->>+Server: session1, token1
    Server-->>-Browser: OK: session1, token1
    Note over Browser,Server: No activity. PHP session expires.
    Browser->>+Server: session1, token1
    Server-->>-Browser: OK: session2, token2   
    Note over Browser,Server: No activity. PHP session expires.
    par
        Browser->>+Server: session2, token2
        Server-->>-Browser: OK: session3, token3
    and
        Browser->>+Server: session2, token2
        Server-->>-Browser: ERROR: session4, logout
    end
Loading

After

Remember me is still handled by a persisted token, but the token is a random one and it doesn't change throughout the session.

sequenceDiagram
    Browser->>+Server: session1, token1
    Server-->>-Browser: OK: session1, token1    
    Note over Browser,Server: No activity. PHP session expires.
    Browser->>+Server: session1, token1
    Server-->>-Browser: OK: session2, token1   
    Note over Browser,Server: No activity. PHP session expires.
    par
        Browser->>+Server: session2, token1
        Server-->>-Browser: OK: session3, token1
    and
        Browser->>+Server: session2, token1
        Server-->>-Browser: OK: session4, token1
    end
Loading

Transition

The session id can be used as the "random" token at migration. The logic will keep that session alive as well. New sessions will use a real random token.

TODO

  • Do
  • Testing
  • Testing
  • Testing

Checklist

* Remove secondary persistent remember me tokens
* Decouple session ID and session token

Signed-off-by: Christoph Wurst <[email protected]>
@kesselb
Copy link
Contributor

kesselb commented Sep 22, 2023

Nice one 👍

} catch (SessionNotAvailableException $ex) {
return;
}
$token = \OCP\Server::get(IRequest::class)->getCookie(self::COOKIE_SESSION_ID);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit scared ;)
Isn't that a cyclic dependency?

@come-nc
Copy link
Contributor

come-nc commented Sep 26, 2023

Would you have an overview of how this is stored in database?
This marks the end of storing login_token in preferences, right?

@Altahrim
Copy link
Collaborator

I have two questions on it:

  • what are the security implications? By reading it (really) quickly, it seems the remember me token is valid for 15 days. If stolen, someone can use it for a long time.
  • is there a possibility to lose data? In your "After" example, if data are added to session 3, they will be lost right?

@solracsf solracsf added this to the Nextcloud 28 milestone Oct 27, 2023
@ChristophWurst ChristophWurst removed this from the Nextcloud 28 milestone Oct 31, 2023
@ChristophWurst ChristophWurst added 0. Needs triage Pending check for reproducibility or if it fits our roadmap and removed 2. developing Work in progress labels Oct 31, 2023
@ChristophWurst
Copy link
Member Author

  • what are the security implications? By reading it (really) quickly, it seems the remember me token is valid for 15 days. If stolen, someone can use it for a long time.

Yes, replay attacks are possible with the current implementation.

@solracsf solracsf added this to the Nextcloud 28 milestone Nov 21, 2023
@blizzz blizzz mentioned this pull request Nov 22, 2023
5 tasks
@ChristophWurst ChristophWurst removed this from the Nextcloud 28 milestone Nov 22, 2023
@blizzz blizzz added this to the Nextcloud 29 milestone Nov 23, 2023
@ChristophWurst
Copy link
Member Author

Closing due to #40543 (comment). This need more time and thought.

@ChristophWurst ChristophWurst deleted the feat/refined-session-handling branch November 29, 2023 15:19
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tried to log in "user" but could not verify token
7 participants