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: Recovering from a GitHub account compromise #2630

Open
jsha opened this issue Jul 12, 2020 · 5 comments
Open

security: Recovering from a GitHub account compromise #2630

jsha opened this issue Jul 12, 2020 · 5 comments
Labels
C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works

Comments

@jsha
Copy link
Contributor

jsha commented Jul 12, 2020

Right now, if an attacker compromises a crate author's GitHub account, they can use it to log into crates.io and get a session cookie. That cookie is signed by an HMAC key owned by crates.io, and contains the user ID. During authentication, crates.io verifies the HMAC on that cookie and then trusts the user ID inside. The problem is that, once the crate author regains control of their GitHub account, the attacker's session cookie is still valid, and as far as I can tell there is no way to revoke it. There are a few possible solutions:

  • Instead of a signed cookie, use a sessions table that has unique tokens in it. This has the added advantage that the sessions table can include the IP address and User-Agent from which the session was initiated, so the user can review extant sessions and revoke them if necessary.
  • Expire the signed cookie after a period of time (Add expiration as part of signed data and enforce it conduit-rust/conduit-cookie#15). This is useful for defense-in-depth, but any reasonable expiration period would be too long to work for this purpose. When you want to revoke an attacker's access, you want to revoke it right away, not in a few days.
  • Add a generation number in the users table, and give the user an option to "sign out all other sessions," which would increment the generation number. The generation number would also be included in the signed cookie and required to match the current values in the table.

I think (1) is the best option but also the most work.

Another, closely related issue: If a user revokes the crates.io app on their GitHub account (for instance, as part of the cleanup after a compromise), it would be nice to temporarily disable all cookie sessions and tokens on their crates.io account until they reauthorize crates.io.

(Related: #815)

@Turbo87 Turbo87 added the C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works label Feb 11, 2021
@Turbo87
Copy link
Member

Turbo87 commented Feb 12, 2021

@jsha FYI we shortly discussed this during our weekly team meeting and we're considering to implement the session table suggestion. would you be available for reviews once we have something that works?

@jsha
Copy link
Contributor Author

jsha commented Feb 12, 2021

Excellent! Yes, I'd definitely be happy to help with reviews.

@jsha
Copy link
Contributor Author

jsha commented Nov 21, 2021

Another item in this area: It might be good to re-verify the GitHub OAuth token at publish time. If the OAuth token has been revoked, the publish should fail.

@Shnatsel
Copy link
Member

There technically is a way to recover: the HMAC key can be changed by crates.io. But that invalidates all the cookies for all users, and can only be performed by the crates.io team.

@adsnaider
Copy link

Hello! Jumping in as this issue is important to folks at Google (myself included). I've gone ahead and created a pull request (#4690) that addresses the issue.

Disclaimer: I used #3307 to help me get started, so thank you very much @Turbo87 for the help :)

The PR doesn't address some issues that I left opened as TODOs, here's he list of the next steps (assuming the PR gets merged)

  • We need to invalidate the old cookie based authentication.
  • We should probably clean up old/stale sessions. I remember reading a comment about using partitions to make this simpler?
  • We should make use of the timestamps, IP address, and user agent for authentication.

Feel free to add more and/or correct me if I'm wrong about any of the above items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

No branches or pull requests

4 participants