OpenID must use canonical ID when authenticating XRI i-names

subscribing

This seems to be critical. The implementation of OpenID seems to be insecure, as the i-name can be taken over and/or reassigned, so the user is either losing access to its Drupal account or the new owner might even gain access. Such an incorrect implementation can not be shipped with core.
However, this is certainly non-trivial, and so I beg our OpenID experts to take a look at this.

Pancho: could you please give examples on how someone takes over a Drupal account, and only mark this critical if you have a concrete reproducable example. Otherwise it is just an OpenID bug.

I apologize if I don't understand this well enough, as I haven't tinkered with OpenID in Drupal just yet, but isn't this more or less the same problem as already exists in both Drupal without OpenID, and Drupal using OpenID logins that are URL's and not iNames?

Examples of all three attack scenarios:

typical Drupal without OpenID
- I login to a drupal site with webmaster@example.com.
- Later, example.com expires and someone else buys example.com.
- One day, they notice a drupal notification or something that comes to webmaster@example.com, because I haven't re-visited the drupal site and updated my e-mail.
- They go to the drupal site from the notification email, and hit "forgot password" and put in webmaster@example.com for their e-mail, and they are automatically sent my username and a forgot password link that gets them into my account.

Drupal with OpenID URL used to register/login
It's also the same situation exactly with an OpenID URL that is not resolved through XRI/iNames...
- I set up an OpenID server at http://example.com/openid/server and use that OpenID URL to log in to a drupal site.
- example.com expires, and someone else sets up their own server and uses the URL to login (again, perhaps after receiving a notification from Drupal that alerts them to the existence of the account)

Drupal with iName used for OpenID login
- I log in with =damien
- later I decide I'd prefer =damien.norris so I register that iName and I let =damien expire
- someone else comes along and gets =damien and guesses somehow I have the account and logs in with it.

personally, I see this third scenario as the least risky of the three. In the top two scenarios, the attacker gets alerted to the presence of the account by receiving a notification email. In the bottom scenario, this is not the case - they would have to have some sort of psychic powers or at least be targeting me for this attack.

Anyway, with XRI and iNames, if it's possible to fix this anyway by resolving the canonical ID (is this easy? possible?) and storing that along with or instead of $edit['auth_openid'] = $identity; in the register logic of function openid_authentication? That way the third scenario is closed as only someone having the matching CanonicalID is really the same person as far as drupal is concerned.

I agree with Gábor that this is not a critical bug, given that it exists in a much more exploitable form for for regular Drupal logins (which are ultimately authenticated by receiving an email) or even for OpenID URls (which ultimately authenticate via ownership of an URL). It is certainly something important to fix so that the module complies properly with the specs, but it doesn't represent a heinous security vulnerability / critical bug, seeing as every drupal site is already affected by the easier two variants of the same attack and there is no way to close those.

I apologize if anything here is naive or crazy, as I said I haven't actually -used- the drupal openID module just yet, only perused its code in CVS to get a feel for how it works. I will be playing with it in the next few months, however, and can't wait!

Thanks, Damien for coming up with those scenarios. While I need to dig through the OpenID stuff the next weeks, I definitely think this needs to be reconsidered for D7 and maybe backported.
While I understand that Gábor set this back to normal in the D6 queue, this surely qualifies as critical in the D7 queue. When I assign this to me, it is thought only as a reminder - feel free to reassign, when someone has the solutions ready-to-go.

I don't think this is going to be that difficult a fix, it depends how the module is doing the XRI resolution. I got halfway through reading the OpenID 2.0 spec today and it does say that RP's can make use of XRI resolvers such as http://xri.net to do the resolution. Therefore I'm sure it's not that difficult to get the CanonicalID, in a pinch it can be done that way.

Then all that's required is to save it where I indicated in user registration and always make sure that the canonicalID's match when validating the user and that should be that.

Ideally we'd make this so new registrations would get saved using the CanonicalID, and old OpenID logins would get converted to the new way as the people log in (ie. people that were identified in the authmap with =foo would end up with =!DEAD.BEEF.DEAD.BEEF.

Armed with my newfound knowledge from that spec, I tried to find my own canonicalID, with: "curl http://xri.net/=damien". No dice, I ended up at the contact page for my iName. BUT, after looking in xrds.inc I see that it's just me using the wrong headers. If the XRI resolver receives the "Accept: application/xrds+xml" it will kick down the XRDS document instead of redirecting with HTTP 302.

So, I tried again forcing that header into curl and voila, back comes my XRDS document and it includes my CanonicalID. (can anyone verify if it MUST contain my CanonicalID?)

