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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c960657’s picture

Version: 6.x-dev » 7.x-dev
wojtha’s picture

Assigned: Unassigned » wojtha
Priority: Normal » Major
Status: Active » Needs work

I 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:

1. User Bob A. have its account  http://bob.someid.com/ (claimed_id)
2. User Bob A. login to http://drupal-e-shop.com using this id, Drupal doesn't follow redirect to "https://bob.someid.com/#fjGhT" which should be used as user's claimed_id in request and use and later saves "http://bob.mojeid.cz/" to authmap instead
3. User Bob A. start using another OpenID service and cancel his account http://bob.someid.com/
4. http://bob.someid.com/ is being deleted by the provider and its free now
5. User Bob X. happily creates new account http://bob.someid.com/  (claimed_id) with identity: "https://bob.someid.com/#XYZIT"
6. User Bob X. login to http://drupal-shop.com using id  "http://bob.someid.com/", Drupal looks to authmap and will see: "http://bob.mojeid.cz/"  its already here!
7. RESULT: User Bob X. is now logged in as a user Bob. A. in the site and see all Bob informations...

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:

// This is not nice - I need to change variable to reference (&) since I need to change claimed_id
// in openid_begin() function which calls openid_discovery() and later generates request to openid provider.
function openid_discovery(&$claimed_id) {
  $services = array();
  $xrds_url = $claimed_id;

  // ... deleted some code

  $url = @parse_url($xrds_url);
  if ($url['scheme'] == 'http' || $url['scheme'] == 'https') {
    // For regular URLs, try Yadis resolution first, then HTML-based discovery
    $headers = array('Accept' => 'application/xrds+xml');
    $result = drupal_http_request($xrds_url, $headers);

    if (!isset($result->error)) {

      // Replace user entered claimed_id if we get 302 "Found" redirect
      if ($result->code == 302 && !empty($result->redirect_url)) {
        $claimed_id = $result->redirect_url;
      }

     // ... discovery process continues

  }
}

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

$result->normalized_uri = $response->final_url;

normalized_uri is later used as claimed_id in the request

vzima’s picture

As 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.

wojtha’s picture

Assigned: wojtha » Unassigned
Priority: Major » Normal
Status: Needs work » Needs review
FileSize
3.02 KB

After 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.

wojtha’s picture

Redirects and Claimed id schema change is also mentioned in 11.5.2. HTTP and HTTPS URL Identifiers:

... If the same end user controls the same URL, differing only by scheme, and it is desired that the Identifier be the HTTPS URL, it is RECOMMENDED that a redirect be issued from the HTTP URL to the HTTPS URL. ... If an attacker could gain control of the HTTP URL, it would have no effect on the HTTPS URL, since the HTTP URL is not ever used as an Identifier except to initiate the discovery process.

wojtha’s picture

Test passed, but drupal.org doesn't respond this time because of network trouble, so the test status wasn't updated.

wojtha’s picture

Patch 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.

if (!empty($result->redirect_code)) {
  $claimed_id = openid_normalize($result->redirect_url);
}

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()).

Heine’s picture

