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
This commit is contained in:
Luca Milanesio
2020-11-13 00:12:38 +00:00
parent 988882e0ad
commit 45071d6977
2 changed files with 48 additions and 1 deletions

View File

@@ -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<SectionMatcher> allSections;
private Map<String, RefControl> 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<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? */