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 - create submission #1551

Closed
songz opened this issue Mar 6, 2022 · 0 comments · Fixed by #1553
Closed

Security - create submission #1551

songz opened this issue Mar 6, 2022 · 0 comments · Fixed by #1553
Assignees
Labels
bug Something isn't working security urgent

Comments

@songz
Copy link
Contributor

songz commented Mar 6, 2022

After I login on c0d3 cli using c0d3 login:

  1. print out ~/.c0d3/credentials.json
  2. base64 decode it. Notice that it is an object with id and token.

Screen Shot 2022-03-06 at 7 05 19 AM

Looking at our mutation for creating submission, it seems like we are just decoding the cliToken and getting the id field to use as user's id.

Problem: So if I was a malicious user, I could simply change the id in the file and then submit as any user I want.

Here's how I would do it.

  1. Base64 encode: {id: <hackedUserId>} - Seems like cliToken isn't even used so we don't even need that.
  2. Paste the base64 string into ~/.c0d3/credentials.json
  3. Submit.

Proposal:

  1. In my experience, tokens should be sent in the request header. Perhaps similar to chatroom jwt auth in js5. Bearer Authorization type seems reasonable because bearer token means an opaque string, not intended to have any meaning to clients using it.
  2. In our user middleware, check to see if user's Authorization Header is present. If so, lookup user by the cliToken field, which should be indexed for fast lookups, and populate req.user object based on the cliToken.
  3. remove cliToken check from create submission and simply use req.user object.

Other thoughts

We ideally also want to remove cliToken from createSubmission typeDef but that may break all existing c0d3 CLI.

Perhaps we could create an announcement, giving 1 week to upgrade?

Maybe we should have this issue looked into as well so we are better prepared for potential CLI breakage in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants