New issue
Advanced search Search tips

Issue 10242 link

Starred by 2 users

Issue metadata

Status: Released
Owner: ----
Closed: Jan 11



Sign in to add a comment

A Gerrit user's account can be taken over when multiple authentication providers are in use

Reported by matt...@unsolvable.org, Jan 2

Issue description

This is observed in Gerrit 2.16 (with NoteDB), but I believe is present in earlier versions.

Scenario:
(1) Gerrit is using more that one authentication provider.
In our case, we are using OAuth with CAS and Github. Extract from gerrit.config:

[plugin "gerrit-oauth-provider-github-oauth"]
	fix-legacy-user-id = false
	client-id = xxxxxxxx
[plugin "gerrit-oauth-provider-cas-oauth"]
	root-url = https://auth.diamond.ac.uk/cas
	client-id = gerrit
	fix-legacy-user-id = false

(2) Person P has an existing Gerrit account with an identity managed by one of those providers.
In our case, P has a CAS-managed Gerrit account, and the Gerrit account name is "aaaaaaaa".

(3) Another person Q has an account with the other provider, which happens by chance to have the same provider account name as Gerrit user in P.
In out case, person Q has a GitHub account, and the GitHub account name is "aaaaaaaa".

(4) When person Q logs on to Gerrit for the first time, Gerrit sees that their username with the external authentication provider matches an existing Gerrit username, and links the two identities.
In our case, person Q is able to authenticate as person P just by owing a GitHub account whose name is the same as P's Gerrit username. 

The specific message that Gerrit logs when Q signs on for the first time is:
WARN  com.google.gerrit.server.account.AccountManager : User Optional[aaaaaaaa] already has an account; link new identity to the existing account.

This is clearly a major security hole - if I know or can discover the Gerrit username of an existing Gerrit user, I can in some situations take over their Gerrit account.

In our specific case, we are using the Gerrit OAuth provider plugin (https://github.com/davido/gerrit-oauth-provider), but it looks like the problem is actually in the core AccountManager class.


 
Please enable David Ostrovsky (david.ostrovsky@gmail.com) to view this issue.
Project Member

Comment 2 by david.os...@gmail.com, Jan 3

So, this is a known issue, that was addressed and solved
in earlier plugin versions.

The problem is to use a unique provider-prefix, and update the plugin to the most recent version.

In your example that would fix the problem:

* github-prefix: github-oauth: [1]
* cas-prefix: cas-oauth: [2]

Now, before a site can enable multiple OAuth providers, all user accounts must be migrated from none-prefixed account to prefixed accounts.

It can be easily achieved, by activating: 

[plugin "gerrit-oauth-provider-github-oauth"]
	fix-legacy-user-id = true

option. That opton would have the following effect:
for every user account "foo", a link would be created to the same account, but with prefix:

github-oauth:foo

Then and only then a site may activate additional OAuth provider, say google-oauth:

[plugin "gerrit-oauth-provider-google-oauth"]

Note: fix-legacy-user-id must not be used at all, because in the latest plugin version, provider prefix is always used.

[1] https://github.com/davido/gerrit-oauth-provider/blob/master/src/main/java/com/googlesource/gerrit/plugins/oauth/GitHubOAuthService.java#L48
[2] https://github.com/davido/gerrit-oauth-provider/blob/master/src/main/java/com/googlesource/gerrit/plugins/oauth/CasOAuthService.java#L49


Thanks David - we were running with fix-legacy-user-id=false. I'm not sure why, I _think_ it was because we had OAuth up and working before that option was introduced, so I ended up setting it to false since what we had was working. Obviously I didn't understand the implications.

Anyhow, that points me in the right direction. I will post here what I end up doing, just for the record.
After looking again, I think this is _not_ related to the provider-prefix (although I could be mistaken).
The problem is not with "identity": "github-oauth:nnnnn" or "identity": "cas-oauth:iiiiii"
The problem is with "identity": "username:vvvvv"

The problem occurs when:
(1) I sign in to Gerrit using a GitHub account (the GitHub account has never been used with Gerrit before)
(2) The _GitHub_username_ is the same as an _existing_Gerrit_account_username_  (that was created using another authentication provider)

Point 2 is the key here. The Gerrit log shows:
DEBUG com.googlesource.gerrit.plugins.oauth.GitHubOAuthService : User info response: {"login":"mwebber","id":562564" ...
WARN  com.google.gerrit.server.account.AccountManager : User Optional[mwebber] already has an account; link new identity to the existing account.
DEBUG com.google.gerrit.server.account.externalids.ExternalIdNotes : Reading external ID note map
DEBUG com.google.gerrit.server.account.AccountManager : Link another authentication identity to an existing account

Setting "fix-legacy-user-id = true" makes no difference.

Below you can see some details.
Any idea as to what needs to be done? Is this a bug / design issue?
I don't understand this enough to draw any definite conclusions.
Matthew

**********
* Config *
**********
gerrit.config
[plugin "gerrit-oauth-provider-github-oauth"]
	fix-legacy-user-id = false
	client-id = xxxxxxxxxxxxxxxxxxx
[plugin "gerrit-oauth-provider-cas-oauth"]
	root-url = https://auth.diamond.ac.uk/cas
	client-id = yyyyyyyyyyyyyyyyyyy
	fix-legacy-user-id = false

secure.config
[plugin "gerrit-oauth-provider-github-oauth"]
	client-secret = pppppppppppppppppp
[plugin "gerrit-oauth-provider-cas-oauth"]
	client-secret = qqqqqqqqqqqqqqqqqq

*******************
* Status at start *
*******************
$ curl -K ~/passwords/http-password_Gerrit_for-curl.txt "https://gerritbeta.diamond.ac.uk/a/accounts/webber/detail"
)]}'
{
  "registered_on": "2015-01-21 10:56:25.000000000",
  "_account_id": 1000000,
  "name": "Matthew Webber",
  "email": "matthew.webber@diamond.ac.uk",
  "secondary_emails": [],
  "username": "mwebber"
}

