In current Drupal OpenID implementation Claimed ID are normalized and Claimed ID unique fragment (if provider uses it) is stripped. This is ok for normalization, but fragment is stripped also from Claimed ID which is going to be saved to the Autmap table and this is is violation of OpenID 2.0 specs, because we should store Claimed ID including the hash/fragment.

It is true that during normalization should be stripped:

7.2. Normalization
... If the URL contains a fragment part, it MUST be stripped off together with the fragment delimiter character "#".

And is also true that the fragment must not be used for purpose of verifying discovered Information

11.2. Verifying Discovered Information

If the Claimed Identifier in the assertion is a URL and contains a fragment, the fragment part and the fragment delimiter character "#" MUST NOT be used for the purposes of verifying the discovered information.

But the fragment is very important! And identifiers MUST be stored including the fragment:

11.5. Identifying the end user

The Claimed Identifier in a successful authentication response SHOULD be used by the Relying Party as a key for local storage of information about the user. The Claimed Identifier MAY be used as a user-visible Identifier. When displaying URL Identifiers, the fragment MAY be omitted.

11.5.1. Identifier Recycling

OpenID Providers with large user bases can use fragments to recycle URL Identifiers if it is so desired. When reassigning a URL Identifier to a new end user OPs should generate a new, unique fragment part.

The full URL with the fragment part constitutes the Claimed Identifier in positive assertions, therefore Relying Parties will distinguish between the current and previous owners of the fragment-less URL.

This mechanism allows the (presumably short, memorable) recycled URL Identifiers without the fragment to be used by end users at login time and by Relying Parties for display purposes.

This issue relays also on #364348: OpenID throws an error when two accounts try to use the same OpenID because claimed id entered by user (without fragment) and claimed id returned by provider (with fragment) could be different.

Additionally this issue is slightly related to #575810: OpenID discovery spec violation - follow redirects. because without following the redirects from http to https etc. some providers fails later to send correct claimed id including the unique fragment.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wojtha’s picture

Status: Active » Needs review
FileSize
1.88 KB
Heine’s picture

Thanks. This is a serious regression from D6.

You can remove lines with the patch, no need to comment them out :)

wojtha’s picture

Issue tags: -Needs backport to D6

Cleaned up the patch, but still leave couple lines to comment fragments.

And yes you are true with Drupal 6. Drupal 6 is not affected in this case, so I'm removing the tag.

wojtha’s picture

Forgot to attach the patch 8-/

wojtha’s picture

Patch failed in this part of the test (openid.test around line 93):

    $identity = url('openid-test/yadis/xrds/dummy-user', array('absolute' => TRUE));   
    // Tell openid_test.module to respond with this identifier. The URL scheme
    // is stripped in order to test that the returned identifier is normalized in
    // openid_complete().
    variable_set('openid_test_response', array('openid.claimed_id' => preg_replace('@^https?://@', '', $identity)));

It failed, because patch "disabled" normalization during openid_complete() and test sends URL with stripped schema to test the URL normalization.

I don't think that the test and the subject of test is right. Maybe we should only check if the provided claimed_id is valid URL address during openid_complete() and if the URL is not valid let break the process as unsuccessful.

The current URL normalization function is useful only for initial normalization. But function _openid_url_normalize() is dangerous and unusable for some kind of final normalization of the final user openid identifier since it injects HTTP schema to the URL by default if schema is not present and furthermore it strips the fragment.

Preferred schema for user URL identifiers is HTTPS if possible, HTTP could be used only for initialization: 11.5.2. HTTP and HTTPS URL Identifiers and here 15.4. HTTP and HTTPS URL Identifiers.

As previously stated, the RECOMMENDED method of an End User expressing control over a URL differing only be scheme is to setup a redirect from the HTTP URL to the HTTPS URL. Relying Parties will never store the HTTP URL as during the discovery and initiation phase will follow the redirect and use the HTTPS URL as the Claimed Identifier. End users with concerns over this recommendation should directly enter their HTTPS URL at each Relying Party. This thus removes the step where the Relying Party follows the redirect to the HTTPS URL.