[~] lappy% curl -H "Accept: application/xrds+xml" http://xri.net/=damien
<?xml version="1.0" encoding="UTF-8"?>
<XRDS ref="xri://=damien" xmlns="xri://$xrds">
 <XRD xmlns="xri://$xrd*($v*2.0)">
  <Query>*damien</Query>
  <Status code="100"/>
  <Expires>2008-02-21T04:39:53.000Z</Expires>
  <ProviderID>xri://=</ProviderID>
  <LocalID priority="10">!F471.DE8E.3DA5.365D</LocalID>
  <CanonicalID priority="10">=!F471.DE8E.3DA5.365D</CanonicalID>
 [ ... some stuff snipped for brevity ... ]
  <Service priority="10">
   <Type match="content" select="true">http://openid.net/signon/1.0</Type>
   <ProviderID>xri://!!1008</ProviderID>
   <URI append="none" priority="20">http://1id.com/sso/</URI>
   <URI append="none" priority="10">https://1id.com/sso/</URI>
  </Service>
[ ... more snipped for brevity ... ]
 </XRD>
</XRDS>

et voila... So all we have to do is extend the parsing in xrds.inc so that it captures this CanonicalID as well as the OpenID service.

This is probalby easier said than done, there are all sorts of use cases. There is also OpenID 1.0 vs 2.0 service identifiers (do they both occur in the wild? do we have to support both?). Also it would be a kludge to how the openid_discovery() function works as it's returning the array of services etc and doesn't know about CanonicalID and it doesn't really fit in with the flow of the logic. Since everyting is so neat and tidy in this module, it warrants some thinking about how to do it neatly before jumping in with the bone saw and duct tape.

Finally, there is the whole issue of converting existing records to work with the new scheme of storing the CanonicalID as the primary key used for OpenID. It should work seamlessly so old logins work, but new logins use the new scheme, and anyone logging in is converted over. Site admins should not have to do anything to make it work (except perhaps run an update.php hook when upgrading the module version).

None of these on their own are complicated changes but combined we are talking about quite a bit changing, and it will require due care and attention to implement and then lots of testing of various use cases.

I'd be into helping with this once I get a bit further along in my projects that are going to involve Drupal/OpenID. Timeframe on that is a month or two. Meanwhile I'm happy to offer advice/encouragement/criticism/testing if you or someone else wants to attempt it. :)

OK and furthermore, reading =jbradley's blog, it sounds like he was responding to a question from =Fen which was originally posed by James Walker (walkah)... like yesterday it would seem... in response to this thread right here.

So it -sounds- like maybe it's already being worked on by James? It also -sounds- from reading John's comments that it is definitely a lot more complicated than my simple use case above... just what I was on about in the last comment saying that it's not going to be as simple as it sounded. We might not always get a canonicalID and need to properly follow the specs for normalizing the XRI so our claimed ID is as normalized as possible.

Please see John's post to his blog at http://thread-safe.livejournal.com/9103.html for more information.

James, are you working on a fix for this? If so let's assign the issue to you as it's being worked on already.

This was discussed in the security team and elsewhere, and we agreed that this is missing implementation other libraries admit as well. A fix is still expected for Drupal 6.x, unless the OpenID maintainer (James Walker) decides otherwise. My last info is that he has plans to implement this as a bugfix update for 6.x.

@Gábor,