curl -K ~/passwords/http-password_Gerrit_for-curl.txt "https://gerritbeta.diamond.ac.uk/a/accounts/webber/external.ids"
)]}'
[
  {
    "identity": "mailto:matthew.webber@diamond.ac.uk",
    "email_address": "matthew.webber@diamond.ac.uk",
    "trusted": true,
    "can_delete": true
  },
  {
    "identity": "username:mwebber",
    "trusted": true
  },
  {
    "identity": "cas-oauth:bmn54829",
    "trusted": true,
    "can_delete": true
  }
]

******************************************************************************************
* Log on to WebUI and attempt to create a new Gerrit account using GitHub authentication *
******************************************************************************************
Instead of creating a new Gerrit account, it links the new identify an existing Gerrit account

$ curl -K ~/passwords/http-password_Gerrit_for-curl.txt "https://gerritbeta.diamond.ac.uk/a/accounts/webber/external.ids"
)]}'
[
  {
    "identity": "mailto:matthew.webber@diamond.ac.uk",
    "email_address": "matthew.webber@diamond.ac.uk",
    "trusted": true,
    "can_delete": true
  },
  {
    "identity": "github-oauth:562564",
    "trusted": true,
    "can_delete": true
  },
  {
    "identity": "username:mwebber",
    "trusted": true
  },
  {
    "identity": "cas-oauth:bmn54829",
    "trusted": true,
    "can_delete": true
  }
]

*************************************
* trace information from Gerrit log *
*************************************

