search

yohcop / openid-go

OpenID consumer implementation in Go - golang
Other
83 stars 40 forks source link

Properly check if all the required fields have been signed #26

Closed xStrom closed 8 years ago

xStrom commented 8 years ago

The OpenID 2.0 spec point 10.1 states that "_[openid.signed] MUST contain at least "op_endpoint", "return_to" "response_nonce" and "assoc_handle", and if present in the response, "claimedid" and "identity"."

Currently the code doesn't verify any of this. This patch implements this verification.

This is a high level security vulnerability.

One attack scenario being where the attacker acquires a legitimate positive assertion without the claimed_id field. Then intercepting the browser redirect to the callback and adding their own malicious claimed_id field. All verifications pass and the malicious claimed_id is treated as verified.

I wrote a crude proof of concept exploit for this and tested it on the Steam OpenID Authentication server. Steam happily confirmed my check_authentication request because all the signed fields were legitimate and the nonce was new & unused. Steam had access to my malicious parameters (because they are sent as part of the spec requirement), but because the spec doesn't seem to require the OP to look for such attacks, ignored them.

yohcop commented 8 years ago

Thanks for finding this! Just added a couple of comments, nothing huge.

xStrom commented 8 years ago

I improved the tests.

As for moving the verifySignedFields call to a later part, I don't think that's a good idea. 1) There's actually a bug currently because the verifySignature call happens so late. I'll describe that bug soon in another pull request. We can deal with the misleading comment there as well. 2) The verifySignedFields function is really fast compared to the other verification functions, e.g. verifyDiscovered can start doing several HTTP requests. I think it's better to fail fast and not waste resources unnecessarily. 3) There is no security problem with doing the signature field check first. In fact, doing the full signature check as the very first thing is a hallmark of a well designed secure system.

yohcop commented 8 years ago

Thanks for the tests.

And yeah, you are right. I didn't re-read the implementations and what could make an http request. I was mostly concerned about keepking the code close to "reading the spec", but this makes sense. In fact we could move verifyNonce and verifySignature abouve verifyDiscovery as well.

But this looks good.