diff options
author | Luca Milanesio <luca.milanesio@gmail.com> | 2020-11-13 00:12:38 +0000 |
---|---|---|
committer | Luca Milanesio <luca.milanesio@gmail.com> | 2020-11-13 19:23:53 +0000 |
commit | 3dc150c8ecf7bb31948e1f8bc3b7c3776a3857a6 (patch) | |
tree | 9ab68f4fdf62b29ff79901bef664bcc1ebeb2f03 | |
parent | ab96c734407ca60c2e70ee7208f6977be82e2bc6 (diff) |
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
(cherry picked from commit 45071d6977932bca5a1427c8abad24710fed2e33)
-rw-r--r-- | java/com/google/gerrit/server/permissions/ProjectControl.java | 8 | ||||
-rw-r--r-- | javatests/com/google/gerrit/server/permissions/RefControlTest.java | 39 |
2 files changed, 43 insertions, 4 deletions
diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java index c3d0719283..85e7fb16b4 100644 --- a/java/com/google/gerrit/server/permissions/ProjectControl.java +++ b/java/com/google/gerrit/server/permissions/ProjectControl.java @@ -33,6 +33,7 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; 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; @@ -75,6 +76,7 @@ class ProjectControl { 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; @@ -90,6 +92,7 @@ class ProjectControl { RefVisibilityControl refVisibilityControl, GitRepositoryManager repositoryManager, DefaultRefFilter.Factory refFilterFactory, + AllUsersName allUsersName, @Assisted CurrentUser who, @Assisted ProjectState ps) { this.changeControlFactory = changeControlFactory; @@ -100,6 +103,7 @@ class ProjectControl { this.refVisibilityControl = refVisibilityControl; this.repositoryManager = repositoryManager; this.refFilterFactory = refFilterFactory; + this.allUsersName = allUsersName; user = who; state = ps; } @@ -175,7 +179,9 @@ class ProjectControl { } 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 bf8ea86f02..0f0f1c3ec1 100644 --- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java +++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java @@ -106,6 +106,14 @@ public class RefControlTest { assertThat(u.isOwner()).named("not owner").isFalse(); } + private void assertAllRefsAreVisible(ProjectControl u) { + assertThat(u.allRefsAreVisible(Collections.emptySet())).named("all refs visible").isTrue(); + } + + private void assertAllRefsAreNotVisible(ProjectControl u) { + assertThat(u.allRefsAreVisible(Collections.emptySet())).named("all refs NOT visible").isFalse(); + } + private void assertNotOwner(String ref, ProjectControl u) { assertThat(u.controlForRef(ref).isOwner()).named("NOT OWN " + ref).isFalse(); } @@ -195,6 +203,7 @@ public class RefControlTest { private final Map<Project.NameKey, ProjectState> all = new HashMap<>(); private Project.NameKey localKey = new Project.NameKey("local"); private ProjectConfig local; + private ProjectConfig allUsers; private Project.NameKey parentKey = new Project.NameKey("parent"); private ProjectConfig parent; private InMemoryRepositoryManager repoManager; @@ -226,7 +235,7 @@ public class RefControlTest { @Override public ProjectState getAllUsers() { - return null; + return get(allUsersName); } @Override @@ -280,12 +289,17 @@ public class RefControlTest { injector.injectMembers(this); try { - Repository repo = repoManager.createRepository(allProjectsName); + Repository allProjectsRepo = repoManager.createRepository(allProjectsName); ProjectConfig allProjects = new ProjectConfig(new Project.NameKey(allProjectsName.get())); - allProjects.load(repo); + allProjects.load(allProjectsRepo); LabelType cr = Util.codeReview(); allProjects.getLabelSections().put(cr.getName(), cr); add(allProjects); + + Repository allUsersRepo = repoManager.createRepository(allUsersName); + allUsers = new ProjectConfig(new Project.NameKey(allUsersName.get())); + allUsers.load(allUsersRepo); + add(allUsers); } catch (IOException | ConfigInvalidException e) { throw new RuntimeException(e); } @@ -360,6 +374,24 @@ public class RefControlTest { } @Test + public void allRefsAreVisibleForRegularProject() throws Exception { + allow(local, READ, DEVS, "refs/*"); + allow(local, READ, DEVS, "refs/groups/*"); + allow(local, READ, DEVS, "refs/users/default"); + + assertAllRefsAreVisible(user(local, DEVS)); + } + + @Test + public void allRefsAreNotVisibleForAllUsers() throws Exception { + allow(allUsers, READ, DEVS, "refs/*"); + allow(allUsers, READ, DEVS, "refs/groups/*"); + allow(allUsers, READ, DEVS, "refs/users/default"); + + assertAllRefsAreNotVisible(user(allUsers, DEVS)); + } + + @Test public void branchDelegation1() throws Exception { allow(local, OWNER, ADMIN, "refs/*"); allow(local, OWNER, DEVS, "refs/heads/x/*"); @@ -1022,6 +1054,7 @@ public class RefControlTest { refVisibilityControl, repoManager, refFilterFactory, + allUsersName, new MockUser(name, memberOf), newProjectState(local)); } |