[2019-01-04 10:22:18,087] [HTTP-2909] TRACE com.google.gerrit.httpd.restapi.RestApiServlet : Received REST request: GET /accounts/self/detail (parameters: [])
[2019-01-04 10:22:18,087] [HTTP-2909] TRACE com.google.gerrit.httpd.restapi.RestApiServlet : Calling user: AnonymousUser
[2019-01-04 10:22:18,087] [HTTP-2909] TRACE com.google.gerrit.httpd.restapi.RestApiServlet : REST call failed: 403
[2019-01-04 10:22:18,087] [HTTP-2909] TRACE com.google.gerrit.httpd.restapi.RestApiServlet : Text response body:
[2019-01-04 10:22:20,182] [HTTP-2909] DEBUG com.google.gerrit.httpd.auth.oauth.OAuthSession : Login OAuthSession [token=null, user=null]
[2019-01-04 10:22:20,182] [HTTP-2909] DEBUG com.google.gerrit.httpd.auth.oauth.OAuthSession : Login-PHASE1 OAuthSession [token=null, user=null]
[2019-01-04 10:22:20,182] [HTTP-2909] DEBUG com.google.gerrit.httpd.HttpServletResponseRecorder : Replaying 302 
[2019-01-04 10:22:20,416] [HTTP-2901] DEBUG com.google.gerrit.httpd.auth.oauth.OAuthSession : Login OAuthSession [token=null, user=null]
[2019-01-04 10:22:20,416] [HTTP-2901] DEBUG com.google.gerrit.httpd.auth.oauth.OAuthSession : Login-Retrieve-User OAuthSession [token=null, user=null]
[2019-01-04 10:22:21,226] [HTTP-2901] DEBUG com.googlesource.gerrit.plugins.oauth.GitHubOAuthService : User info response: {"login":"mwebber","id":562564,"node_id":"MDQ6VXNlcjU2MjU2NA==","avatar_url":"https://avatars2.githubusercontent.com/u/562564?v=4","gravatar_id":"","url":"https://api.github.com/users/mwebber","html_url":"https://github.com/mwebber","followers_url":"https://api.github.com/users/mwebber/followers","following_url":"https://api.github.com/users/mwebber/following{/other_user}","gists_url":"https://api.github.com/users/mwebber/gists{/gist_id}","starred_url":"https://api.github.com/users/mwebber/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mwebber/subscriptions","organizations_url":"https://api.github.com/users/mwebber/orgs","repos_url":"https://api.github.com/users/mwebber/repos","events_url":"https://api.github.com/users/mwebber/events{/privacy}","received_events_url":"https://api.github.com/users/mwebber/received_events","type":"User","site_admin":false,"name":"Matthew Webber","company":"Diamond Light Source","blog":"","location":"Oxfordshire, England","email":null,"hireable":null,"bio":null,"public_repos":2,"public_gists":1,"followers":10,"following":0,"created_at":"2011-01-13T11:50:04Z","updated_at":"2019-01-02T09:43:24Z"}
[2019-01-04 10:22:21,226] [HTTP-2901] DEBUG com.google.gerrit.httpd.auth.oauth.OAuthSession : Login-SUCCESS OAuthSession [token=null, user=com.google.gerrit.extensions.auth.oauth.OAuthUserInfo@394bbecc]
[2019-01-04 10:22:21,232] [HTTP-2901] DEBUG com.google.gerrit.server.account.externalids.ExternalIdNotes : Reading external ID note map
[2019-01-04 10:22:21,234] [HTTP-2901] DEBUG com.google.gerrit.server.account.externalids.ExternalIdNotes : Reading external ID note map
[2019-01-04 10:22:21,235] [HTTP-2901] WARN  com.google.gerrit.server.account.AccountManager : User Optional[mwebber] already has an account; link new identity to the existing account.
[2019-01-04 10:22:21,237] [HTTP-2901] DEBUG com.google.gerrit.server.account.externalids.ExternalIdNotes : Reading external ID note map
[2019-01-04 10:22:21,238] [HTTP-2901] DEBUG com.google.gerrit.server.account.AccountManager : Link another authentication identity to an existing account
[2019-01-04 10:22:21,238] [HTTP-2901] DEBUG com.google.gerrit.server.account.AccountManager : Linking new external ID to the existing account
[2019-01-04 10:22:21,243] [HTTP-2901] DEBUG com.google.gerrit.server.git.meta.VersionedMetaData : Read file 'account.config' from ref 'refs/users/00/1000000' of project 'All-Users' from revision '2fe80e6e9cb22dd2d6e0eb2ec3358698cc62e364'
[2019-01-04 10:22:21,243] [HTTP-2901] DEBUG com.google.gerrit.server.git.meta.VersionedMetaData : Read file 'watch.config' from ref 'refs/users/00/1000000' of project 'All-Users' from revision '2fe80e6e9cb22dd2d6e0eb2ec3358698cc62e364'
[2019-01-04 10:22:21,244] [HTTP-2901] DEBUG com.google.gerrit.server.git.meta.VersionedMetaData : Read file 'preferences.config' from ref 'refs/users/00/1000000' of project 'All-Users' from revision '2fe80e6e9cb22dd2d6e0eb2ec3358698cc62e364'
[2019-01-04 10:22:21,244] [HTTP-2901] DEBUG com.google.gerrit.server.git.meta.VersionedMetaData : Read file 'preferences.config' from ref 'refs/users/default' of project 'All-Users' from revision '93fff9516420c954fd86c3259bdb2ec935e4c0ed'
[2019-01-04 10:22:21,246] [HTTP-2901] DEBUG com.google.gerrit.server.account.externalids.ExternalIdNotes : Reading external ID note map
[2019-01-04 10:22:21,251] [HTTP-2901] DEBUG com.google.gerrit.server.logging.TraceContext.TraceTimer : Loading external IDs (revision=AnyObjectId[5290ae9b0d65e4568d855d2cc2aceff7ac729011]) (6 ms)
[2019-01-04 10:22:21,252] [HTTP-2901] DEBUG com.google.gerrit.server.account.externalids.ExternalIdNotes : Reading external ID note map
[2019-01-04 10:22:21,253] [HTTP-2901] DEBUG com.google.gerrit.server.git.meta.VersionedMetaData : Save file 'account.config' in ref 'refs/users/00/1000000' of project 'All-Users'
[2019-01-04 10:22:21,254] [HTTP-2901] DEBUG com.google.gerrit.server.account.externalids.ExternalIdNotes : Updating external IDs
[2019-01-04 10:22:21,371] [HTTP-2901] DEBUG com.google.gerrit.server.account.AccountCacheImpl : Evict account 1000000
[2019-01-04 10:22:21,371] [HTTP-2901] DEBUG com.google.gerrit.server.account.AccountCacheImpl : Evict account 1000000
[2019-01-04 10:22:21,374] [HTTP-2901] DEBUG com.google.gerrit.server.git.meta.VersionedMetaData : Read file 'account.config' from ref 'refs/users/00/1000000' of project 'All-Users' from revision '2fe80e6e9cb22dd2d6e0eb2ec3358698cc62e364'
[2019-01-04 10:22:21,374] [HTTP-2901] DEBUG com.google.gerrit.server.git.meta.VersionedMetaData : Read file 'watch.config' from ref 'refs/users/00/1000000' of project 'All-Users' from revision '2fe80e6e9cb22dd2d6e0eb2ec3358698cc62e364'
[2019-01-04 10:22:21,374] [HTTP-2901] DEBUG com.google.gerrit.server.git.meta.VersionedMetaData : Read file 'preferences.config' from ref 'refs/users/00/1000000' of project 'All-Users' from revision '2fe80e6e9cb22dd2d6e0eb2ec3358698cc62e364'
[2019-01-04 10:22:21,375] [HTTP-2901] DEBUG com.google.gerrit.server.git.meta.VersionedMetaData : Read file 'preferences.config' from ref 'refs/users/default' of project 'All-Users' from revision '93fff9516420c954fd86c3259bdb2ec935e4c0ed'
[2019-01-04 10:22:21,386] [HTTP-2901] DEBUG com.google.gerrit.server.logging.TraceContext.TraceTimer : Loading account 1000000 (15 ms)
[2019-01-04 10:22:21,386] [HTTP-2901] DEBUG com.google.gerrit.server.index.account.AccountIndexerImpl : Replace account 1000000 in index
[2019-01-04 10:22:21,501] [HTTP-2901] DEBUG com.google.gerrit.server.logging.TraceContext.TraceTimer : Replacing account 1000000 in index version 9 (115 ms)
[2019-01-04 10:22:21,517] [Index-Batch-3] DEBUG com.google.gerrit.index.query.QueryProcessor : Executing 1 changes index queries for com.google.gerrit.server.index.change.ReindexAfterRefUpdate$GetChanges.impl(ReindexAfterRefUpdate.java:162) [CONTEXT PLUGIN="gerrit" ]

