Workaround Gitiles bug on All-Users visibility
Gitiles has special FilteredRepository wrapper that
allows to carefully hide refs based on the project's ACLs.
There is however an optimisation that skips the filtering
in case a user has READ permissions on every ACLs patterns.
When the target repository is All-Users, the optimisation
turns into a security issue because it allows seeing everything
that belongs to everyone:
- draft comments
- PII of all users
- external ids
- draft edits
Block Gitiles or any other part of Gerrit to abuse of this
power when the target repository is All-Users, where nobody
can be authorised to skip the ACLs evaluation.
Cover the additional special case of the All-Users project
access with two explicit positive and negative tests,
so that the security check is covered.
Bug: Issue 13621
Change-Id: Ia6ea1a9fd5473adff534204aea7d8f25324a45b7
diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java
index 82fce53..edffcc6 100644
--- a/java/com/google/gerrit/server/permissions/ProjectControl.java
+++ b/java/com/google/gerrit/server/permissions/ProjectControl.java
@@ -36,6 +36,7 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.GroupMembership;
+import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.GitReceivePackGroups;
import com.google.gerrit.server.config.GitUploadPackGroups;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -76,6 +77,7 @@
private final ChangeControl.Factory changeControlFactory;
private final PermissionCollection.Factory permissionFilter;
private final DefaultRefFilter.Factory refFilterFactory;
+ private final AllUsersName allUsersName;
private List<SectionMatcher> allSections;
private Map<String, RefControl> refControls;
@@ -91,6 +93,7 @@
RefVisibilityControl refVisibilityControl,
GitRepositoryManager repositoryManager,
DefaultRefFilter.Factory refFilterFactory,
+ AllUsersName allUsersName,
@Assisted CurrentUser who,
@Assisted ProjectState ps) {
this.changeControlFactory = changeControlFactory;
@@ -101,6 +104,7 @@
this.refVisibilityControl = refVisibilityControl;
this.repositoryManager = repositoryManager;
this.refFilterFactory = refFilterFactory;
+ this.allUsersName = allUsersName;
user = who;
state = ps;
}
@@ -176,7 +180,9 @@
}
boolean allRefsAreVisible(Set<String> ignore) {
- return user.isInternalUser() || canPerformOnAllRefs(Permission.READ, ignore);
+ return user.isInternalUser()
+ || (!getProject().getNameKey().equals(allUsersName)
+ && canPerformOnAllRefs(Permission.READ, ignore));
}
/** Can the user run upload pack? */
diff --git a/javatests/com/google/gerrit/server/permissions/RefControlTest.java b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
index d4442d4..336b6fb 100644
--- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java
+++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
@@ -48,6 +48,7 @@
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.ListGroupMembership;
import com.google.gerrit.server.config.AllProjectsName;
+import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.index.SingleVersionModule.SingleVersionListener;
import com.google.gerrit.server.project.ProjectCache;
@@ -63,6 +64,7 @@
import com.google.inject.Inject;
import com.google.inject.Injector;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.Optional;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Repository;
@@ -90,6 +92,18 @@
assertWithMessage("not owner").that(u.isOwner()).isFalse();
}
+ private void assertAllRefsAreVisible(ProjectControl u) {
+ assertWithMessage("all refs visible")
+ .that(u.allRefsAreVisible(Collections.emptySet()))
+ .isTrue();
+ }
+
+ private void assertAllRefsAreNotVisible(ProjectControl u) {
+ assertWithMessage("all refs NOT visible")
+ .that(u.allRefsAreVisible(Collections.emptySet()))
+ .isFalse();
+ }
+
private void assertNotOwner(String ref, ProjectControl u) {
assertWithMessage("NOT OWN " + ref).that(u.controlForRef(ref).isOwner()).isFalse();
}
@@ -181,6 +195,7 @@
private final Project.NameKey parentKey = Project.nameKey("parent");
@Inject private AllProjectsName allProjectsName;
+ @Inject private AllUsersName allUsersName;
@Inject private InMemoryRepositoryManager repoManager;
@Inject private MetaDataUpdate.Server metaDataUpdateFactory;
@Inject private ProjectCache projectCache;
@@ -272,6 +287,32 @@
}
@Test
+ public void allRefsAreVisibleForRegularProject() throws Exception {
+ projectOperations
+ .project(localKey)
+ .forUpdate()
+ .add(allow(READ).ref("refs/*").group(DEVS))
+ .add(allow(READ).ref("refs/groups/*").group(DEVS))
+ .add(allow(READ).ref("refs/users/default").group(DEVS))
+ .update();
+
+ assertAllRefsAreVisible(user(localKey, DEVS));
+ }
+
+ @Test
+ public void allRefsAreNotVisibleForAllUsers() throws Exception {
+ projectOperations
+ .project(allUsersName)
+ .forUpdate()
+ .add(allow(READ).ref("refs/*").group(DEVS))
+ .add(allow(READ).ref("refs/groups/*").group(DEVS))
+ .add(allow(READ).ref("refs/users/default").group(DEVS))
+ .update();
+
+ assertAllRefsAreNotVisible(user(allUsersName, DEVS));
+ }
+
+ @Test
public void branchDelegation1() throws Exception {
projectOperations
.project(localKey)
