Potential Highjacking Vulnerability #386

Closed
twoolie opened this Issue Jul 4, 2012 · 11 comments

Comments

Projects
None yet
3 participants
@twoolie

twoolie commented Jul 4, 2012

From the front page of reddit today: The Most Common OAuth2 Vulnerability

In this article It describes a way to force associate with someone else's account by sending them a disguised link or picture that causes them to load the callback url with your association code.

From reading the django-social-auth code, it seems we may be vulnerable to this attack.

The Article goes on to say that the best way to mitigate is to set the state parameter to a hash of session id going out, and check it coming back.

Is there any way that we can mitigate this attack within the current pipeline system, or does it require changes to each oauth2 backend?

@omab

This comment has been minimized.

Show comment
Hide comment
@omab

omab Jul 4, 2012

Owner

@twoolie, I've introduced the state parameter to OAuth2 backends, base backend defines, uses and checks it, but subclass can disable the process if needed like Github and Instagram backends that don't sent the parameter back :-/

Backends for Mail.ru, Odnoklassniki and Yandex weren't tested because I know nothing about Russian, I'll ask the contributors to check them. The rest were tested and worked as expected.

Thanks for pointing the issue, I saw the article earlier today but it banished from my mind at the end of the day.

Will leave this open until the Russian backends are checked.

Owner

omab commented Jul 4, 2012

@twoolie, I've introduced the state parameter to OAuth2 backends, base backend defines, uses and checks it, but subclass can disable the process if needed like Github and Instagram backends that don't sent the parameter back :-/

Backends for Mail.ru, Odnoklassniki and Yandex weren't tested because I know nothing about Russian, I'll ask the contributors to check them. The rest were tested and worked as expected.

Thanks for pointing the issue, I saw the article earlier today but it banished from my mind at the end of the day.

Will leave this open until the Russian backends are checked.

@twoolie

This comment has been minimized.

Show comment
Hide comment
@twoolie

twoolie Jul 4, 2012

@omab, for backends that do not support passing state through their service, you could use redirect_uri that points back at an url that contains some sort of hash.

twoolie commented Jul 4, 2012

@omab, for backends that do not support passing state through their service, you could use redirect_uri that points back at an url that contains some sort of hash.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Jul 6, 2012

Great job, guys! It seems you fixed it properly!

homakov commented Jul 6, 2012

Great job, guys! It seems you fixed it properly!

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Jul 6, 2012

@twoolie yeah, trick with hashed redirect_uri is good only if it's not checked for Exact match. I personally dislike agile redirect_uri and I hope in the future all redirect_uri will be static. 'state' is the best option if available. I hope those providers(shame on them) will add this future.

homakov commented Jul 6, 2012

@twoolie yeah, trick with hashed redirect_uri is good only if it's not checked for Exact match. I personally dislike agile redirect_uri and I hope in the future all redirect_uri will be static. 'state' is the best option if available. I hope those providers(shame on them) will add this future.

@twoolie

This comment has been minimized.

Show comment
Hide comment
@twoolie

twoolie Jul 9, 2012

@homakov actually, state has a defined purpose for allowing webapps, and to a lesser extent traditional server-side code the ability to control where the user is sent after returning from authorization. by using state to carry a hash, we are corrupting it's true purpose to fit our needs. It would be better if the Oauth2 spec carried an extra parameter for this purpose.

twoolie commented Jul 9, 2012

@homakov actually, state has a defined purpose for allowing webapps, and to a lesser extent traditional server-side code the ability to control where the user is sent after returning from authorization. by using state to carry a hash, we are corrupting it's true purpose to fit our needs. It would be better if the Oauth2 spec carried an extra parameter for this purpose.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Jul 9, 2012

@twoolie totally disagreed.
Oauth2 follows KISS rule. There is absolutely no problem to store 'returning point' in session or cookie or in redirect_uri - what ever. But only 'state' can help us to prevent CSRF. Also, CSRF protection MUST be implemented. Thus - state must be used for CSRF protection. And storing returning point is routine task. Mission of state was often misunderstood, a lot of manuals say 'state for returning point' and that is bullsh*t. I dislike this situation

homakov commented Jul 9, 2012

@twoolie totally disagreed.
Oauth2 follows KISS rule. There is absolutely no problem to store 'returning point' in session or cookie or in redirect_uri - what ever. But only 'state' can help us to prevent CSRF. Also, CSRF protection MUST be implemented. Thus - state must be used for CSRF protection. And storing returning point is routine task. Mission of state was often misunderstood, a lot of manuals say 'state for returning point' and that is bullsh*t. I dislike this situation

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Jul 9, 2012

I have nothing against additional parameter btw. I had similar thought, e.g. redirect_token. But it will work the same 'state' ..

perhaps, developers should combine it in one value, e.g. HASH_POINT and split '_' at the end.

Also, I think about getting rid of 'state' in Provider's callback. We have state in session, let's extract it and send it to obtain access_token, along with code and client credentials.

homakov commented Jul 9, 2012

I have nothing against additional parameter btw. I had similar thought, e.g. redirect_token. But it will work the same 'state' ..

perhaps, developers should combine it in one value, e.g. HASH_POINT and split '_' at the end.

Also, I think about getting rid of 'state' in Provider's callback. We have state in session, let's extract it and send it to obtain access_token, along with code and client credentials.

@omab

This comment has been minimized.

Show comment
Hide comment
@omab

omab Jul 9, 2012

Owner

Additional parameter is not possible on every case since some providers enforce the redirect_uri check between the request value and the one defined in the app configuration, Google does it and fails showing a "redirect_uri mismatch" error page.

Owner

omab commented Jul 9, 2012

Additional parameter is not possible on every case since some providers enforce the redirect_uri check between the request value and the one defined in the app configuration, Google does it and fails showing a "redirect_uri mismatch" error page.

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Jul 9, 2012

@omab additional parameter is not passed in redirect_uri, it's passed along with other params - redirect_uri, scope, state.
and it's returned back with code. No problem with static redirect_uri

homakov commented Jul 9, 2012

@omab additional parameter is not passed in redirect_uri, it's passed along with other params - redirect_uri, scope, state.
and it's returned back with code. No problem with static redirect_uri

@omab

This comment has been minimized.

Show comment
Hide comment
@omab

omab Jul 9, 2012

Owner

@homakov, gotcha, got lost in the discuss for a little.

Owner

omab commented Jul 9, 2012

@homakov, gotcha, got lost in the discuss for a little.

@twoolie

This comment has been minimized.

Show comment
Hide comment
@twoolie

twoolie Jul 10, 2012

@homakov From reading the implementation notes of providers (e.g. Google) the state parameter seemed to be designated for appstate. It really should be better named.

I agree that it's possible to split the state into a hash and a redirect url, but there should be an extra param there somewhere. g.g. Oauth2 standards body.

twoolie commented Jul 10, 2012

@homakov From reading the implementation notes of providers (e.g. Google) the state parameter seemed to be designated for appstate. It really should be better named.

I agree that it's possible to split the state into a hash and a redirect url, but there should be an extra param there somewhere. g.g. Oauth2 standards body.

@omab omab closed this Jul 14, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment