opendev/gerrit
Code Issues Proposed changes

Close security hole allowing normal user to become admin

Browse Source 
Icc105c39e introduced severe security problem, allowing non admin
users that was granted modifyAccount capability to modify SSH keys
of all users, including administrators. But that means such a user
can change the authentication of an existing administrator and then
impersonate the admin to elevate their own account status to that
of an admin.

Rectify it but restricting changing of SSH keys for a user only to
members of administrators group.

Change-Id: If82965391369121b282b969e7072a2bfa3074be9
This commit is contained in:
David Ostrovsky 2014-08-22 08:20:59 +02:00 committed by David Ostrovsky
parent cf9bce2191
commit c055d46280
4 changed files with 17 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
Documentation/cmd-set-account.txt
View File

@ -26,6 +26,8 @@ verification step we force within the UI.
Caller must be a member of the privileged 'Administrators' group,
or have been granted
link:access-control.html#capability_modifyAccount[the 'Modify Account' global capability].
For security reasons only the members of the privileged 'Administrators'
group can add or delete SSH keys for a user.
To set the HTTP password for the user account (option --http-password) or
to clear the HTTP password (option --clear-http-password) caller must be

2
gerrit-server/src/main/java/com/google/gerrit/server/account/AddSshKey.java
View File

@ -63,7 +63,7 @@ public class AddSshKey implements RestModifyView<AccountResource, Input> {
public Response<SshKeyInfo> apply(AccountResource rsrc, Input input)
throws AuthException, BadRequestException, OrmException, IOException {
if (self.get() != rsrc.getUser()
&& !self.get().getCapabilities().canModifyAccount()) {
&& !self.get().getCapabilities().canAdministrateServer()) {
throw new AuthException("not allowed to add SSH keys");
}
return apply(rsrc.getUser(), input);

14
gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteSshKey.java
View File

@ -14,9 +14,11 @@
package com.google.gerrit.server.account;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.DeleteSshKey.Input;
import com.google.gerrit.server.ssh.SshKeyCache;
import com.google.gwtorm.server.OrmException;
@ -32,18 +34,26 @@ public class DeleteSshKey implements
public static class Input {
}
private final Provider<CurrentUser> self;
private final Provider<ReviewDb> dbProvider;
private final SshKeyCache sshKeyCache;
@Inject
DeleteSshKey(Provider<ReviewDb> dbProvider, SshKeyCache sshKeyCache) {
DeleteSshKey(Provider<ReviewDb> dbProvider,
Provider<CurrentUser> self,
SshKeyCache sshKeyCache) {
this.self = self;
this.dbProvider = dbProvider;
this.sshKeyCache = sshKeyCache;
}
@Override
public Response<?> apply(AccountResource.SshKey rsrc, Input input)
throws OrmException {
throws AuthException, OrmException {
if (self.get() != rsrc.getUser()
&& !self.get().getCapabilities().canAdministrateServer()) {
throw new AuthException("not allowed to delete SSH keys");
}
dbProvider.get().accountSshKeys()
.deleteKeys(Collections.singleton(rsrc.getSshKey().getKey()));
sshKeyCache.evict(rsrc.getUser().getUserName());

3
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetAccountCommand.java
View File

@ -18,6 +18,7 @@ import com.google.common.base.Strings;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.extensions.annotations.RequiresCapability;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.RawInput;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
@ -260,7 +261,7 @@ final class SetAccountCommand extends SshCommand {
}
}
private void deleteSshKey(SshKeyInfo i) throws OrmException {
private void deleteSshKey(SshKeyInfo i) throws AuthException, OrmException {
AccountSshKey sshKey = new AccountSshKey(
new AccountSshKey.Id(user.getAccountId(), i.seq), i.sshPublicKey);
deleteSshKey.apply(