OK that sounds great. I discussed this some more with the OpenID testers group on Plaxo Pulse (the discussion is mirrored publically at =jbradley's thead safe blog). According to those guys, we WILL always get the CanonicalID when doing XRI resolution via http://xri.net (which is how Drupal OpenID is doing it in xrds.inc) - regardless of whether it resolves to an OpenID 1.0, 1.1 or 2.0 service endpoint for the authentication. That part is irrelevant to finding the CanonicalID.

Furthermore, they explained what to do when it's not there or not an iNumber. If the CanonicalID is not there, we should be failing, and if the CanonicalID comes back with something that looks just like the claimed ID, then it's a test iName and we can just accept that CanonicalID that we get back and use that (much as it's being used in Drupal OpenID today).

Here's a snippet of the end of the conversation (see the rest at John's thread safe blog if interested)

Me:

OK... but the issue for the drupal folks is they are just using the claimed ID not the canonical ID to save as the primary key... so OP resolution aside, if they can be guaranteed that they'll always receive the CanonicalID from http://xri.net/=example (regardless of which iBroker =example was registered at) then I think it will solve the problem for them.

I rooted through their code and it does do all XRI resolution via http://xri.net. So, if they/I modified the code to snag the CanonicalID returned by that resolver (assuming it is always present) then it could be used for the primary key.

The bigger problem no one seems to have articulated yet aside from the URL recylcing problem is what happens if I have two iNames resolving to the same iNumber then decide to login with the second one... Drupal will create a different account for me instead of realizing the two iNames point to the same iNumber and therefore are the same persistent identity. In my mind this is the bigger problem than the security vulnerability of recycled iNames.... that is just never going to happen in practice, where i could see the two accounts when it should be one being a much more prevalent issue.

SO - will XRI resolution via http://xri.net always yield a CanonicalID? If not, when doesn't it?

Michael Krellin:

Damien, it MUST be always the case, but, for instance, try to resolve @xrid*hacker - the free iname I created for testing. It has '@xrid*hacker' as a CanonicalID! And that's because I put it there, previously it had nothing. The same goes for @id*, or @free* inames. That means, technically it's possible to have an iname with no CanonicalID. Although, when doing xri resolution you should fail the discovery on the CanonicalID-less inames. Well, if CanonicalID is set to something like '@xrid*hacker', feel free to accept it, it's iname holder/broker problem, not RP's.

Two inames resolving to the same inumber shouldn't result in the new account, since CID must be used as a key, not iname. It's also possible for iname to resolve to different CIDs using different service types (for instance for 1.1 and 2.0 service endpoints), but we came to conclusion that it must also be an XRD author's problem.

... so there you have it!

Also note what I was saying in my quote about the problem with two iNames representing the same person ending up with two different drupal accounts. Besides the (unlikely) security scenario of recycled iNames, I see this as a much bigger problem. If I switch between iNames, or happen to use two, they should still represent the same identity to Drupal, which would happen with the CanonicalID fix implemented.

This still breaks down a bit if a user has gone and got themselves a community iName, such as @drupal.org*damien_vancouver (dare to dream!!) and they also have themselves a personal iName (=damien) and these resolve to two different iNumbers... it would still result in Drupal seeing those two iNames as different. There is undoubtedly some clever XDI way of associating these two, but one thing at a time.

While I don't see this bug as -critical-, I do think it is important to fix so we are compliant, and that usability issue with ending up with 2 accounts is fixed. (the security issue is important too but I see the usability issue as more important). Just because other libraries are not compliant doesn't mean it's OK for Drupal not to be, especially when we are looking at quite a simple fix.

James, please let me know if I can help in any way. I would be happy to discuss and/or come up with a patch / test. I am also happy to take responsibility for backporting the Drupal 6 bugfix to the Drupal 5 version, which is probably what I'll be running in production for a while.

I see that there are people much more involved into OpenID than me... :) Unassigning...

@pancho,

I stopped pasting every reply into john's =thread_safe blog, but I learned a lot more in the Plaxo openID testers group thread about how we are not properly OpenID 2.0 compliant. The XRI resolution needs to recurse and do several things differently.

I have been pointed at sample code from the C++ library on how to do it in a compliant way. I'm just waiting to hear back from walkah on how I can help and then we can attack this.

but anyway I will stick with this until we are properly compliant, in whatever role I can help in (whether making the patch, gathering requirements, helping design fix, or just testing).

In short, my old capture it in a $_SESSION plan is not good enough... needs to be more in depth a fix than that. But nothing insurmountable!

You can definitely help with the testing when the time comes!

OMG, I've just reported an OpenID login problem to DC2009.drupalcon.org, and it turns out Yahoo modified their internal OpenID format and it is therefore affected by this missing fuctionality in Drupal.

Not critical. Please read and understand Priority levels of Issues, thanks.

@sun - this seems like a release blocker.

This patch now used the <CanonicalID> element from the XRDS document. In order to do this I decided not to extend the horrible SAX-based XRDS parser but rather move to a much simpler implementation based on SimpleXML.

The dummy proxy resolver failed when used with unclean URLs.

Small adjustment.

This looks solid, and the tests still pass. Committed to CVS HEAD.

Does this need to be backported to Drupal 6?

Ouch, I forgot to check the cid attribute for the result of the proxy's verification of CanonicalID (section 14.3.4):
http://docs.oasis-open.org/xri/xri-resolution/2.0/specs/cd03/xri-resolut...
I'll wrap up a patch tomorrow.

Backporting to D6 is feasible and relevant. Note that unless we implement some kind of transition mechanism, any i-names added to existing accounts no longer work but will have to be re-added to the account.

Here is the promised follow-up patch that adds the necessary check for <Status cid="verified">.

Thanks c960657. Committed to CVS HEAD. Thanks.

D6 backport.

Reroll.

Patch still applies. The testbot has an issue with D6 patches at the moment (#961172).

#31: openid-i-names-D6-2.patch queued for re-testing.