Disable administrative permissions during X-Gerrit-RunAs
When executing an action on behalf of an administrator, disable the
administrateServer capability during the request. This may limit
the damage a compromised role account could cause by avoiding any
permissions that are not explicitly granted.
Change-Id: I263e1d8e1a645617842f11b7712f79f5c009c6ca
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index b016087..fab71dd 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -1271,10 +1271,21 @@
Run As
~~~~~~
-Allow users to impersonate any other user with the X-Gerrit-RunAs HTTP
-header on REST API calls or the link:cmd-suexec.html[suexec] SSH
-command. Site administrators do not inherit this capability; it must
-be granted explicitly.
+Allow users to impersonate any other user with the `X-Gerrit-RunAs`
+HTTP header on REST API calls, or the link:cmd-suexec.html[suexec]
+SSH command.
+
+When impersonating an administrator the Administrate Server capability
+is not honored. This security feature tries to prevent a role with
+Run As capability from modifying the access controls in All-Projects,
+however modification may still be possible if the impersonated user
+has permission to push or submit changes on `refs/meta/config`. Run
+As also blocks using most capabilities including Create User, Run
+Garbage Collection, etc., unless the capability is also explicitly
+granted to a group the administrator is a member of.
+
+Administrators do not automatically inherit this capability; it must
+be explicitly granted.
[[capability_runGC]]
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java
index 80144c4..9968e2a 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java
@@ -185,7 +185,7 @@
public void setUserAccountId(Account.Id id) {
key = new Key("id:" + id);
val = new Val(id, 0, false, null, 0, null, null);
- user = null;
+ user = identified.runAs(id, user);
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java
index 86a6ef8..ae8c6bf 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java
@@ -51,6 +51,20 @@
}
/**
+ * Identity of the authenticated user.
+ * <p>
+ * In the normal case where a user authenticates as themselves
+ * {@code getRealUser() == this}.
+ * <p>
+ * If {@code X-Gerrit-RunAs} or {@code suexec} was used this method returns
+ * the identity of the account that has permission to act on behalf of this
+ * user.
+ */
+ public CurrentUser getRealUser() {
+ return this;
+ }
+
+ /**
* Get the set of groups the user is currently a member of.
* <p>
* The returned set may be a subset of the user's actual groups; if the user's
@@ -76,11 +90,9 @@
/** Capabilities available to this user account. */
public CapabilityControl getCapabilities() {
- CapabilityControl ctl = capabilities;
- if (ctl == null) {
- ctl = capabilityControlFactory.create(this);
- capabilities = ctl;
+ if (capabilities == null) {
+ capabilities = capabilityControlFactory.create(this);
}
- return ctl;
+ return capabilities;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java
index 3826293..8e61c9a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java
@@ -97,13 +97,20 @@
public IdentifiedUser create(Provider<ReviewDb> db, Account.Id id) {
return new IdentifiedUser(capabilityControlFactory,
authConfig, anonymousCowardName, canonicalUrl, realm, accountCache,
- groupBackend, null, db, id);
+ groupBackend, null, db, id, null);
}
public IdentifiedUser create(SocketAddress remotePeer, Account.Id id) {
return new IdentifiedUser(capabilityControlFactory,
authConfig, anonymousCowardName, canonicalUrl, realm, accountCache,
- groupBackend, Providers.of(remotePeer), null, id);
+ groupBackend, Providers.of(remotePeer), null, id, null);
+ }
+
+ public CurrentUser runAs(SocketAddress remotePeer, Account.Id id,
+ @Nullable CurrentUser caller) {
+ return new IdentifiedUser(capabilityControlFactory,
+ authConfig, anonymousCowardName, canonicalUrl, realm, accountCache,
+ groupBackend, Providers.of(remotePeer), null, id, caller);
}
}
@@ -152,7 +159,13 @@
public IdentifiedUser create(Account.Id id) {
return new IdentifiedUser(capabilityControlFactory,
authConfig, anonymousCowardName, canonicalUrl, realm, accountCache,
- groupBackend, remotePeerProvider, dbProvider, id);
+ groupBackend, remotePeerProvider, dbProvider, id, null);
+ }
+
+ public IdentifiedUser runAs(Account.Id id, CurrentUser caller) {
+ return new IdentifiedUser(capabilityControlFactory,
+ authConfig, anonymousCowardName, canonicalUrl, realm, accountCache,
+ groupBackend, remotePeerProvider, dbProvider, id, caller);
}
}
@@ -183,6 +196,7 @@
private GroupMembership effectiveGroups;
private Set<Change.Id> starredChanges;
private Collection<AccountProjectWatch> notificationFilters;
+ private CurrentUser realUser;
private IdentifiedUser(
CapabilityControl.Factory capabilityControlFactory,
@@ -192,7 +206,9 @@
final Realm realm, final AccountCache accountCache,
final GroupBackend groupBackend,
@Nullable final Provider<SocketAddress> remotePeerProvider,
- @Nullable final Provider<ReviewDb> dbProvider, final Account.Id id) {
+ @Nullable final Provider<ReviewDb> dbProvider,
+ final Account.Id id,
+ @Nullable CurrentUser realUser) {
super(capabilityControlFactory);
this.canonicalUrl = canonicalUrl;
this.accountCache = accountCache;
@@ -202,6 +218,12 @@
this.remotePeerProvider = remotePeerProvider;
this.dbProvider = dbProvider;
this.accountId = id;
+ this.realUser = realUser != null ? realUser : this;
+ }
+
+ @Override
+ public CurrentUser getRealUser() {
+ return realUser;
}
// TODO(cranger): maybe get the state through the accountCache instead.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java
index 4c5bd97..193ea04 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java
@@ -65,8 +65,12 @@
/** @return true if the user can administer this server. */
public boolean canAdministrateServer() {
if (canAdministrateServer == null) {
- canAdministrateServer = user instanceof PeerDaemonUser
- || matchAny(capabilities.administrateServer, ALLOWED_RULE);
+ if (user.getRealUser() != user) {
+ canAdministrateServer = false;
+ } else {
+ canAdministrateServer = user instanceof PeerDaemonUser
+ || matchAny(capabilities.administrateServer, ALLOWED_RULE);
+ }
}
return canAdministrateServer;
}
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SuExec.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SuExec.java
index 54bb84c..298a099 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SuExec.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SuExec.java
@@ -126,8 +126,12 @@
} else {
peer = peerAddress;
}
+ CurrentUser self = caller.get();
+ if (self instanceof PeerDaemonUser) {
+ self = null;
+ }
return new SshSession(session.get(), peer,
- userFactory.create(peer, accountId));
+ userFactory.runAs(peer, accountId, self));
}
private static String join(List<String> args) {
