OAuth attachment endpoint vulnerable to CSRF attack

[awh]

https://groups.google.com/a/weave.works/d/msg/security/3yJ5xswqbkI/qlANg_aqBwAJ

Attack Preparation

• Start with a browser logged in to two google accounts attacker1@gmail.com & attacker2@gmail.com
• Create an instance in weave cloud using attacker1@gmail.com, log in via email
• Attach Google as an external login; you will be redirected to accounts.google.com to select an account. Select attacker2@gmail.com
• Before pressing 'Allow', configure browser proxy settings to block access to cloud.weave.works
• When you press 'Allow', the browser will be redirected to a cloud.weave.works/login-via/google?state link, but the request will fail due to the proxy settings. It's important that the link isn't followed at this stage, if it is the attack doesn't work. Copy the link.

Attack

• A victim who is logged into their own instance is tricked into visiting the copied link
• They are taken to their instance, but attacker2@gmail.com is now attached to it. Attacker can now log in to victim's instance using Google authentication of attacker2@gmail.com

[awh]

Reproduced successfully on cloud.weave.works this morning; csrf_token values are different in the two browser profiles I used to simulate the attacker & victim.

[2opremio]

Relevant code: https://github.com/weaveworks/service/blob/5027572d9d06aafd97cfebb9bbbcd2bcae3acc52/users/login/oauth.go#L102

[2opremio]

The verification used to work, See https://groups.google.com/a/weave.works/d/msg/security/R0R9O15NEoE/XkVFg9fxBQAJ

Last time I checked, I got the following error:

[rade]

AFAICT, the request is a GET and we have exempted GETs from CSRF checks (for, iirc, good reasons).

[2opremio]

Even if it's a GET request we check it explicitly. Note that we are invoking the verifier manually on the status parameter.

What you are suggesting would be true for any other GET requests.

On Apr 28, 2017 3:19 PM, "Matthias Radestock" notifications@github.com wrote:

AFAICT, the request is a GET and we have exempted GETs from CSRF checks (for, iirc, good reasons).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/weaveworks/service/issues/1187#issuecomment-297995636, or mute the thread https://github.com/notifications/unsubscribe-auth/ACQOJLzEAPCxRvPFzsj_NrlR1U_jWW68ks5r0ed6gaJpZM4NLSXM .

[aaron7]

When linking my weave cloud, I get the state eyJ0b2tlbiI6IiJ9%3AeyJ0b2tlbiI6IiJ9zn4cLCTcVj-ianBlnRUlAJ8U9_kr_XOOEh4ZtDj8F3Q. This is exactly the same state as the one from the video, which shows that we could be always creating the state hmac.New(sha256.New, []byte(a.Config.ClientSecret)).Sum("") - the token always being "" because of the GET request exception.

So it looks like the problem is that the state is the same for everyone.

[rade]

Well spotted, @aaron7!

So should we simply be adding a nonce here?

[2opremio]

That could be it, although Tokens are initialized in GET requests (butt not verified), see how the safety of methods is checked after generating the token.

Either way, it's clear something is wrong. I will take it up

[2opremio]

I get the state eyJ0b2tlbiI6IiJ9%3AeyJ0b2tlbiI6IiJ9zn4cLCTcVj-ianBlnRUlAJ8U9_kr_XOOEh4ZtDj8F3Q. This is exactly the same state as the one from the video

I can also reproduce (locally)

[2opremio]

Actually, not true, the prefix is the same though eyJ0b2tlbiI6IiJ9%3AeyJ0b2tlbiI6IiJ9iHjSlN5vWsWe6ZIGAulGW12vzOlnjGc0gPSNQrBPKn4

[aaron7]

Actually, not true, the prefix is the same though eyJ0b2tlbiI6IiJ9%3AeyJ0b2tlbiI6IiJ9iHjSlN5vWsWe6ZIGAulGW12vzOlnjGc0gPSNQrBPKn4

is that from cloud.weave.works?

[2opremio]

it's local

[aaron7]

local probably has a different ClientSecret - could you try and reproduce on cloud.weave.works?

[2opremio]

You are right, I get the exact same state. I will look into this