<< end >>

Project Member

Comment 5 by david.os...@gmail.com, Jan 4

I thing you are right, and I suspect, that this behaviour is a regression from this commit: [1]. I think that in review process of this commit, we have forgotten about different autherntication providers, possibly having the same username for different identities. For different auth schemes, like LDAP that problem doesn't exist.

If different OAuth providers are used (like in your example: CAS-OAuth2-provider and GitHub-OAuth2-Provider), then linking should only take place, when you was already logged in in account_1 and trigger linking by selecting link from the UI to a different account, account_2 possibly from another OAuth2 provider or even OpenID provider. However if you are not linking accounts, then a fresh new account must be created. This still raises the question, what will happen in our case, when username is already taken.

Can you revert: [1] and retry again and report if reverting [1] fixed the unwanted linking problem? Can you also test a different scenario, where you actually know, that username "mwebber" on CAS and GitHub is the same user/identity and 
you effectively would like to link those accounts, by clicking link from the UI. Is linking working in that case?

[1] https://gerrit-review.googlesource.com/c/gerrit/+/162450

Yes, that looks like the commit that introduced the problem.

- I agree that linking should only take place, when you was already logged in in account_1 and trigger linking by selecting link from the UI to a different account

- If the username is already taken, then i think it's acceptable that no username should be assigned, and the user will have to choose one. We already have to do this in the case where the external authentication does not return a username, which is possible. In general, Gerrit does not require external authenticators to provide any particular information (hence "Anonymous Coward").

