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

fix CVE-2015-9284 per omniauth/omniauth#809 #926

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix CVE-2015-9284 per omniauth/omniauth#809 #926

wants to merge 1 commit into from

Conversation

wenzowski
Copy link

@wenzowski wenzowski commented Jul 25, 2019

The request phase of the OmniAuth Ruby gem is vulnerable to Cross-Site Request Forgery when used as part of the Ruby on Rails framework, allowing accounts to be connected without user intent, user interaction, or feedback to the user. This permits a secondary account to be able to sign into the web application as the primary account.

same fix as merged in Shopify/omniauth-identity#8
for details see https://nvd.nist.gov/vuln/detail/CVE-2015-9284

The request phase of the OmniAuth Ruby gem is vulnerable to Cross-Site Request Forgery when used as part of the Ruby on Rails framework, allowing accounts to be connected without user intent, user interaction, or feedback to the user. This permits a secondary account to be able to sign into the web application as the primary account.

same fix as merged in Shopify/omniauth-identity#8
@see https://nvd.nist.gov/vuln/detail/CVE-2015-9284
@ghost ghost added the cla-needed label Jul 25, 2019
@wenzowski
Copy link
Author

wenzowski commented Jul 25, 2019

Corporate CLA signed on behalf of Button Inc.

@wenzowski
Copy link
Author

@ajshepley any idea who to ping about the CLA?

@wenzowski
Copy link
Author

@casperisfine I don't know how to trigger CLA status check. It's been signed for both myself and my org. When I visit https://cla.shopify.com/ I see:

GitHub username has already signed a CLA

@ghost ghost removed the cla-needed label Aug 1, 2019
@casperisfine
Copy link
Contributor

For the CLA: Must have been a dropped hook or something, triggering a re-run fixed it.

As for the patch I really doubt it works. You can't just add the gem, you also have to make sure the request is initiated with a POST, which I'm pretty sure it isn't right now, and making it so is actually quite hard.

@wenzowski
Copy link
Author

That's too bad.

You're saying that since the redirect_to when the user is not authorized is still an http get is the reason this is hard?

Presumably redirecting to a splash page with a "login with github" button that sends a post would do the trick? I don't mind taking a stab at that if that's all it would be.

@casperisfine
Copy link
Contributor

You're saying that since the redirect_to when the user is not authorized is still an http get is the reason this is hard?

Yes, we'd need to show a page with a button to trigger the auth with a POST.

I don't mind taking a stab at that if that's all it would be.

I'd like to make sure it's actually needed first. I don't fully understand the implications of the CVE, but it seems very mild to me.

allowing accounts to be connected without user intent, user interaction, or feedback to the user. This permits a secondary account to be able to sign into the web application as the primary account.

That part I don't quite get, Shipit doesn't have roles or anything like that, and I can't think of any negative consequence about being authenticated "against your will".

@EiNSTeiN- I could use your help (or someone else from your team if you don't have time) to assert the criticity of this thing, and wether or not we should redirect to a form or not.

@EiNSTeiN-
Copy link
Contributor

@clayton-shopify would someone from your team be able to handle this?

@wenzowski
Copy link
Author

@casperisfine @EiNSTeiN- @clayton-shopify Would a splash page with a "login with github" button that sends a post be welcome here? Where did we land on figuring out if it's actually needed?

Being authenticated against your will would presumably allow an attacker to deploy commits that should otherwise be held back, no?

@EiNSTeiN-
Copy link
Contributor

Where did we land on figuring out if it's actually needed?

that will indeed work to fix this issue

@casperisfine
Copy link
Contributor

Being authenticated against your will would presumably allow an attacker to deploy commits that should otherwise be held back, no?

I don't think so no, the CSRF protection will prevent that. You'll only be able to generate GET requests.

@spy-v2 spy-v2 bot deleted the branch Shopify:master October 5, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants