This "D7 only" issue is tight related to #1076366: OpenID discovery spec violation - fragments are removed from claimed id , that one needs to be fixed in D8 first. But because this needs to be fixed ASAP in D7, in meantime we can work on a part of the fix which affects D7 only.

For D7 we need to provide some transition path for users with Openid accounts which has fragment part but Drupal 7.0 stripped it, so after #1076366: OpenID discovery spec violation - fragments are removed from claimed id will be commited these users will fails to login using Openid identifiers which were affected (not all providers are using the fragment part e.g. Yahoo does but Flickr doesn't).

Me and c960657 agreed in #1076366-17: OpenID discovery spec violation - fragments are removed from claimed id that the probably best way how to provide this transition will be this way:

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?

Marking as critical since the related issue is also marked as critical.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wojtha’s picture

Title: Provide transition for accounts with incompletely stored OpenIDs (fragment cutoff) » Provide transition for accounts with incompletely stored OpenIDs
Issue tags: +Needs backport to D6

This transition needs to be backported also to D6 not because of fragment cutoff but because of this issue: #575810: OpenID discovery spec violation - follow redirects..

I know about at least one provider which doesn't return proper claimed id including fragment in the response if claimed id received through following redirects isn't used in the request. So Drupal stores the user entered claimed id (w/ HTTP schema and w/o fragment) and consequences of this behaviour is practically same as of the fragments cuttof issue.

RobLoach’s picture

Is this still considered a critical issue? Critical issues are meant as release blockers. We want Drupal 7.1, but can't have it put out with critical issues in the queue. Maybe switch it to major, or postponed until #1076366: OpenID discovery spec violation - fragments are removed from claimed id goes in?

catch’s picture

We need to clarify what critical means for point releases of stable branches (which I've just tried to do here: http://drupal.org/node/45111).

When we introduced the major priority, I was thinking we'd clear the critical queue each time before a point release same as Rob, however pwolanin pointed out this doesn't make sense in practice. Since we have already released 7.0 (with some very nasty bugs in the upgrade path etc.), then if we're able to fix a certain number of critical issues in 7.1 (even if not everything marked in the queue), then it makes sense to put out 7.1.

What I would like to see is Dries' promise of 'no more than 15 criticals in Drupal 8 at any one time' extended to 'no more than 15 criticals in Drupal 7 or Drupal 8 at any one time'. This is on the basis that anything that's critical in D7 is critical in D8, and issues like this are exceptions - but eventually we'll have sufficient divergence in the code base that more issues may get forked like this, or won't be an immediate backport from 8 to 7. If we are serious about not generating a huge backlog of hundreds of gnarly issues and an 18 month code freeze like we had for Drupal 7, this is about the only chance of keeping us honest.

So in that case, while a critical issue against Drupal 7 might not block a Drupal 7 point release, if it is something that is genuinely critical, then it will contribute to the total pool of "things that are horribly broken and need to be fixed", whereas major issues are more "this is very important". Having said that, I don't know what this particular issue falls into, you can run core fine without OpenID module, if it's not actually a security issue I would downgrade to major.

wojtha’s picture

Dries wanted to commit the related issue to D7 #1076366-24: OpenID discovery spec violation - fragments are removed from claimed id , but accidentaly he didn't, but after he does so, this will be release blocker IMO, since users with wrong stored IDs will not be able to login than.

This is related also to #575810: OpenID discovery spec violation - follow redirects. which also cause either authentication refuse or storing of incomplete openid identifiers and affects Drupal all current Drupal versions (6-8).

... and yes, the related issues has the potential to be the security issues, I originally contacted the Security Team about them several times, but at the end they send me to the public issue queue with reply that "this is well known public issue". So me and one member of staff of mojeID.cz provider described in #575810: OpenID discovery spec violation - follow redirects. how this issues could be used for stealing user accounts or gaining access to the site in comments #2 and #3 which are now unpublished.

catch’s picture

Alright that makes complete sense for this to be critical, assuming the related issue is committed before the next point release, this issue should also block that, otherwise there's no transition. I updated things over there, thanks for the summary!

wojtha’s picture

Issue tags: +Release blocker
wojtha’s picture

Issue tags: -Release blocker

Removing Release blocker tag since webchick rolled back the #1076366-30: OpenID discovery spec violation - fragments are removed from claimed id from D7 core.

wojtha’s picture

Status: Active » Needs review
FileSize
7.66 KB

First attempt: Openid identifier is updated automatically during the login, when the email in the $account and in the $response are equal. Otherwise user needs to reset the password or contact the administrator.

Status: Needs review » Needs work

The last submitted patch, 1120290-9_openid_update_stripped_fragments.patch, failed testing.

wojtha’s picture

Status: Needs work » Needs review
FileSize
7.67 KB

Status: Needs review » Needs work

The last submitted patch, 1120290-11_openid_update_stripped_fragments.patch, failed testing.

wojtha’s picture

Status: Needs work » Needs review
FileSize
15 KB

Ok, now the complete fix including fixes for the related issues: #575810: OpenID discovery spec violation - follow redirects. and #1076366: OpenID discovery spec violation - fragments are removed from claimed id .

DISCLAIMER: This patch isn't intended for commit.

webchick’s picture

Ok!

If you (like me) are wondering what's up with there being 3 separate OpenID critical issues between D8 and D7 and what on earth it all means, and more importantly how to fix it, you're in luck! wojtha kindly spent about an hour with me in IRC today walking me through the state of things.

Here's the skinny:

There are two main issues where core's OpenID module is violating spec, and this leads to consequences ranging from certain OpenID providers just plain not working to possible impersonation risks:

#1: #575810: OpenID discovery spec violation - follow redirects.: When you enter an OpenID provider like "webchick.openid.com", Drupal normalizes it into a fully-qualified domain for you, like "http://webchick.openid.com", sets it as a user "Claimed Identifier" and sends a request to that URL to retrieve supported services from the provider and that kind of stuff. The problem is that all public providers (and probably all of the providers around the world) redirects "http://webchick.openid.com" to more secure https://webchick.openid.com or sometimes to completely different URL like https://openid.com/webchick. According to specs, Drupal should change that Claimed ID to that final URL and use it in the authentication request which he sends to the provider. However Drupal is completely ignoring this recommendation and instead of this it further uses the normalized user entered Claimed ID in the response (http://webchick.openid.com) no matter if that URL was redirected. We need to fix that.

#2: #1076366: OpenID discovery spec violation - fragments are removed from claimed id : Another issue is related to Claim IDs. When you attach a new OpenID provider to your account, you're given a fragment identifier, something like a form token, to tie it uniquely to your openid account. For example, https://webchick.openid.com/#FUSGDKS. If webchick deletes their openid account and another webchick creates a new one, the new account will have different fragment. Drupal is erroneously stripping this fragment identifier from the external auth tables, which could lead to impersonation attacks.

#1120290: Provide transition for accounts with incompletely stored OpenIDs (this issue) THEN you have the problem of existing D6 and D7 sites where these values have already been stored all screwed up, and they need to be fixed (D7 needs a workaround for both of these issues; D6 only the redirect one because #728278: openid_complete should normalize $response['openid.claimed_id'] before discovery did not yet get committed to D6). The only reasonable place to cut in and do this is during the login, because it requires manual intervention with a user's OpenID provider. But this involves sticking in some pretty nasty code in openid_authenticate() that needs to run on each openid login request. Furthermore API change for D7 and D6 is needed to fix #575810: OpenID discovery spec violation - follow redirects. since Claimed ID were designed here as immutable.

The OpenID standard is pretty complex, so both issues target a significant part of providers, but sure not how many of them.

So. That is where we're at. The order of operations in terms of reviewing/committing these patches is:

1. Redirects for D8
2. Fragments for D8
3. Redirects for D7
4. Fragments for D7
5. Workarounds for D7 (must be committed along #3 and #4, or else no one with invalid openids can log in)
6. Redirects for D6
7. Workaround for D6 (must be committed along with #6)

Whew!

steinmb’s picture

Thanx @webchick for the summary. I have been looking at these issues, but not able to make the connections on what goes where, when and why :)

wojtha’s picture

The transition procedure was moved to the separate function.

I also tried to tune up the "we are very sorry" message if the external login authentication fails (including the automated transition fallback). If it happens, user is redirected to the user/register form by the openid and following message is displayed:

There is already an existing account which has assigned the OpenID identifier which you have provided. However, due to the bug in the previous version of the authentication system, we can't be sure that this account belongs to you. If you are new on this site, please continue with registering the new user account. If you already have had registered account on this site assigned to the provided OpenID identifier, please try to reset the password or contact the site administrator. In that case we are very sorry for this inconvenience.

How it sounds? Any suggestions?

Status: Needs review » Needs work

The last submitted patch, 1120290-16-openid_provide_transition.patch, failed testing.

wojtha’s picture

Status: Needs work » Needs review

The two failed tests depends on #1076366: OpenID discovery spec violation - fragments are removed from claimed id - if I commited it together as I done in #13 they shell pass. Marking back as needs review.

Message that OpenID identifier must be updated manually was displayed.	Other	openid.test	538	OpenIDInvalidIdentifierTransitionTestCase->testStrippedFragmentAccountEmailMismatch()	
Message that OpenID identifier was updated automatically was displayed.	Other	openid.test	583	OpenIDInvalidIdentifierTransitionTestCase->testStrippedFragmentAccountAutoUpdateSreg()
wojtha’s picture

Code refactoring and fixing some bugs.

Status: Needs review » Needs work

The last submitted patch, 1120290-19-openid_provide_transition.patch, failed testing.

wojtha’s picture

Status: Needs work » Needs review
FileSize
14.1 KB

Same patch as in #19 but inherits the dependency #1076366: OpenID discovery spec violation - fragments are removed from claimed id to confirm that the tests are still working.

Status: Needs review » Needs work

The last submitted patch, 1120290-21-openid_provide_transition_plus_1076366.patch, failed testing.

wojtha’s picture

Ok, forgot to change the string in the test.

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/modules/openid/openid.moduleundefined
@@ -650,6 +656,80 @@ function openid_authentication($response) {
+ * This functions provides safe but not too much obtrusive procedure for users
+ * whose can't login with theirs OpenID identifiers. OpenID identifiers in the

This function provides a safe and less obtrusive procedure for users who cannot login with their OpenID identifiers.

+++ b/modules/openid/openid.moduleundefined
@@ -650,6 +656,80 @@ function openid_authentication($response) {
+  // Step 2: Try to replace https with http. Openid providers often redirects

s/Openid/OpenID for consistency

+++ b/modules/openid/openid.moduleundefined
@@ -650,6 +656,80 @@ function openid_authentication($response) {
+      drupal_set_message(t('There is already an existing account which has assigned the OpenID identifier which you have provided. However, due to the bug in the previous version of the authentication system, we can\'t be sure that this account belongs to you. If you are new on this site, please continue with registering the new user account. If you already have had registered account on this site assigned to the provided OpenID identifier, please try to <a href="@url_password">reset the password</a> or contact the site administrator. In that case we are very sorry for this inconvenience.', array('@url_password' => 'user/password')), 'warning');

There is already an existing account associated with the OpenID identifier that you have provided. However, due to the bug in the previous version of the authentication system, we can\'t be sure that this account belongs to you. If you are new on this site, please continue registering the new user account. If you already have a registered account on this site associated with the provided OpenID identifier, please try to reset the password or contact the site administrator.

+++ b/modules/openid/openid.testundefined
@@ -479,6 +479,114 @@ class OpenIDRegistrationTestCase extends OpenIDWebTestCase {
+      'description' => 'Tries to correct openid identifiers attached to accounts if theirs identifiers were stripped.',

Tries to correct OpenID identifiers attached to accounts if their identifiers were stripped.

+++ b/modules/openid/openid.testundefined
@@ -479,6 +479,114 @@ class OpenIDRegistrationTestCase extends OpenIDWebTestCase {
+    $this->assertRaw(t('There is already an existing account which has assigned the OpenID identifier which you have provided.'), t('Message that OpenID identifier must be updated manually was displayed.'));

There is already an existing account associated with the OpenID identifier that you have provided.

Powered by Dreditor.

wojtha’s picture

@tim.plunkett thanks!

c960657’s picture

   $account = user_external_load($identity);
+
+  // Tries to load user account if user_external_load fails due to possibly
+  // incompletely stored OpenID identifier in the authmap.
+  if (!isset($account->uid)) {

Why is the condition not just !$account? I assume that user_external_load() returns FALSE if there is no matching identity.

Inside _openid_invalid_openid_transition() I assume each of the steps address a separate bug. I suggest adding a reference to the d.o ticket for each. E.g. for step 1 it is http://drupal.org/node/1076366, and for step 2 it is http://drupal.org/node/575810.

+    $fallback_identity = preg_replace('/#[^?]*/', '', $fallback_identity);

Why the "?" inside the regexp pattern? Shouldn't you just strip the "#" and everything after, i.e. strtok($normalized_url, '#')?

+  // Step 3: Try to use original identifier.
+  if (!$fallback_account && isset($_SESSION['openid']['user_login_values']['openid_identifier'])) </

Which bug is this? Is it the same as step 2?

 /**
+ * Menu callback; regular HTML page with OpenID 2.0 <link> element.
+ */
+function openid_test_redirect() {

The comment seems to be copy-pasted from openid_test_html_openid2().

+      // Set the account to the found one.
+      $account =& $fallback_account;

Why =& instead of = ?

If I understand this correctly, the strategy for updating existing accounts is this:

  1. The old bugs very violations of the spec, but they were not “very” insecure, so we still trust the identity claimed using the old behaviour.
  2. Comparing the email addresses of the two users is not a security measure but just a sanity check in the event that an new user was about to take over an existing account belonging to another user – new user could actually have succeeded in this if he entered the old user's email address as his SREG information.

Is this right?

This is very complex, and I am worried that we may actually introduce new security bugs in this way. What is your gut feeling?

What do you think about providing this migration facility as an opt-in feature, i.e. site-owners must manually enable it (e.g. by specifying $conf['openid_migrate_insecure_identifiers'] in settings.php), and they can disable it when they consider it appropriate?

Shouldn't we restrict the migration feature to accounts that were created prior to the upgrade to 7.3 (or whatever is the first release containing a fix for this)? Otherwise we haven't completely eliminated the bugs even for new installs. We could check that {authmap}.aid <= variable_get('openid_max_aid_at_openid_update_7001', 0) (with the variable being set in hook_update_N()), and instead of doing an UPDATE we should rather INSERT a new row into {authmap}.

wojtha’s picture

+ // Step 3: Try to use original identifier.<br>
+ if (!$fallback_account isset($_SESSION['openid']['user_login_values']['openid_identifier']))

This one is a fallback to use user entered claimed id, its addressed as a fallback for both steps, but mainly for redirection bug. The behavior of the OpenID implementation in Drupal is unpredictable. So I tried to prepare various fallback scenarios. Some providers has no problems with the current implementation, with some of them Drupal stores the invalid values and with some providers users can't even login.

As to the security - when I was thinking about security - most of the providers (and all big providers) rely on the e-mail address and the e-mail address must be verified. If the potential attacker steals the email, he can just reset the password, using fake openid account will be unnecessary step here. The security matter is similar to rely on the e-mail address and allowing users to reset the password. For me personally is relying on the e-mail address safer than relying on the invalid openid identifier that could be recycled in the meantime or someone could use DNS spoofing as well to create his own Openid provider for a while. I'm also giving the possibilty of the password reset to the user if the automatic transition fails.

Now when I'm thinking further for me is the asking for password reset completely ok (each user has some e-mail address stored in the users table - which he entered during registration or the one which came with the OpenID in SREG). The automatic transition is something to make the process less painful for the users but less secure, but not necessary since the password reset could be used to get the access to the account again. I went into the automatic transition way mainly because webchick called for some transition path. But she supposed originally that the transition could some bulk operation placed in the hook_update_N ... this is far away from this, even the automatic transition. But we should be more secure, you're right.

So I agree that the automatic transition could be optional... but I would like to reatain the part where we inform the user about the problems with openid and asking him to reset the password or to create new account (in case he doesn't have the account yet). User should know about it - that the login fails due to invalid identifier - so he will know that he need to revoke the invalid identifier and authorize the openid account again.

If we provide hook_update_N, we can also inform the administrator, that there is this problem and ask him to enable the automatic transition (with the warning that it is less secure) ...

We could check that {authmap}.aid <= variable_get('openid_max_aid_at_openid_update_7001', 0)

Its interesting idea, is there something like this in core already, like storing the particular historical state of the system because of a bug?

Instead of doing an UPDATE we should rather INSERT a new row into {authmap}

Ok, I have no problem with this and this also makes sense together with the previous improvement.

Fabianx’s picture

Status: Needs review » Needs work

Marking this back to "needs work" as points from #26 and #27 have not yet been addressed in the patches in #25.

Best Wishes,

Fabian

c960657’s picture

is there something like this in core already, like storing the particular historical state of the system because of a bug?

Not that I am aware of.

wojtha’s picture

Why the "?" inside the regexp pattern? Shouldn't you just strip the "#" and everything after, i.e. strtok($normalized_url, '#')?

$first_token  = strtok('#something', '/');
$second_token = strtok('#');
var_dump($first_token, $second_token);

Result will be "something" and "FALSE". I'm going to stay with preg_replace for now it is giving me more predictable results.

c960657’s picture

The problem with the regular expression is that if $fallback_identifier is http://example.com/abc#def?ghi, then preg_replace('/#[^?]*/', '', $fallback_identity) will change it into http://example.com/abc?ghi, but the expected value is http://example.com/abc.

I'm not sure what you mean by predictable? I didn't suggest that you call strtok() with only one argument.

wojtha’s picture

I didn't said that I won't change the regular expression. I was just trying this approach, because strtok seems like interesting tweak, but run into this issue, which I've mentioned above. (BTW The regex wasn't from my head, I had seen it in one of the openid libraries...)

I'm working on it ATM. I've implemented your opt-in idea, and now I'm trying to find reasonable way how to implement the max "aid" idea.

wojtha’s picture

Updated patch from #25, includes most of the @c960657 suggestions from the #26.

Max AID limit has not been implemented at the moment, since I would need to to override user_external_load() to use the "max AID at the update time idea". So I've postponed this.

wojtha’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1120290-34-openid_provide_transition_plus_1076366.patch, failed testing.

wojtha’s picture

Fixed one issue (wrong db_insert arguments which caused 500 internal error in the last test), added #575810: OpenID discovery spec violation - follow redirects. to the test (and fixed one isseu there too).

Successfully tested in my local dev environment.

wojtha’s picture

FileSize
309.04 KB

Little showcase ... what happen if the transition method is either enabled or disabled... (created using 1120290-36-openid_provide_transition_plus_1076366_plus_575810.patch)

See the attachment!

xjm’s picture

Tagging.

catch’s picture

Status: Needs review » Reviewed & tested by the community

There are only 2-3 people able to review this patch properly other than the author, and they are all busy. So I'm marking this RTBC.

I suggest we don't commit this before 7.8 comes out. But then do so immediately afterwards or with a short grace period. That gives four weeks for wojtha, c960657 or Heine to follow up if they need to before 7.10.

I personally reached out to walkah in irc to review the patch a few weeks ago, but it looks like he never got to it.

See #1255674-24: [meta] Make core maintainable for more background.

I did look at this patch a while ago. But while I once spent several hours debugging and eventually fixing 16 second login times with OpenID (D7) and OpenID provider (D6) modules, which involved reading a lot of code and reviewing some of the spec, I still do not, and have no desire to, understand the spec at all - it is byzantine and esoteric.

webchick’s picture

I'm pretty comfortable with that plan.

c960657’s picture

c960657’s picture

Which of the patches is supposed to be committed into 7.x? The last patch changes the return value of openid_discovery(), i.e. it breaks backwards compatibility between two 7.x releases. I don't think this is acceptable.

+ * This function provides a safe and less obtrusive procedure for users who

In default.settings.php the procedure is described as “less secure”, so I think it is confusing that it is described as “safe” here.

+    if (!empty($sreg_values['email']) && valid_email_address($sreg_values['email'])) {

Why it the valid_email_address() necessary? We don't do a similar in openid_form_user_register_form_alter().

+      // Set the account to the found one.
+      $account =& $fallback_account;

I asked about it before, but I don't think you replied: Why =& instead of just = ?

+    variable_set('openid_test_response', array());
+  }

Variables are deleted automatically during tearDown(), so you don't need to reset them here.

+    $identity_stripped = preg_replace('/#[^?]*/', '', $identity);
...
+    $identity_stripped = preg_replace('/#.*/', '', $identity);

These two lines are different for no good reason, I assume. I suggest making them identical.

+    $edit = array('openid_identifier' => $identity_stripped);
+    $this->drupalPost('user/login', $edit, t('Log in'));
+
+    // Check we are on the OpenID redirect form.
+    $this->assertTitle(t('OpenID redirect'), t('OpenID redirect page was displayed.'));
+
+    // Submit form to the OpenID Provider Endpoint.
+    $this->drupalPost(NULL, array(), t('Send'));
+
+    // Verify user was redirected away from user/login to an accessible page.
+    $this->assertResponse(200);

I'm not sure why the last check is relevant? But can't the other be replaced with just $this->submitLoginForm($identity_stripped); ?

I'm not sure that default.settings.php is the best place to document the openid_less_obtrusive_transition setting. The only users who would want to enable the setting are those who already installed D7 prior to the 7.8 release, and they already have a copy of settings.php and probably will not look in the default.settings.php. Would CHANGELOG.txt be a better place? Perhaps something like this:
“Fixed bug in OpenID module that prevents users of certain OpenID providers from logging in, if they registered before upgrading. If you have enabled the OpenID module prior to Drupal 7.8, see http://drupal.org/node/xxxx on how to provide a smoother transition for these users.”

Apart from these minor points I think you have done a good job, and I hope that we can soon get this annoying bug behind us.

wojtha’s picture

#43 c960657, thanks for review...

Which of the patches is supposed to be committed into 7.x?

The first one (w/o any of the "plus_XY") however tests of this patch is dependent on the #1076366: OpenID discovery spec violation - fragments are removed from claimed id issue.

The last patch changes the return value of openid_discovery(), i.e. it breaks backwards compatibility between two 7.x releases. I don't think this is acceptable.

That's needed unfortunately to fix the #575810: OpenID discovery spec violation - follow redirects. in Drupal 7. I've discussed this half year ago (in March) with Heine (see #12 #575810-12: OpenID discovery spec violation - follow redirects.) and we agreed this way is probably the best - alternatives:
1) change the API to allow returning more data than services array from the discovery methods(current approach)
2) leave the API almost unchanged, just pass $claimed_id variable as reference to the discovery methods (that scares both me and Heine esp. in D7, in D6 is the openid API simpler and this approach will be more appropriate to stay backward compatible)
3) use global variable ($GLOBAL or $_SESSION)...

In #13 #575810-13: OpenID discovery spec violation - follow redirects. you responded with:

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.

However I didn't found any better way for D7...

IMHO change of the API is necessary, the patch doesn't introduce new feature, it fixes CRITICAL bug in Openid which now breaks Openid standard. If you have better idea how to fix this w/o changing the return value of the openid_discovery() in D7 I'm open for discussion. I've tried all three approaches, this one is (IMHO) currently "the cleanest and the soundest one".

If the fix was commited in Drupal 7.1 the change will be more acceptable than committing it in 7.10, but still - what is more acceptable? Change of the API which affects 5% of the Drupal 7 sites (currently how much? 0.5% of the total Drupal sites?) or leaving critical bug behind... Plus it affects only sites with own discovery method implementation... so I'm sure the percentage will be less then 0.5%. In worst case it could affect dozen(s) of sites.

webchick’s picture

So here's what I'd kind of like to do.

I'd like to commit all of these OpenID critical patches to D7, something this week, which would give us a few weeks to work out any kinks before the next release. However, it's not clear to me if we should hold back on this patch, based on c960657's feedback. Those sound like legitimate concerns, and wothja's reply is only referencing part of them.

wojtha’s picture

@webchick #45, yeah, I have been working on the new patch meanwhile...

@c960657 #43:

+ * This function provides a safe and less obtrusive procedure for users who

Fixed.

+    if (!empty($sreg_values['email']) && valid_email_address($sreg_values['email'])) {

Since the value comes from the 3rd party I think it is better to make sure it contains the email address, validation of the e-mail address is very efficient, it uses PHP's built-in function filter_var().

+      // Set the account to the found one.
+      $account =& $fallback_account;

Fixed. A habit from the PHP4 period.

+    $identity_stripped = preg_replace('/#[^?]*/', '', $identity);

Fixed.

+    $edit = array('openid_identifier' => $identity_stripped);
+    $this->drupalPost('user/login', $edit, t('Log in'));
+
+    // Check we are on the OpenID redirect form.
+    $this->assertTitle(t('OpenID redirect'), t('OpenID redirect page was displayed.'));
+
+    // Submit form to the OpenID Provider Endpoint.
+    $this->drupalPost(NULL, array(), t('Send'));

Fixed - replaced by $this->submitLoginForm($identity_stripped).

$this->assertResponse(200);

It is not really necessary, but it helped me several times when I made something which went horribly wrong and I got 5xx error.

I'm not sure that default.settings.php is the best place to document the openid_less_obtrusive_transition setting. The only users who would want to enable the setting are those who already installed D7 prior to the 7.8 release, and they already have a copy of settings.php and probably will not look in the default.settings.php.

I'm considering default.settings.php in a similar way like openid.api.php, I think it is right place for the example and little help, however I agree that some kind propaganda in the Changelog.txt and in release information would be helpful. I was thinking also about displaying some warning during upgrade, but I'm not sure if it is worth to implement hook_update_N just for the warning message.

Status: Reviewed & tested by the community » Needs work
wojtha’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
11.49 KB

#575810: OpenID discovery spec violation - follow redirects. has made it to D7 before I post the last set of patches and the commited patch accidentally includes #1076366: OpenID discovery spec violation - fragments are removed from claimed id too so both dependencies are commited now and we can test just transition procedure alone.

Changes: simplification of the tests.

webchick’s picture

Status: Needs work » Needs review

Oops. :\

And let's get some review on this latest patch.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/modules/openid/openid.incundefined
@@ -793,3 +793,77 @@ function _openid_math_powmod($x, $y, $z) {
+ * This function provides less safe but less obtrusive procedure for users who

This doesn't read particularly well. I'd change it to "Provides a less safe but more unobtrusive procedure..."

+++ b/modules/openid/openid.incundefined
@@ -793,3 +793,77 @@ function _openid_math_powmod($x, $y, $z) {
+  // Step 1: Try to strip the fragment if it is present.

Please remove 'Step 1'.

+++ b/modules/openid/openid.incundefined
@@ -793,3 +793,77 @@ function _openid_math_powmod($x, $y, $z) {
+  // Step 2: Try to replace https with http. OpenID providers often redirects
+  // from http to https, but Drupal didn't follow these redirects.

s/often redirects/often redirect

s/these redirect/the redirect

+++ b/modules/openid/openid.incundefined
@@ -793,3 +793,77 @@ function _openid_math_powmod($x, $y, $z) {
+      drupal_set_message(t('There is already an existing account associated with the OpenID identifier that you have provided. However, due to the bug in the previous version of the authentication system, we can\'t be sure that this account belongs to you. If you are new on this site, please continue registering the new user account. If you already have a registered account on this site associated with the provided OpenID identifier, please try to <a href="@url_password">reset the password</a> or contact the site administrator.', array('@url_password' => 'user/password')), 'warning');

s/due to the bug/due to a bug.

Instead of 'can\'t' please double quote the full string - makes it simpler for translators.

+++ b/modules/openid/openid.moduleundefined
@@ -646,8 +646,15 @@ function openid_association($op_endpoint) {
+  // incompletely stored OpenID identifier in the authmap.
+  if (!isset($account->uid) && variable_get('openid_less_obtrusive_transition', FALSE)) {

Part of me feels this should be set to TRUE in an update for existing sites. I doubt people who are confused by this will find it. Those that care could set it to FALSE.

20 days to next Drupal core point release.

wojtha’s picture

re #50: Thanks catch, I've fixed all the English grammar errors.

Instead of 'can\'t' please double quote the full string - makes it simpler for translators.

Yea, as a former head of Czech translator team I'm trying to make the translators life easier, but this string is bit exceptional: did you noticed the <a href="@url_password">reset the password</a>? It is also part of the string... so if I change the quotes to double qoutes I need also to escape these doublequotes or I need to convert them to quotes - all browsers will handle that, but I dont like placing HTML attributes in the single-quotes.

if (!isset($account->uid) && variable_get('openid_less_obtrusive_transition', FALSE)) {
Part of me feels this should be set to TRUE in an update for existing sites. I doubt people who are confused by this will find it. Those that care could set it to FALSE.

I didn't change this yet, we need to discuss the security consequences of the "less obtrusive method" enabled. c960657 in his comment #26 called for the procedure to be opt-in only, not enabled by default. The procedure declares that it is less obtrusive but also less secure.

It uses legacy behavior to find the right account and it uses e-mail matching as a "safety catch". How someone will be able to steal someone other's account? We skip the complex attacks like man in the middle attack or DNS spoofing and using fake OpenID provider - we assume that the attack is possible using the original OpenID provider. In that case:

1. OpenID Less Obtrusive Transition Method ™ is enabled.
2. Victim created account before the issues (following redirects, stripping of the hash) were fixed.
3. Victim didn't login using OpenID and didn't delete the invalid identifier after that.
4. Attacker will need to have the same user identifier as the original user (victim).
5. Attacker will need to have the same e-mail address set as its primary e-mail.

Attacker could receive the same user identifiers only when the victim quits the provider before and if the provider is using identifier recycling. Then he needs to assign the same e-mail address as the victim has... in most cases the provider requires to verify the e-mail address or the e-mail address is tightly bound to the OpenID identifier (Yahoo ID, Google ID).

Basically the "attack" is possible only when the victim quits the OpenID provider and the identifier is recycled.

There are also differences between which providers were affected in Drupal 6 and Drupal 7. In Drupal 6 were affected just several providers using particular OpenID methods (e.g. Yahoo ID was working ok, but mojeID wasn't working). However Drupal 7 was stripping hashes "by design", so all providers with identifier recycling were affected.

wojtha’s picture

After thinking of the security consequences... this comes to my mind... after c960657's review in #26 I accepted his idea:

instead of doing an UPDATE we should rather INSERT a new row into {authmap}.

It is defensive style - let user decide what to delete. However I'm in doubts how many of the users will really delete the old identifier and all invalid identifiers (e.g. w/o the hash) are security threats for the site in the future - until the OpenID Less Obtrusive Transition Method ™will remain enabled.

wojtha’s picture

re #50 @catch

if (!isset($account->uid) && variable_get('openid_less_obtrusive_transition', FALSE)) {
Part of me feels this should be set to TRUE in an update for existing sites. I doubt people who are confused by this will find it. Those that care could set it to FALSE.

After some thinking around this, might be better strategy for the webmasters will be to enable the transition method just for some limited period of the time (month, several months) to let the active users a chance to update their's OpenID accounts unobtrusively and after that disable the transition method to be more secure. So no one will be able to steal the accounts of the inactive users with the incomplete OpenID identifiers in the future both willingly or unwillingly.

catch’s picture

This might be overthinking it but could we even do two updates, one to set it to true, one to false a couple of months later? I'd also be completely fine with the patch as it is, so if you think the more obtrusive transition is better let's do that. Can always be a note in the release notes.

catch’s picture

Issue tags: +Release blocker

Adding release blocker tag, next point release can't go out without a resolution to this since #1076366: OpenID discovery spec violation - fragments are removed from claimed id was committed without it.

catch’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

Talked to wojtha.

If we default to the more permissive method, then it leaves people's accounts still open to impersonation/hijack.

We think we could possibly implement a grace period of one or two months with some tricky updating of the default etc., but that will need co-ordination so let's do it in a follow-up if at all.

Adding 'needs change notification' tag - we'll need to put a note about this issue in the release notes (both the sec issue that is fixed and the transition behaviour, probably point to the existence of the variable).

catch’s picture

switching tag.

webchick’s picture

Title: Provide transition for accounts with incompletely stored OpenIDs » Change notice for Provide transition for accounts with incompletely stored OpenIDs
Category: bug » task
Status: Reviewed & tested by the community » Active

Blah. Really sorry about the delay in getting back to this patch. :(

I think this all looks good, except I do not want OpenID stuff in default.settings.php. If we leave that chunk out, then all the workaround are constrained to just /modules/openid, which is far preferable to me.

How I'd prefer to handle this instead is in a change notice which we can reference in the 7.9 release notes.

So...

Committed and pushed #51 (without the default.settings.php hunk) to 7.x!! YAY! wothja, you are a saint.

Marking back to a critical task though, for said change notice.

wojtha’s picture

Ok, so here is the part which was stripped from the settings.default.php:

/**
 * Automatic invalid OpenID identifiers transition:
 *
 * A less obtrusive but also less secure (and optional) method of invalid OpenID
 * identifier transition. Due to the bug in the OpenID module, wrong identifiers
 * were being saved to the authmap table (affects only particular OpenID
 * providers).
 *
 * If enabled, Drupal will try to find matching account from the user database
 * using the alternative OpenID identifier matching when the standard OpenID
 * login failed. If the found site user account and the OpenID account have
 * identical e-mail address, Drupal will assign the identifier to the existing
 * user account and login user to the site.
 *
 * If disabled or when e-mail addresses don't match, user must login to the site
 * using other method, e.g. using ordinary user login or password reset for the
 * one-time temporary login. After login, user could fix the account by himself
 * by re-adding his OpenID identifier and deleting the invalid one. For more
 * information, see http://drupal.org/node/1120290.
 *
 * Enable only if you upgrade from Drupal 6.x or Drupal 7.8 and below!
 *
 * Remove the leading hash sign to enable.
 */
# $conf['openid_less_obtrusive_transition'] = TRUE;

Who is supposed to add the change notice? Is there any example how it should look like? The comment is IMO good base.

catch’s picture

Change notice is here: http://drupal.org/node/add/changenotice

Comment looks like a good base to me too. Not sure if there's a template anywhere for changes affecting admins.

thatoneguy’s picture

Just so I understand this correctly: when the above comment states "if the email addresses match" -- this is if the email address sent by the IdP matches the address in the Drupal users table? Effectively, this would allow a crafty individual to roll his own IdP and take advantage of this scenario to take over an account. Seems that makes matters worse, not better.

Am I misunderstanding something?

wojtha’s picture

@thatoneguy No, the alternative matching algorithm will try to find the user account using the identifiers in a shape as they were saved by the previous buggy method and in addition the e-mail address is matched, which is kind of STO, but better than nothing. So it returns the security level to the almost same state before the fix and thats the reason why it is this method disabled by default. We wanted to provide some transition method for the issue, however we didn't find anything better. The alternative method is vulnerable in a same way as the standard Openid authentication method in Drupal pre 7.9 for certain providers.

How many users are affected?

You can review your authmap table, if you see the authnames like https://openidprovider.com/identifier#fG6X5T5 (notice: https protocol and hash on the end), these are safe. However if you see authnames like http://openidprovider.com/identifier (notice: http protocol and no hash), these accounts are potentially affected by this issue.

We recommend to enable the alternative method only for limited period of time to allow users migrate their Openid accounts easily without need of resetting their passwords or contacting the site support. We also recommend to use this method only if you have plenty of users with the "bad" openid identifiers in the authmap and if potential breaching their accounts won't have fatal consequences.

You can enable the alternative transition method by setting $conf['openid_less_obtrusive_transition'] = TRUE; in your settings.php.

wojtha’s picture

Title: Change notice for Provide transition for accounts with incompletely stored OpenIDs » Provide transition for accounts with incompletely stored OpenIDs
Category: task » bug
Issue tags: -Release blocker
catch’s picture

Status: Active » Fixed

Thanks!

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