How to handle Claimed ID in openid_complete() is described AFAIK here: 11.2. Verifying Discovered Information and there is nothing about normalization. Only User-Supplied identifier should be normalized (the user INPUT) as described in 3. Protocol Overview item #1 and in item #3 at 7.2. Normalization, plus URLs of one or two redirects during initialization should be normalized too (item #4 at 7.2. Normalization). But nothing else.

My suggestion is to abandon that part of the test.

wojtha’s picture

Modified test

Status: Needs review » Needs work

The last submitted patch, openid_claimed_id_fragments_1076366-3.patch, failed testing.

wojtha’s picture

Status: Needs work » Needs review
FileSize
3.07 KB
meba’s picture

Priority: Major » Critical

This is critical.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

And needs to be applied to 8.x first.

wojtha’s picture

Here it is - re-roll against D8.

c960657’s picture

Version: 8.x-dev » 7.x-dev
Priority: Critical » Major
Issue tags: -Needs backport to D7

The bug was introduced in #728278: openid_complete should normalize $response['openid.claimed_id'] before discovery. I'm not sure I understand why that change was needed. It's not obvious to me from the part of the spec cited in that issue that we need to normalize before doing discovery.

The only thing that we do need to do AFAICT is to strip the fragment part from URL identifiers before comparing the Claimed Identifier and the User-supplied Identifier (I suppose this is want is meant by the part of 7.2 quoted above). openid_normalize() does that also, I know, but wouldn't it be cleaner to just strip the fragment?

When this patch is applied, users who previously connected to an account can no longer log in, if their Claimed Identifier contains a fragment. Should we provide a transition mechanism, e.g. by doing something like this in openid_authentication()?:

  $account = user_external_load($identity):
if (!$account && variable_get('openid_allow_missing_fragment', FALSE)) {
  $account = user_external_load($identity_without_fragment);
  db_query("UPDATE {authmap} SET authname = :identity WHERE module = 'openid' AND authname = :identity_without_fragment", array(...));
}

We can set openid_allow_missing_fragment to TRUE in an upgrade hook so that it only applies to existing installs. We can add this code D7 and remove it in D8.

The spec seems to indicate that two users cannot have the same identifier modulo the fragment at the same time, so I assume this is not a security hazard. But it is a violation of a SHOULD requirement in the spec. Of course we can just ignore the transition issue and just let end users request a new password and delete the bad identifier and add a new one, but this is not very user-friendly. But if being user-friendly is a security issue, then security obviously wins.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Not sure why this was moved back to 7, fixes go into 8.x first. I'll let others decide on critical or not.

c960657’s picture

Priority: Major » Critical

> Not sure why this was moved back to 7, [...]
That was not intentional. I guess I submitted a cached copy of the page. I did not mean to change the priority from "major" to "critical" either.

wojtha’s picture

@c960657 ... The only thing that we do need to do AFAICT is to strip the fragment part from URL identifiers before comparing the Claimed Identifier and the User-supplied Identifier (I suppose this is want is meant by the part of 7.2 quoted above). openid_normalize() does that also, I know, but wouldn't it be cleaner to just strip the fragment? ...

You are right that the openid specs. doesn't say it explicitly. I think we could look to other Openid libraries. I choose ZendFramework as it is "enterprise-stable". And in ZF they are normalizing the claimed_id too during verification of the server response instead of just stripping the fragment.

Snippet taken from: http://framework.zend.com/svn/framework/standard/trunk/library/Zend/Open...

class Zend_OpenId_Consumer {

// ...

/**
     * Verifies authentication response from OpenID server.
     *
     * This is the second step of OpenID authentication process.
     * The function returns true on successful authentication and false on
     * failure.
     *
     * @param array $params HTTP query data from OpenID server
     * @param string &$identity this argument is set to end-user's claimed
     *  identifier or OpenID provider local identifier.
     * @param mixed $extensions extension object or array of extensions objects
     * @return bool
     */
    public function verify($params, &$identity = "", $extensions = null)  {
                // ...              
                /* OpenID 2.0 (11.2) Verifying Discovered Information */
                if (isset($params['openid_claimed_id'])) {
                    $id = $params['openid_claimed_id'];
                    if (!Zend_OpenId::normalize($id)) {
                        $this->_setError("Normalization failed");
                        return false;
                    } else if (!$this->_discovery($id, $discovered_server, $discovered_version)) {
                        $this->_setError("Discovery failed: " . $this->getError());
                        return false;
                    } else if ((!empty($params['openid_identity']) &&
                                $params["openid_identity"] != $id) ||
                               (!empty($params['openid_op_endpoint']) &&
                                $params['openid_op_endpoint'] != $discovered_server) ||
                               $discovered_version != $version) {
                        $this->_setError("Discovery information verification failed");
                        return false;
                    }
                }
                return true;
               //...
    }
   // ...
}

