opendev/gerrit
Code Issues Proposed changes

Merge changes If8296539,Id907cc10

Browse Source 
* changes:
  Close security hole allowing normal user to become admin
  Remove the generateHttpPassword capability
This commit is contained in:
David Pursehouse 2014-10-22 07:14:10 +00:00 committed by Gerrit Code Review
commit 76623a1a56
12 changed files with 22 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
Documentation/access-control.txt
View File

@ -1205,13 +1205,6 @@ This capability doesn't imply permissions to the show-caches command. For that
you need the <<capability_viewCaches,view caches capability>>.
[[capability_generateHttpPassword]]
=== Generate HTTP Password
Allow the user to generate HTTP passwords for other users. Typically this would
be assigned to a non-interactive users group.
[[capability_kill]]
=== Kill Task

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

@ -26,12 +26,12 @@ 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
a member of the privileged 'Administrators' group, or have been granted
link:access-control.html#capability_generateHttpPassword[the 'Generate HTTP Password' global capability]
in addition to 'Modify Account' global capability.
a member of the privileged 'Administrators' group.
== SCRIPTING
This command is intended to be used in scripts.

4
gerrit-common/src/main/java/com/google/gerrit/common/data/GlobalCapability.java
View File

@ -61,9 +61,6 @@ public class GlobalCapability {
/** Can flush any cache except the active web_sessions cache. */
public static final String FLUSH_CACHES = "flushCaches";
/** Can generate HTTP passwords for user other than self. */
public static final String GENERATE_HTTP_PASSWORD = "generateHttpPassword";
/** Can terminate any task using the kill command. */
public static final String KILL_TASK = "killTask";
@ -112,7 +109,6 @@ public class GlobalCapability {
NAMES_ALL.add(CREATE_PROJECT);
NAMES_ALL.add(EMAIL_REVIEWERS);
NAMES_ALL.add(FLUSH_CACHES);
NAMES_ALL.add(GENERATE_HTTP_PASSWORD);
NAMES_ALL.add(KILL_TASK);
NAMES_ALL.add(MODIFY_ACCOUNT);
NAMES_ALL.add(PRIORITY);

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);

6
gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java
View File

@ -169,12 +169,6 @@ public class CapabilityControl {
|| canAdministrateServer();
}
/** @return true if the user can generate HTTP passwords for users other than self. */
public boolean canGenerateHttpPassword() {
return canPerform(GlobalCapability.GENERATE_HTTP_PASSWORD)
|| canAdministrateServer();
}
/** @return true if the user can impersonate another user. */
public boolean canRunAs() {
return canPerform(GlobalCapability.RUN_AS);

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());

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

@ -20,7 +20,6 @@ import static com.google.gerrit.common.data.GlobalCapability.CREATE_GROUP;
import static com.google.gerrit.common.data.GlobalCapability.CREATE_PROJECT;
import static com.google.gerrit.common.data.GlobalCapability.EMAIL_REVIEWERS;
import static com.google.gerrit.common.data.GlobalCapability.FLUSH_CACHES;
import static com.google.gerrit.common.data.GlobalCapability.GENERATE_HTTP_PASSWORD;
import static com.google.gerrit.common.data.GlobalCapability.KILL_TASK;
import static com.google.gerrit.common.data.GlobalCapability.MODIFY_ACCOUNT;
import static com.google.gerrit.common.data.GlobalCapability.PRIORITY;
@ -115,7 +114,6 @@ class GetCapabilities implements RestReadView<AccountResource> {
have.put(CREATE_PROJECT, cc.canCreateProject());
have.put(EMAIL_REVIEWERS, cc.canEmailReviewers());
have.put(FLUSH_CACHES, cc.canFlushCaches());
have.put(GENERATE_HTTP_PASSWORD, cc.canGenerateHttpPassword());
have.put(KILL_TASK, cc.canKillTask());
have.put(MODIFY_ACCOUNT, cc.canModifyAccount());
have.put(RUN_GC, cc.canRunGC());

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

@ -36,7 +36,7 @@ public class GetHttpPassword implements RestReadView<AccountResource> {
public String apply(AccountResource rsrc) throws AuthException,
ResourceNotFoundException {
if (self.get() != rsrc.getUser()
&& !self.get().getCapabilities().canGenerateHttpPassword()) {
&& !self.get().getCapabilities().canAdministrateServer()) {
throw new AuthException("not allowed to get http password");
}
AccountState s = rsrc.getUser().state();

6
gerrit-server/src/main/java/com/google/gerrit/server/account/PutHttpPassword.java
View File

@ -79,19 +79,19 @@ public class PutHttpPassword implements RestModifyView<AccountResource, Input> {
String newPassword;
if (input.generate) {
if (self.get() != rsrc.getUser()
&& !self.get().getCapabilities().canGenerateHttpPassword()) {
&& !self.get().getCapabilities().canAdministrateServer()) {
throw new AuthException("not allowed to generate HTTP password");
}
newPassword = generate();
} else if (input.httpPassword == null) {
if (self.get() != rsrc.getUser()
&& !self.get().getCapabilities().canGenerateHttpPassword()) {
&& !self.get().getCapabilities().canAdministrateServer()) {
throw new AuthException("not allowed to clear HTTP password");
}
newPassword = null;
} else {
if (!self.get().getCapabilities().canGenerateHttpPassword()) {
if (!self.get().getCapabilities().canAdministrateServer()) {
throw new AuthException("not allowed to set HTTP password directly, "
+ "requires the Generate HTTP Password permission");
}

1
gerrit-server/src/main/java/com/google/gerrit/server/config/CapabilityConstants.java
View File

@ -29,7 +29,6 @@ public class CapabilityConstants extends TranslationBundle {
public String createProject;
public String emailReviewers;
public String flushCaches;
public String generateHttpPassword;
public String killTask;
public String modifyAccount;
public String priority;

1
gerrit-server/src/main/resources/com/google/gerrit/server/config/CapabilityConstants.properties
View File

@ -5,7 +5,6 @@ createGroup = Create Group
createProject = Create Project
emailReviewers = Email Reviewers
flushCaches = Flush Caches
generateHttpPassword = Generate HTTP Password
killTask = Kill Task
modifyAccount = Modify Account
priority = Priority

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(