Shortcut CreateRefControl#checkCreateCommit only for push processing

The shortcut is valid when processing pushes, as the connectivity
check in JGit (class ConnectivityChecker) ensures that the client can
only push objects that point to objects they have access to.

The CreateBranch REST API does not do a connectivity check, so we have
to verify visiblity of the new commit even if the user has UPDATE
access.

Add a test.

This fixes a data leak, where users with both UPDATE and CREATE_REF
permission could obtain read access to commits by creating a branch
pointing at the secret commit.

Bug: Issue 16054
Release-Notes: fixes confidentiality leak in create branch API
Change-Id: Icc16c2dc4d1356b9538fdc7f98b4265e32cd5e8a
Co-authored-by: Han-Wen Nienhuys <hanwen@google.com>

5 files changed, 130 insertions, 12 deletions

diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 55ff26093f..e61c938067 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -1349,7 +1349,7 @@ class ReceiveCommits {
         // Must pass explicit user instead of injecting a provider into CreateRefControl, since
         // Provider<CurrentUser> within ReceiveCommits will always return anonymous.
         createRefControl.checkCreateRef(
-            Providers.of(user), receivePack.getRepository(), branch, obj);
+            Providers.of(user), receivePack.getRepository(), branch, obj, /* forPush= */ true);
       } catch (AuthException denied) {
         rejectProhibited(cmd, denied);
         return;
diff --git a/java/com/google/gerrit/server/project/CreateRefControl.java b/java/com/google/gerrit/server/project/CreateRefControl.java
index 2e76949942..54ab628ccb 100644
--- a/java/com/google/gerrit/server/project/CreateRefControl.java
+++ b/java/com/google/gerrit/server/project/CreateRefControl.java
@@ -25,10 +25,13 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.permissions.RefPermission;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.update.RetryHelper;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
+import java.util.List;
 import java.util.Optional;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.PersonIdent;
@@ -41,18 +44,24 @@ import org.eclipse.jgit.revwalk.RevWalk;
 /** Manages access control for creating Git references (aka branches, tags). */
 @Singleton
 public class CreateRefControl {
+
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   private final PermissionBackend permissionBackend;
   private final ProjectCache projectCache;
   private final Reachable reachable;
+  private final RetryHelper retryHelper;
 
   @Inject
   CreateRefControl(
-      PermissionBackend permissionBackend, ProjectCache projectCache, Reachable reachable) {
+      PermissionBackend permissionBackend,
+      ProjectCache projectCache,
+      Reachable reachable,
+      RetryHelper retryHelper) {
     this.permissionBackend = permissionBackend;
     this.projectCache = projectCache;
     this.reachable = reachable;
+    this.retryHelper = retryHelper;
   }
 
   /**
@@ -67,7 +76,11 @@ public class CreateRefControl {
    * @throws ResourceConflictException if the project state does not permit the operation
    */
   public void checkCreateRef(
-      Provider<? extends CurrentUser> user, Repository repo, BranchNameKey branch, RevObject object)
+      Provider<? extends CurrentUser> user,
+      Repository repo,
+      BranchNameKey branch,
+      RevObject object,
+      boolean forPush)
       throws AuthException, PermissionBackendException, NoSuchProjectException, IOException,
           ResourceConflictException {
     ProjectState ps =
@@ -77,7 +90,7 @@ public class CreateRefControl {
     PermissionBackend.ForRef perm = permissionBackend.user(user.get()).ref(branch);
     if (object instanceof RevCommit) {
       perm.check(RefPermission.CREATE);
-      checkCreateCommit(user, repo, (RevCommit) object, ps.getNameKey(), perm);
+      checkCreateCommit(user, repo, (RevCommit) object, ps.getNameKey(), perm, forPush);
     } else if (object instanceof RevTag) {
       RevTag tag = (RevTag) object;
       try (RevWalk rw = new RevWalk(repo)) {
@@ -97,9 +110,9 @@ public class CreateRefControl {
 
       RevObject target = tag.getObject();
       if (target instanceof RevCommit) {
-        checkCreateCommit(user, repo, (RevCommit) target, ps.getNameKey(), perm);
+        checkCreateCommit(user, repo, (RevCommit) target, ps.getNameKey(), perm, forPush);
       } else {
-        checkCreateRef(user, repo, branch, target);
+        checkCreateRef(user, repo, branch, target, forPush);
       }
 
       // If the tag has a PGP signature, allow a lower level of permission
@@ -122,13 +135,31 @@ public class CreateRefControl {
       Repository repo,
       RevCommit commit,
       Project.NameKey project,
-      PermissionBackend.ForRef forRef)
+      PermissionBackend.ForRef forRef,
+      boolean forPush)
       throws AuthException, PermissionBackendException, IOException {
     try {
-      // If the user has update (push) permission, they can create the ref regardless
-      // of whether they are pushing any new objects along with the create.
-      forRef.check(RefPermission.UPDATE);
-      return;
+      // If the user has UPDATE (push) permission, they can set the ref to an arbitrary commit:
+      //
+      //  * if they don't have access, we don't advertise the data, and a conforming git client
+      //  would send the object along with the push as outcome of the negotation.
+      //  * a malicious client could try to send the update without sending the object. This
+      //  is prevented by JGit's ConnectivityChecker (see receive.checkReferencedObjectsAreReachable
+      //  to switch off this costly check).
+      //
+      // Thus, when using the git command-line client, we don't need to do extra checks for users
+      // with push access.
+      //
+      // When using the REST API, there is no negotiation, and the target commit must already be on
+      // the server, so we must check that the user can see that commit.
+      if (forPush) {
+        // We can only shortcut for UPDATE permission. Pushing a tag (CREATE_TAG, CREATE_SIGNED_TAG)
+        // can also introduce new objects. While there may not be a confidentiality problem
+        // (the caller supplies the data as documented above), the permission is for creating
+        // tags to existing commits.
+        forRef.check(RefPermission.UPDATE);
+        return;
+      }
     } catch (AuthException denied) {
       // Fall through to check reachability.
     }
@@ -145,6 +176,18 @@ public class CreateRefControl {
       return;
     }
 
+    // Previous check only catches normal branches. Try PatchSet refs too. If we can create refs,
+    // we're not a replica, so we can always use the change index.
+    List<ChangeData> changes =
+        retryHelper
+            .changeIndexQuery(
+                "queryChangesByProjectCommitWithLimit1",
+                q -> q.enforceVisibility(true).setLimit(1).byProjectCommit(project, commit))
+            .call();
+    if (!changes.isEmpty()) {
+      return;
+    }
+
     AuthException e =
         new AuthException(
             String.format(
diff --git a/java/com/google/gerrit/server/restapi/project/CreateBranch.java b/java/com/google/gerrit/server/restapi/project/CreateBranch.java
index 2fd2d6530f..ba3a5be21a 100644
--- a/java/com/google/gerrit/server/restapi/project/CreateBranch.java
+++ b/java/com/google/gerrit/server/restapi/project/CreateBranch.java
@@ -129,7 +129,7 @@ public class CreateBranch
         object = rw.parseCommit(object);
       }
 
-      createRefControl.checkCreateRef(identifiedUser, repo, name, object);
+      createRefControl.checkCreateRef(identifiedUser, repo, name, object, /* forPush= */ false);
 
       RefUpdate u = repo.updateRef(ref);
       u.setExpectedOldObjectId(ObjectId.zeroId());
diff --git a/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java
index 385780b2d5..7567c42101 100644
--- a/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java
@@ -47,6 +47,7 @@ import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.transport.PushResult;
 import org.eclipse.jgit.transport.RefSpec;
+import org.eclipse.jgit.transport.RemoteRefUpdate;
 import org.eclipse.jgit.transport.RemoteRefUpdate.Status;
 import org.eclipse.jgit.transport.TrackingRefUpdate;
 import org.junit.Before;
@@ -94,6 +95,32 @@ public class PushPermissionsIT extends AbstractDaemonTest {
   }
 
   @Test
+  public void pushNewCommitsRequiresPushPermission() throws Exception {
+    testRepo.branch("HEAD").commit().create();
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(allow(Permission.CREATE).ref("refs/*").group(REGISTERED_USERS))
+        .update();
+
+    PushResult r = push("HEAD:refs/heads/newbranch");
+
+    String msg = "update for creating new commit object not permitted";
+    RemoteRefUpdate rru = r.getRemoteUpdate("refs/heads/newbranch");
+    assertThat(rru.getStatus()).isNotEqualTo(Status.OK);
+    assertThat(rru.getMessage()).contains(msg);
+
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(allow(Permission.PUSH).ref("refs/*").group(REGISTERED_USERS))
+        .update();
+    RemoteRefUpdate success =
+        push("HEAD:refs/heads/newbranch").getRemoteUpdate("refs/heads/newbranch");
+    assertThat(success.getStatus()).isEqualTo(Status.OK);
+  }
+
+  @Test
   public void fastForwardUpdateDenied() throws Exception {
     testRepo.branch("HEAD").commit().create();
     PushResult r = push("HEAD:refs/heads/master");
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java
index 93ce255f62..33a7dc51cd 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java
@@ -26,7 +26,10 @@ import static com.google.gerrit.testing.GerritJUnit.assertThrows;
 import com.google.common.collect.ImmutableList;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.ExtensionRegistry;
+import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
 import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
 import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
 import com.google.gerrit.entities.Account;
@@ -61,6 +64,7 @@ import org.junit.Test;
 public class CreateBranchIT extends AbstractDaemonTest {
   @Inject private ProjectOperations projectOperations;
   @Inject private RequestScopeOperations requestScopeOperations;
+  @Inject private GroupOperations groupOperations;
   @Inject private ExtensionRegistry extensionRegistry;
 
   private BranchNameKey testBranch;
@@ -410,6 +414,50 @@ public class CreateBranchIT extends AbstractDaemonTest {
     assertThat(ex).hasMessageThat().isEqualTo("ref must match URL");
   }
 
+  @Test
+  public void createBranchRevisionVisibility() throws Exception {
+    AccountGroup.UUID privilegedGroupUuid =
+        groupOperations.newGroup().name(name("privilegedGroup")).create();
+    TestAccount privilegedUser =
+        accountCreator.create(
+            "privilegedUser", "privilegedUser@example.com", "privilegedUser", null);
+    groupOperations.group(privilegedGroupUuid).forUpdate().addMember(privilegedUser.id()).update();
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(block(Permission.READ).ref("refs/heads/secret/*").group(REGISTERED_USERS))
+        .add(allow(Permission.READ).ref("refs/heads/secret/*").group(privilegedGroupUuid))
+        .add(allow(Permission.READ).ref("refs/heads/*").group(REGISTERED_USERS))
+        .add(allow(Permission.CREATE).ref("refs/heads/*").group(REGISTERED_USERS))
+        .add(allow(Permission.PUSH).ref("refs/heads/*").group(REGISTERED_USERS))
+        .update();
+    PushOneCommit push =
+        pushFactory.create(admin.newIdent(), testRepo, "Configure", "file.txt", "contents");
+    PushOneCommit.Result result = push.to("refs/heads/secret/main");
+    result.assertOkStatus();
+    RevCommit secretCommit = result.getCommit();
+    requestScopeOperations.setApiUser(privilegedUser.id());
+    BranchInfo info = gApi.projects().name(project.get()).branch("refs/heads/secret/main").get();
+    assertThat(info.revision).isEqualTo(secretCommit.name());
+    TestAccount unprivileged =
+        accountCreator.create("unprivileged", "unprivileged@example.com", "unprivileged", null);
+    requestScopeOperations.setApiUser(unprivileged.id());
+    assertThrows(
+        ResourceNotFoundException.class,
+        () -> gApi.projects().name(project.get()).branch("refs/heads/secret/main").get());
+    BranchInput branchInput = new BranchInput();
+    branchInput.ref = "public";
+    branchInput.revision = secretCommit.name();
+    assertThrows(
+        AuthException.class,
+        () -> gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput));
+
+    branchInput.revision = "refs/heads/secret/main";
+    assertThrows(
+        AuthException.class,
+        () -> gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput));
+  }
+
   private void blockCreateReference() throws Exception {
     projectOperations
         .project(project)