Zend_OpenId ::normalize is here, $id is passed by reference:
http://framework.zend.com/svn/framework/standard/trunk/library/Zend/Open...

class Zend_OpenId {
  // ...
  /**
     * Normalizes OpenID identifier that can be URL or XRI name.
     * Returns true on success and false of failure.
     * ---  stripped ---
     * @param string &$id identifier to be normalized
     * @return bool
     */
    static public function normalize(&$id)  {
        $id = trim($id);
        if (strlen($id) === 0) {
            return true;
        }

        // 7.2.1 ...
        // 7.2.2 ...
        // 7.2.3 ...

        // 7.2.4
        return self::normalizeURL($id);
    }
  // ...
}
wojtha’s picture

@c960657 ... When this patch is applied, users who previously connected to an account can no longer log in, if their Claimed Identifier contains a fragment. Should we provide a transition mechanism, e.g. by doing something like this in openid_authentication()?:

Yes, you are right. Users will need to remap their openid accounts. But my opinion is that the security matters. As long as this issue stays unfixed there will be bigger and bigger set of users with "bad" openid identifiers in authmap. There is a risk that you allows other users to login to accounts with same normalized claimed id, but with different fragments. With the snippet you introduced, this small security hole will be still there since user accounts of users which are now creating openid accounts in D7 websites all over the world and later revoke that openid accounts could be attacked if the openid provider recycle their revoked identifiers.

I was also thinking about some automatic transition, but I think it is not possible in this case. You can't be sure that stored identifier without fragment really belongs to the user which will try to login with it anytime in the future.

I think better will be some security announcement and message during unsuccesfull openid login if we found that there is same identifier in the DB but without fragment, may be?

c960657’s picture

I think we could look to other Openid libraries. I choose ZendFramework as it is "enterprise-stable". And in ZF they are normalizing the claimed_id too during verification of the server response instead of just stripping the fragment.

In the JanRain PHP library they just strip the fragment:
https://github.com/openid/php-openid/blob/master/Auth/OpenID/Consumer.ph...

Both approaches will work, and as long as the OpenID Provider returns normalized URLs (modulo the fragment), the result is the same.

If we call openid_normalize() instead of just stripping the fragment, and the OpenID Provider returns e.g. the identifier without the http/https prefix, then we will store the unnormalized identifier in the authmap table. If we just strip the fragment, we will reject the response, because we cannot call openid_discovery() on an identifier without http/https prefix. I don't think this is a security problem, and I don't know why the OpenID Provider would do that. So I guess the difference is not important.

Over in #1076366: OpenID discovery spec violation - fragments are removed from claimed id , I have asked Heine to elaborate on why he thinks the identifier should be normalized.

I think better will be some security announcement and message during unsuccesfull openid login if we found that there is same identifier in the DB but without fragment, may be?

That might be the best solution. I know it adds some complexity to the code, but it would be a nice service to the users of Drupal 7.0. But I agree – let's get the fix releases ASAP, preferably in Drupal 7.1.

wojtha’s picture

So we just waiting for Heine what he answers. And after he'll answer we might be changing the url_normalization to fragment stripping. But except of this is the patch ready for the D8 FMPOV.

I created new issue exclusively for the D7 transition process: #1120290: Provide transition for accounts with incompletely stored OpenIDs. So there we can work on "ASAP fix" for D7 until this issue will be commited to D8.

BTW JanRain's way how strip fragment is too complicated... I will suggest some onliner as

$url = preg_replace('/#[^?]*/', '', $url);
... the fragment part and the fragment delimiter character "#" MUST NOT be used for the purposes of verifying the discovered information.
Dries’s picture

Status: Needs review » Needs work

Only had a quick first look -- I think the various code comments need work:

+++ b/modules/openid/openid.module
@@ -341,14 +341,17 @@ function openid_complete($response = array()) {
+            // Returned claimed id could have unique fragment hash to allow
+            // identifier recycling so we need to preserve it in response.
+            $response_claimed_id = openid_normalize($response['openid.claimed_id']);

The code comment might not be 100% proper English. For example, shouldn't that be 'preserve it in THE response'?

+++ b/modules/openid/openid.test
@@ -89,12 +89,12 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase {
+    // Identifier. The OpenID Provider responds with the actual identifier
+    // including fragment.

Shouldn't that be 'including THE fragment'?

+++ b/modules/openid/openid.test
@@ -89,12 +89,12 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase {
+    // Tell openid_test.module to respond with this identifier. We test if
+    // openid_complete() process it right.
+    variable_set('openid_test_response', array('openid.claimed_id' => $identity));

Should be 'processES it'?

wojtha’s picture

Status: Needs work » Needs review
FileSize
3.37 KB

Fixing comments.

c960657’s picture

+            // Returned claimed id could have unique fragment hash to allow

"The returned Claimed Identifier could have a unique fragment to allow" would be more in line with other comments (the word "hash" in an OpenID contenxt also refers to hash functions).

-    // Identifier is the URL of an XRDS document containing an OP Identifier
+    // Identifier is the URL of a XRDS document containing an OP Identifier

AFAIK the original text is fine because XRDS begins with a vowel sound (i.e. "an ex-are-dee-ess document").

wojtha’s picture

@c960657 yeah, you were right.

Fixing the comments. Again :-)

c960657’s picture

+            // Returned claimed id could have unique fragment hash to allow
+            // identifier recycling so we need to preserve it in the response.

This still suffers from the same problems as the previous wording. I suggest something like this:
“The returned Claimed Identifier may include a unique fragment to allow for identifier recycling. We need to preserve this in the response.”

Dries’s picture

Priority: Critical » Minor
Status: Needs review » Needs work

I committed the patch to 7.x and 8.x. Thanks!

I'm marking this as 'needs work' so we can still incorporate the documentation improvements suggested in #23. Also lowering the priority of the patch accordingly.

xmacinfo’s picture

Version: 8.x-dev » 7.x-dev
Priority: Minor » Critical

Commit for 7.x missing from the commit log: http://drupal.org/node/3060/commits

catch’s picture

Status: Needs work » Needs review

I'm not sure what's going on here exactly, but let's get the commit against 7.x, then back to 8.x for the comment followup. Although I should note this was never actually RTBC when it was committed, so I'm not going to put it at that status for D7.

xmacinfo’s picture

Maybe Dries meant to commit only for 8.x. :-)

catch’s picture

Actually looking at #1120290: Provide transition for accounts with incompletely stored OpenIDs, this should not go into Drupal 7 until there's also an RTBC patch there, since it will prevent people from logging in at all, unless that issue gets marked won't fix for some reason. So this is very much needs review (or even postponed) against D7.

wojtha’s picture

It was commited to the D7 in http://drupalcode.org/project/drupal.git/commit/fcf48f0 (Thu, 12 May 2011 01:34:06). So rollback or resolving the #1120290: Provide transition for accounts with incompletely stored OpenIDs is needed before release of 7.1.

webchick’s picture

Status: Needs review » Postponed
Issue tags: +Needs tests, +Needs backport to D7

Ok, rolled #22 back in 7.x, for now. Also marking this postponed on #1120290: Provide transition for accounts with incompletely stored OpenIDs.

I would absolutely love a walkthrough on IRC with someone about why this is a "release blocker," and why these two issues are separate and not coupled together. I will also try reading these issues again when I'm not completely exhausted and see if that helps. :P

Also, if this breaks logins, and the tests don't indicate it, we need tests.

webchick’s picture

Version: 7.x-dev » 8.x-dev
Priority: Critical » Normal
Status: Postponed » Needs work

Actually, back to 8.x for the documentation improvements (my but this is confusing :P).

catch’s picture

Priority: Normal » Critical

Still a critical D7 issue even if the D8 change may end up being minor.

wojtha’s picture

Issue tags: -Release blocker

@webchick Transition issue is separate 'cause:

1) This is not the only issue which results in non working openid or storing invalid openid identifiers in some cases. the second one which I know is active even in Drupal 6: #575810: OpenID discovery spec violation - follow redirects.
2) I wanted to make some progress at D7 during the "4 months vacuum time" when almost nothing were commited to core, I needed some place where I could develop and test against D7, because this issue was against D8... but than I went bit overloaded with client work so this reason is partly irrelevant these days, but still bit needed have it separate I think, since D8 will not need (hopefully) the transition mechanism.