I'm happy to do the tests, but I'm not in a position to build a new Gerrit. If you can provide me with a .jar to test (based on 2.16 would be good), I'm happy to test with that.

Thanks for looking at this.
The fix that looks like it introduced this problem (see comment 5)was to address bug https://bugs.chromium.org/p/gerrit/issues/detail?id=7652

Therefore, I would suggest
cc: ekempin@google.com
as he is familiar with the original bug, and with the account stuff
Project Member

Comment 8 by david.os...@gmail.com, Jan 5

> If you can provide me with a .jar to test (based on 2.16 would be good), I'm happy to test with that.

Here is release.war: [1] built on stable-2.16 branch with this diff included: [2].

[1] https://ostrovsky.org/gerrit/release-v2.16.2-50-gceacc25ba0.war
[2] http://paste.openstack.org/show/740074
Project Member

Comment 9 by david.os...@gmail.com, Jan 5

Status: ChangeUnderReview (was: New)
https://gerrit-review.googlesource.com/c/gerrit/+/209089
I've tested that .jar with the fix (I'm running NoteDB).

(1) Unfortunately, it won't let me link identities.
- I log on with my existing CAS credentials
- I switch to the Old UI (PolyGerrit doesn't support "Link another identity"!)
- Do "Link another identity" and specify GitHub as authentication provider
- The browser displays "Forbidden", and error?log shows a stack trace:
[2019-01-07 09:15:32,510] [HTTP-78] ERROR com.google.gerrit.httpd.auth.oauth.OAuthSession : Unable to authenticate user "com.google.gerrit.extensions.auth.oauth.OAuthUserInfo@211d105"
com.google.gerrit.server.account.AccountException: Cannot assign external ID "username:mwebber" to account 1000108; external ID already in use.
	at com.google.gerrit.server.account.AccountManager.create(AccountManager.java:307)
	at com.google.gerrit.server.account.AccountManager.authenticate(AccountManager.java:141)
	at com.google.gerrit.httpd.auth.oauth.OAuthSession.authenticateAndRedirect(OAuthSession.java:146)
	at com.google.gerrit.httpd.auth.oauth.OAuthSession.login(OAuthSession.java:110)
	at com.google.gerrit.httpd.auth.oauth.OAuthWebFilter.doFilter(OAuthWebFilter.java:105)

NOTE: the 1000108 account number here is a new number, it should be trying to link to the existing 100000 account number.

(2) If I log out, and the log in using GitHub, I expect it to just create a new account. Instead, I get a similar error:
[2019-01-07 09:30:12,268] [HTTP-139] ERROR com.google.gerrit.httpd.auth.oauth.OAuthSession : Unable to authenticate user "com.google.gerrit.extensions.auth.oauth.OAuthUserInfo@7e4d733"
com.google.gerrit.server.account.AccountException: Cannot assign external ID "username:mwebber" to account 1000109; external ID already in use.
	at com.google.gerrit.server.account.AccountManager.create(AccountManager.java:307)
	at com.google.gerrit.server.account.AccountManager.authenticate(AccountManager.java:141)
	at com.google.gerrit.httpd.auth.oauth.OAuthSession.authenticateAndRedirect(OAuthSession.java:146)
	at com.google.gerrit.httpd.auth.oauth.OAuthSession.login(OAuthSession.java:110)

Again, the 1000109 account number here is a new number, which is what is expected here

Project Member

Comment 11 by david.os...@gmail.com, Jan 7

Thanks for testing. And in fact neither cfreating a new account,
nor linking is working in case the user name is already taken.

One possible workaround to make the account linking work again is to make sure to use different user names across different identity providers: e.g.: 

* CAS-OAuth2: mwebber
* GitHUb-OAuth2: mwebber_github

The fact, that PolyGerrit UI doesn't support account linking is a serious regression, that would prevent Gerrit sites to upgrade to Gerrit 3.0, that depend on account linking.

However, the priority is to fix the security vulnerability here. Can you confirm, that the revert solved the non wanted linking of different users? ideally, by voting VRFY+1 on the referenced commit?

Labels: FixedIn-2.14.18 FixedIn-2.16.3 FixedIn-2.15.8
Status: Released (was: ChangeUnderReview)
Labels: -Security -NonPublic
Removing the visibility restriction now that the issue is fixed.

Sign in to add a comment