drupal_http_request is also broken :(

#1081068: drupal_http_request inconsistent redirect_url

Heine’s picture

I don't see that in 7.2.4, but I can see why you think so:

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

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.

wojtha’s picture

Yeah 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?

c960657’s picture

+    // TODO: drupal_http_request should return error if reaches max allowed
+    // redirects

Please create a ticket rather than filing bugs/feature requests as comments.

+    if (!isset($result->error) || !in_array($result->code, array(301, 302, 307))) {
+

Isn't it better to check whether $result->code is 200?
We usually don't start an if block with a newline.

+      // Replace user entered claimed_id if we got redirect
+      // Fixes http://drupal.org/node/575810 OpenID discovery spec violation.

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.

+      if (!empty($result->redirect_code)) {
+        $claimed_id = openid_normalize($result->redirect_url);
+      }

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).

wojtha’s picture

@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:

Heine: wojtha, drupal_http_request is also a prerequisite for the other patches. Wondering if we should simply change the return structure of the discovery.
wojtha: the return structure of the discovery - it is possible, but it will have serious affect on the API
wojtha: but it will be the most clean solution IMO
Heine: true, it might not be acceptable for D7 and D6
wojtha: now the discovery returns only services, so we need to isolate them to the e.g. 'services' array key and than we could add other parameters like dicovered claimed_id and so
Heine: though passing claimed_id by ref is also a bit scary when it is later used in verification, as you need to copy the claimed_id to prevent it from changing
Heine: right and properly order and/or include the special URLs for claimed id
wojtha: yes I know, but it was smallest possible change to the API, but not very defensive
Heine: Also still need to look into certificate verification with fsockopen (D6)
wojtha: I think we should try to modify the return structure of the discovery, defensive approach and better flexibility could succeed insted of passing claimed_id by ref... even if it API change, because passing claimed_id by ref through two levels is API change too and more scary as you've mentioned :-)

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

c960657’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Now 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.

wojtha’s picture

Patch 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:

  return array(
     'services' => $services, // (array)
     'claimed_id' => $claimed_id, // (string)
  );
wojtha’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 575810-15-openid_redirect_better_api.patch, failed testing.

wojtha’s picture

Assigned: Unassigned » wojtha
Status: Needs work » Needs review
Issue tags: +Needs backport to D6, +Needs backport to D7
FileSize
6.64 KB
Dries’s picture

Status: Needs review » Needs work
+++ b/modules/openid/openid.module
@@ -256,16 +256,24 @@ function openid_login_validate($form, &$form_state) {
+  // Set claimed_id from discovery

Missing dot/point at end of sentence.

+++ b/modules/openid/openid.module
@@ -423,18 +431,26 @@ function _openid_xri_discovery($claimed_id) {
+          // Ignore service if CanonicalID could not be verified.

Elsewhere we write CanonicalID as two words, I believe.

+++ b/modules/openid/openid.module
@@ -423,18 +431,26 @@ function _openid_xri_discovery($claimed_id) {
+        // TODO: maybe we can just return $discovery itself

Would be good if we could resolve that TODO.

+++ b/modules/openid/openid.module
@@ -454,7 +470,18 @@ function _openid_xrds_discovery($claimed_id) {
+    // Check for HTTP error and make sure, that we reach the target. If maximum
+    // allowed redirects are exhausted, final destination URL isn't reached, but
+    // drupal_http_request will not return error. (This check for 200 result
+    // code could be removed after http://drupal.org/node/1096890 will be
+    // fixed.)

This should be marked with a @TODO so we can 'grep' for it.

wojtha’s picture

Status: Needs work » Needs review
FileSize
6.8 KB
wojtha’s picture

Priority: Normal » Critical
FileSize
11.91 KB

Adding 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.

wojtha’s picture

Better comments and API.

catch’s picture

#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.

wojtha’s picture

@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.

+++ b/modules/openid/openid.module
@@ -458,7 +498,18 @@ function _openid_xrds_discovery($claimed_id) {
+      // Replace user entered claimed_id if we got redirect
+      if (!empty($result->redirect_url)) {
+        $claimed_id = openid_normalize($result->redirect_url);
+      }

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.

xjm’s picture

Tagging issues not yet using summary template.

xjm’s picture

The 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?

xjm’s picture

Status: Needs review » Needs work
+++ b/modules/openid/openid.moduleundefined
@@ -250,28 +250,39 @@ function openid_login_validate($form, &$form_state) {
- * @param $claimed_id The OpenID to authenticate
- * @param $return_to The endpoint to return to from the OpenID Provider
+ * @param $claimed_id
+ *   The OpenID to authenticate
+ * @param $return_to
+ *   The endpoint to return to from the OpenID Provider

This change is just a formatting cleanup, so would be better to put it in a separate patch.

+++ b/modules/openid/openid.moduleundefined
@@ -250,28 +250,39 @@ function openid_login_validate($form, &$form_state) {
-  // user_exteral_login later.
+  // user_external_login later.

Also appears to be just a commment cleanup; also for that separate issue.

+++ b/modules/openid/openid.moduleundefined
@@ -306,10 +317,12 @@ function openid_begin($claimed_id, $return_to = '', $form_values = array()) {
- * @param $response Array of returned values from the OpenID Provider.
+ * @param $response
+ *   Array of returned values from the OpenID Provider.
  *
- * @return $response Response values for further processing with
- *   $response['status'] set to one of 'success', 'failed' or 'cancel'.
+ * @return $response
+ *   Response values for further processing with $response['status'] set to one
+ *   of 'success', 'failed' or 'cancel'.

Again, separate issue.

+++ b/modules/openid/openid.moduleundefined
@@ -406,10 +432,10 @@ function openid_discovery($claimed_id) {
 function openid_openid_discovery_method_info() {
-  // The discovery process will stop as soon as one discovery method succeed.
+  // The discovery process will stop as soon as one discovery method succeeds.
   // We first attempt to discover XRI-based identifiers, then standard XRDS
-  // identifiers via Yadis and HTML-based discovery, conforming to the OpenID 2.0
-  // specification.
+  // identifiers via Yadis and HTML-based discovery, conforming to the
+  // OpenID 2.0 specification.

More pure cleanup for a separate issue.

+++ b/modules/openid/openid.moduleundefined
@@ -421,24 +447,33 @@ function openid_openid_discovery_method_info() {
+          // Ignore service if Canonical ID could not be verified

Needs a period. Also "if the Canonical ID..."

+++ b/modules/openid/openid.moduleundefined
@@ -458,7 +498,18 @@ function _openid_xrds_discovery($claimed_id) {
+      // Replace user entered claimed_id if we got redirect

Needs a period. I'd suggest: "Replace the user-entered claimed_id if we received a redirect."

+++ b/modules/openid/openid.testundefined
@@ -124,6 +124,28 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase {
+    // Single redirect

Needs a period.

+++ b/modules/openid/openid.testundefined
@@ -280,6 +302,41 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase {
+    // Cleanup

Needs a period and should be two words, since it is a verb: "Clean up."

Edit: Silly dreditor. :)

wojtha’s picture

Assigned: wojtha » Unassigned
Status: Needs work » Needs review
FileSize
29.22 KB

Stripped down comments fixes as suggested @xjm in #27.

Status: Needs review » Needs work

The last submitted patch, 575810-28-openid_redirect_better_api.patch, failed testing.

wojtha’s picture

Status: Needs work » Needs review
FileSize
14.27 KB

Test failed at the patch format check... CRLF converted to LF.

xjm’s picture

Added summary.

wojtha’s picture

Small 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).

  // Set claimed id from discovery.
-  if (!empty($service['claimed_id'])) {
+ if (!empty($discovery['claimed_id'])) {
    $claimed_id = $discovery['claimed_id'];
  }
catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks 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.

aspilicious’s picture

I managed to log in with an openid. Sounds rtbc for me than :)

webchick’s picture

Status: Reviewed & tested by the community » Needs work

This doesn't seem to apply to 7.x. I can haz a D7 patch?

wojtha’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
16.42 KB

Backport of the #32 patch for D7, temporarily changing the issue version to test the patch.

aspilicious’s picture

Version: 7.x-dev » 8.x-dev

Back to 8.x for webchick! :D

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

R T B C

webchick’s picture

Version: 8.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Needs backport to D7

Great, thanks SO much!! Committed and pushed to 8.x and 7.x.

Marking down to 6.x and "to be ported".

wojtha’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.07 KB

Oh 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.

wojtha’s picture

wojtha’s picture

Issue summary: View changes

Updated issue summary.

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.