I added "release blocker" tag just because Dries commited this to D7, but transition issue has not been resolved in D7 yet. So after rollback I'm removing this tag.

wojtha’s picture

The work on the transition feature for Drupal 7.x has been finally started... #1120290-9: Provide transition for accounts with incompletely stored OpenIDs

wojtha’s picture

Status: Needs work » Needs review
Looked to the recent patch and what Dries actually commited and the sentence which c960657 want to change in #23 isn't here. He probably reviewed the older patch by accident. The comment in this part contains:

+++ b/modules/openid/openid.module
@@ -341,14 +341,18 @@ function openid_complete($response = array()) {
+            // Returned Claimed Identifier could contain unique fragment
+            // identifier to allow identifier recycling so we need to preserve
+            // it in the response.

c960657's documentation suggestion differs in details:
"The returned Claimed Identifier may include a unique fragment to allow for identifier recycling. We need to preserve this in the response."

Is it worth for patch?

wojtha’s picture

@webchick #30, as to the Tests: The login will fail because the current behavior of openid module is "bad" and saves "not the right things" to the authmap. E.g. it saves user entered Claimed ID instead of the one provided by the Provider. So after the fix, user with "bad" openids will not be able login since openid module will place the right information to the "claimed id", however it will be different than the previous "bad" one. So it is because we have bad data in the authmap. So the test for this is just that we have different authname in the authmap than it should be, so the openid login will logically fail... that I mean with the sentence that it "breaks the login".

The commited patch includes a small test improvement, so it is tested if the fragment is stripped or not after openid_complete() and I hava also added tests which are dealing with the "bad" Openid transition to the #1120290-13: Provide transition for accounts with incompletely stored OpenIDs (experimental) patch. (I'm going to make regular patch from it). I'm suggesting to create better tests for this Openid after the whole bung of openid fixes will be commited. Since they are connected together and it is hard to create some tests for the second step, if the first step is producing errors or unexpected outputs, which are fixed, but in the different issue which has not been commited yet... Thats also why I created the experimental "all-in-one" patch, which includes all needed fixes, because previous attempts which solved just one tile from the "puzzle" failed - and the "all-in-one" succeed.

wojtha’s picture

Based on my comment #36 should we remove the "Needs tests" tag and change the status to RTBC? since the patch was already commited to D8 (and actually to D7 too but rolled back later because of the missing transition path).

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

wojtha's comment makes sense - the comment was in a different patch to the one eventually committed, and that was the only reason to move this back to D8.

So....

Back to D7 again, although this still depends on the transition issue (otherwise user logins are broken), so while I'm marking it RTBC again, it is really "postponed and commit very close to the other issue".

Also removing needs tests tag since that's covered it seems.

webchick’s picture

Status: Reviewed & tested by the community » Postponed

I'm going to mark this postponed so it's not accidentally committed again until the other issue is in. Fear not; this will show up in my http://drupal.org/project/issues/search/drupal?status[]=Open&priorities[... view so I won't forget it.

wojtha’s picture

Status: Postponed » Reviewed & tested by the community
Issue tags: -Needs backport to D7 +Needs committer feedback

Already commited to Drupal 7 together with the issue #575810: OpenID discovery spec violation - follow redirects. in the commit http://drupalcode.org/project/drupal.git/commit/9d35f25

Needs webchick's or Drieses feedback.

catch’s picture

Issue tags: +Release blocker

Adding release blocker tag so this gets sorted out before the next point release.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I believe we can mark this back to fixed now that #1120290: Provide transition for accounts with incompletely stored OpenIDs was committed. If that's incorrect, please let me know.

wojtha’s picture

Yeah, I can confirm it is finally committed. Yupeee! :-)

Status: Fixed » Closed (fixed)
Issue tags: -Release blocker, -Needs committer feedback

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

xjm’s picture

Issue tags: -Needs committer feedback