Problem/Motivation
From the OpenID 2.0 specification:
URL Identifiers MUST then be further normalized by both following redirects when retrieving their content and finally applying the rules in Section 6 of RFC3986 to the final destination URL. This final URL MUST be noted by the Relying Party as the Claimed Identifier and be used when requesting authentication.
Redirects are not currently followed.
Proposed resolution
Alter the return value of hook_openid_discovery_method_info()
to include the new claimed identifier after following redirects, and use this value when available. See #12 for more information. Patch in #30 implements this change.
Remaining tasks
The current patch includes updated tests and hook documentation.
Evaluate the possibility of backporting this fix to D7 and D6. There is an API change in the expected return structure of a hook, so to make it backwards-compatible, D7 and D6 would need to add code checking for this structure and assign the list of services based on whether the new structure is used. If the return value from the hook has the keys 'services'
, it uses the new format, but if it returns a flat array of services, it uses the old format.
User interface changes
None.
API changes
The expected return value of hook_openid_discovery_method_info()
changes from a flat array of services to an associative array with the following structure.:
'services'
(required) an array of discovered services (including OpenID version, endpoint URI, etc).'claimed_id'
(optional) new claimed identifer, found by following HTTP redirects during the services discovery.
Original report by Heine
From the OpenID 2.0 specification:
URL Identifiers MUST then be further normalized by both following redirects when retrieving their content and finally applying the rules in Section 6 of RFC3986 to the final destination URL. This final URL MUST be noted by the Relying Party as the Claimed Identifier and be used when requesting authentication.
Emphasis added.
I see no evidence for following redirects.
Comment | File | Size | Author |
---|---|---|---|
#40 | 575810-40_openid_redirect_minimal_fix.patch | 1.07 KB | wojtha |
#36 | 575810-36_openid_redirect_dr7.patch | 16.42 KB | wojtha |
#32 | 575810-32-openid_redirect_better_api.patch | 14.27 KB | wojtha |
#30 | 575810-30-openid_redirect_better_api.patch | 14.27 KB | wojtha |
#28 | 575810-28-openid_redirect_better_api.patch | 29.22 KB | wojtha |
Comments
Comment #1
c960657 CreditAttribution: c960657 commentedComment #2
wojtha CreditAttribution: wojtha commentedI contacted Security Team about that week ago since this issue is little bit dangerous AFAIK and allows impersonation when Openid identifier - claimed id - is recycled.
Damien Tournaud from Drupal Security Team surprisingly directs me here today, to this public issue which is laying here 1.5 year unattended.
What I mean with impersonation? Here is example situation:
Currently I know about at least one provider, which is affected by this.
I made quick simple patch for D6 which fixes that, but in D7 is thing little bit more complicated, it seems that OpenID Discovery part should be reworked more.
This is initial fix which works for D6 now:
I'm pushing this issue to MAJOR since the bug which allows impersonation can't have "normal" priority IMHO.
If you want to compare how Yadis Discovery is working in JanRain PHP Openid Library
See https://github.com/openid/php-openid/blob/master/Auth/Yadis/Yadis.php#L343
normalized_uri is later used as claimed_id in the request
Comment #3
vzima CreditAttribution: vzima commentedAs I discovered this bug with wojtha and since I work on provider side of OpenID I found much worse way how this can be used to attack users.
Let us have a user Bob with secure claimed identity https://bob.example.com/ and with redirect (302) from http://bob.example.com/ to his identity. This is perfectly good strategy how to protect your identity against attack with https and keeping possibility to only insert 'bob.example.com' in logging dialogs.
Correct OpenID should look like following:
1) Bob tries to login to Drupal via OpenID inserting 'bob.example.com' into OpenID login form
2) Drupal normalizes identity to 'http://bob.example.com'
3) Drupal makes discovery on 'http://bob.example.com/'
4) Drupal gets redirect to 'https://bob.example.com/'
5) Drupal makes discovery on 'https://bob.example.com/'
6) Drupal gets XRDS/HTML with information of Bob's provider and identity and uses 'https://bob.example.com/' as Bob's claimed_id to make OpenID request
7) Now continues with regular authentication on Bob's provider that ends with returning of response with Bob's claimed_id 'https://bob.example.com/'.
Let us have user Evil that tries to get to Bobs account.
Evil has regular account at provider provider.com with local_id 'http://evil.provider.com/'. Evil does not have to control provider.com.
But here is problem with Drupal behaviour which links user account to identity user inserts not following proper YADIS discovery.
1) Evil tries to login to Drupal via OpenID inserting 'bob.example.com' into OpenID login form
2) Drupal normalizes identity to 'http://bob.example.com'
3) Drupal makes discovery on 'http://bob.example.com/'
4) Evil now replies to consumer with 200 HTTP page that is XRDS/HTML that contains Evil's local_id 'http://evil.provider.com/' and his provider 'http://provider.com/'
5) Drupal now sends regular OpenID request to provider.com with claimed_id 'http://bob.example.com' and local_id 'http://evil.provider.com/'. Evil authenticates to his account and is returned back to Drupal with identity 'http://bob.example.com/' and is logged is as Bob. :(
At correct consumer this attack would end in creation of new account for Evil with claimed_id 'http://bob.example.com/' and not in logging him as Bob.
Evil just needs to send his XRDS/HTML to consumer either by man id the middle attack or DNS poissoning, maybe some other way to get to Bob's account.
Comment #4
wojtha CreditAttribution: wojtha commentedAfter this patch openid implementation will follow redirects and change of claimed id will be reflected in the scope of openid_discovery() and openid_begin().
Unfortonutelly little API change is needed: hook_openid_discovery_method_info() callbacks should pass $claimed_id by reference and not by value. Alternative approaches which could be considered: changing claimed_id in $_SESSION or in $GLOBAL context.
Comment #5
wojtha CreditAttribution: wojtha commentedRedirects and Claimed id schema change is also mentioned in 11.5.2. HTTP and HTTPS URL Identifiers:
Comment #6
wojtha CreditAttribution: wojtha commentedTest passed, but drupal.org doesn't respond this time because of network trouble, so the test status wasn't updated.
Comment #7
wojtha CreditAttribution: wojtha commentedPatch cleanup + couple of fixes:
- Removed $result->redirect_code code checking, because when it comes from drupal_http_redirect it could be only one of the (301, 302 and 307).
- Added second URL normalization as spec requires.
There could be 2 redirects, but no other check is needed, because drupal_http_request() is super powerful and it will do it for us (check option 'max_redirects' at drupal_http_request()).
Comment #8
Heine CreditAttribution: Heine commenteddrupal_http_request is also broken :(
#1081068: drupal_http_request inconsistent redirect_url
Comment #9
Heine CreditAttribution: Heine commentedI don't see that in 7.2.4, but I can see why you think so:
The both indicates that you need to do 1) follow redirects and 2) applying the rules in Section 6 of RFC3986.
Not both redirects :)
That also means we still need to check the response code of drupal_http_request as it simply ends following redirects when max_redirect is reached instead of throwing an error. If this happens, the Identifier cannot be used.
Comment #10
wojtha CreditAttribution: wojtha commentedYeah true, I understood the "both" in a bad way. So here is new patch...
But checking if $result->code contains redirect code seems to me like a workaround, what about returning error from drupal_http_request when max redirects was reached?
Comment #11
c960657 CreditAttribution: c960657 commentedPlease create a ticket rather than filing bugs/feature requests as comments.
Isn't it better to check whether $result->code is 200?
We usually don't start an if block with a newline.
You usually don't need to specify references to the original ticket in your patch. People interested in the original ticket can look it up using the Git log.
Wouldn't it be more intuitive to look at $result->redirect_url instead of $result->redirect_code?
I wonder whether it would make sense in D8 to eliminate openid_normalize() in favor of the new &$claimed_id by-reference argument? It seems that normalization and discovery are more tightly coupled than currently reflected by the code (e.g. following redirects is also part of the normalization process). We do call openid_normalize() a few places outside the discovery process, but I'm not sure that is required by the spec (I just quickly browsed it).
Comment #12
wojtha CreditAttribution: wojtha commented@c960657 Ok, I mostly agree, here is new patch.
As to &$claimed_id by-reference argument: I have discussed it with Heine couple days ago on IRC. We thinks that it might be better to redesign API little bit. So discovery will not return only array of services but array with services and discovered claimed id and might be some other parameters.
From the record:
As to the normalization: it should be used before and during discovery and than during final assertion verification (if claimed id doesn't match, there is one more check; but current code is wrong too in this part - see #1076366: OpenID discovery spec violation - fragments are removed from claimed id ).
I also opened new issue: #1096890: drupal_http_request should return error if reaches max allowed redirects
Comment #13
c960657 CreditAttribution: c960657 commentedNow that D8 is open for development I guess the proper approach is to create a patch for D8 first (including the suggested API change) and then discuss D7 and D6 afterward.
Comment #14
wojtha CreditAttribution: wojtha commentedPatch against D8 including the API changes suggested in #12, this patch will probably fails since I didn't change any test. So now the functions of discovery methods returned by the hook_openid_discovery_method_info() have to return following array:
Comment #15
wojtha CreditAttribution: wojtha commentedComment #17
wojtha CreditAttribution: wojtha commentedComment #18
Dries CreditAttribution: Dries commentedMissing dot/point at end of sentence.
Elsewhere we write CanonicalID as two words, I believe.
Would be good if we could resolve that TODO.
This should be marked with a @TODO so we can 'grep' for it.
Comment #19
wojtha CreditAttribution: wojtha commentedComment #20
wojtha CreditAttribution: wojtha commentedAdding tests and bumping to critical. Since this has at least same consequences as #1076366: OpenID discovery spec violation - fragments are removed from claimed id .
Issue #1120290: Provide transition for accounts with incompletely stored OpenIDs is now solving the transition of the stored Claimed Identifiers, which are invalid or incomplete mainly because of this and "stripped fragments" issue.
Comment #21
wojtha CreditAttribution: wojtha commentedBetter comments and API.
Comment #22
catch#1081068: drupal_http_request inconsistent redirect_url says it blocks this, but it is not marked critical, nor is it not rolled into this patch - please clarify either way.
Comment #23
wojtha CreditAttribution: wojtha commented@catch: Heine thought that the #1081068: drupal_http_request inconsistent redirect_url issue affects the following part of code (and it should theoretically), but I didn't notice in real tests of openid authentication.
IMHO it affects this issue only in certain conditions - it depends if number of redirects was 1 or 2 and more, thats the inconsistency.
I will focus on that during my next testing session.
Comment #25
xjmTagging issues not yet using summary template.
Comment #26
xjmThe patch has some documentation cleanup mixed in, which makes it hard to review. (e.g., reformatting existing docblocks and comments).
Can we remove the documentation cleanup from the patch and open it as a separate issue?
Comment #27
xjmThis change is just a formatting cleanup, so would be better to put it in a separate patch.
Also appears to be just a commment cleanup; also for that separate issue.
Again, separate issue.
More pure cleanup for a separate issue.
Needs a period. Also "if the Canonical ID..."
Needs a period. I'd suggest: "Replace the user-entered claimed_id if we received a redirect."
Needs a period.
Needs a period and should be two words, since it is a verb: "Clean up."
Edit: Silly dreditor. :)
Comment #28
wojtha CreditAttribution: wojtha commentedStripped down comments fixes as suggested @xjm in #27.
Comment #30
wojtha CreditAttribution: wojtha commentedTest failed at the patch format check... CRLF converted to LF.
Comment #31
xjmAdded summary.
Comment #32
wojtha CreditAttribution: wojtha commentedSmall fix. Might be a test is needed for this since it passed the tests here (but not passed the tests in #1120290: Provide transition for accounts with incompletely stored OpenIDs).
Comment #33
catchLooks good, although I do not and have no intention of reviewing it against the OpenID spec, however I'll trust wojtha and the other contributors to this issue who have.
Since we have test coverage being added at #1120290: Provide transition for accounts with incompletely stored OpenIDs there seems no reason to add an explicit test here too.
Comment #34
aspilicious CreditAttribution: aspilicious commentedI managed to log in with an openid. Sounds rtbc for me than :)
Comment #35
webchickThis doesn't seem to apply to 7.x. I can haz a D7 patch?
Comment #36
wojtha CreditAttribution: wojtha commentedBackport of the #32 patch for D7, temporarily changing the issue version to test the patch.
Comment #37
aspilicious CreditAttribution: aspilicious commentedBack to 8.x for webchick! :D
Comment #38
aspilicious CreditAttribution: aspilicious commentedR T B C
Comment #39
webchickGreat, thanks SO much!! Committed and pushed to 8.x and 7.x.
Marking down to 6.x and "to be ported".
Comment #40
wojtha CreditAttribution: wojtha commentedOh yeah, finally :-)
For the Drupal 6, I'm suggesting the minimal fix - just passing the claimed_id by reference instead referencing by value. I is opposite of the D7 and D8 approach but Drupal 6 OpenID API is much simpler and more straightforward, it has the only hook - hook_openid. Using this approach we will stay compatible with the contrib modules.
Comment #41
wojtha CreditAttribution: wojtha commentedI've added change notice for Drupal 7: Change in Openid discovery methods registered by hook_openid_discovery_method_info().
Comment #41.0
wojtha CreditAttribution: wojtha commentedUpdated issue summary.