From 2ed0b44e01efdef4090099484f3b126d47fa4e82 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 13 Nov 2020 00:12:38 +0000 Subject: 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 --- .../gerrit/server/permissions/ProjectControl.java | 8 ++++- .../gerrit/server/permissions/RefControlTest.java | 41 ++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java index 52478a4d5e..c0fbf364de 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.conditions.BooleanCondition; 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 @@ class ProjectControl { private final ChangeControl.Factory changeControlFactory; private final PermissionCollection.Factory permissionFilter; private final DefaultRefFilter.Factory refFilterFactory; + private final AllUsersName allUsersName; private List allSections; private Map refControls; @@ -91,6 +93,7 @@ class ProjectControl { RefVisibilityControl refVisibilityControl, GitRepositoryManager repositoryManager, DefaultRefFilter.Factory refFilterFactory, + AllUsersName allUsersName, @Assisted CurrentUser who, @Assisted ProjectState ps) { this.changeControlFactory = changeControlFactory; @@ -101,6 +104,7 @@ class ProjectControl { this.refVisibilityControl = refVisibilityControl; this.repositoryManager = repositoryManager; this.refFilterFactory = refFilterFactory; + this.allUsersName = allUsersName; user = who; state = ps; } @@ -176,7 +180,9 @@ class ProjectControl { } boolean allRefsAreVisible(Set 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 7f8f2592a4..6a1c037acf 100644 --- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java +++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java @@ -47,6 +47,7 @@ import com.google.gerrit.server.CurrentUser; 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; @@ -62,6 +63,7 @@ import com.google.inject.Guice; 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; @@ -89,6 +91,18 @@ public class RefControlTest { 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(); } @@ -180,6 +194,7 @@ public class RefControlTest { 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; @@ -270,6 +285,32 @@ public class RefControlTest { assertAdminsAreOwnersAndDevsAreNot(); } + @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 -- cgit v1.2.3