According to OpenID 2.0 section 10

If the Relying Party supplied an association handle with the authentication request, the OP SHOULD attempt to look up an association based on that handle. If the association is missing or expired, the OP SHOULD send the "openid.invalidate_handle" parameter as part of the response with the value of the request's "openid.assoc_handle" parameter, and SHOULD proceed as if no association handle was specified.

This is not a security issue as the signature will fail and openid_complete will not be able to verify responses. The bug 1) is against spec and 2) blocks sign-in for certain OPs when their authentication handles expire.

CommentFileSizeAuthor
#11 openid-invalidate-handle-1-D6.patch3.4 KBc960657
#8 730462-invalid-handle-response.patch2.89 KBDamien Tournoud
#5 do-730462-invalidate_handle.patch1.76 KBHeine
#4 do-730462.patch1.76 KBHeine
#2 730462-invalidate-handle.patch1.17 KBDamien Tournoud
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

Hm. The way I'm reading that is that when the request openid.assoc_handle is expired, the OP has to "proceed as if no association handle was specified" (ie. as if that field was not in the request), and has to put the expired association handle in the response as openid.invalidate_handle.

As a consequence, the response should not contain any openid.assoc_handle, and openid_verify_assertion() will proceed with direct verification.

So, except if the OP doesn't comply with the spec, nothing wrong should happen. The only issue is that we will continue to send invalid association handles to this OP, that will have to be verified in the (less performant) direct verification mode.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Active » Needs review
FileSize
730462-invalidate-handle.patch1.17 KB

Anyway, this should do it.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

Just to clarify #1, my analysis is that 1) we do not violate the spec here, because the spec only specify the behavior of the OP, not of the RP, 2) nothing wrong should happen as long as the RP comply with the spec.

Heine’s picture

Heine CreditAttribution: Heine commented
Status: Needs review » Needs work
FileSize
do-730462.patch1.76 KB

Alas,

If we receive an invalid_handle response, we must verify directly with the OP (we do now, assuming the OP is well-behaved and sends an empty assoc_handle).

Only then, when the OP confirms that the handle is in valid should we remove it. see section 11.4.2.2:

Note: This two-step process for invalidating associations is necessary to prevent an attacker from invalidating an association at will by adding "invalidate_handle" parameters to an authentication response.

TODO - I still need to look into 11.4.2.1. about shared keys.
TODO - we must do ns checking - maybe in a separate issue?

Heine’s picture

Heine CreditAttribution: Heine commented
Status: Needs work » Needs review
FileSize
do-730462-invalidate_handle.patch1.76 KB

Gah, wrong patch.

Status: Needs review » Needs work

The last submitted patch, do-730462-invalidate_handle.patch, failed testing.

Heine’s picture

Heine CreditAttribution: Heine commented
Status: Needs work » Needs review

#5: do-730462-invalidate_handle.patch queued for re-testing.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Reviewed & tested by the community
FileSize
730462-invalid-handle-response.patch2.89 KB

Rerolled with a some additional comments and a smallish style issue.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Needs review
FileSize
openid-invalidate-handle-1-D6.patch3.4 KB

D6 backport.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

still an open core bug

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.