auto-merge-on-push: Check perms on all changes in submit set When submitting multiple changes through a receive-pack with the submit option, check that the user has the UPDATE_BY_SUBMIT for every single change's project/ref included in the submission change set. Prior to this change, the check happened only on the tip of the change pushed for submission, but not on all other changes, which exposed a critical security vulnerability, where any change on any project that was visible to the calling user were allowed to be submitted without having the necessary permission to perform the action. One sample scenario of the above is the submission with a topic when the change.submitWholeTopic is set to true. Bug: Issue 486131256 Release-Notes: Enforce the UPDATE_BY_SUBMIT permission check on every change to be submitted as part of the push to the magic branch with the submit option Change-Id: I514b22e75194bcd92d1b5c9b9eb3ce70e57fb5f2
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 0a296cd..73b680b 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -184,6 +184,7 @@ import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gerrit.server.restapi.change.ReplyAttentionSetUpdates; +import com.google.gerrit.server.submit.ChangeSet; import com.google.gerrit.server.submit.MergeOp; import com.google.gerrit.server.submit.MergeOpRepoManager; import com.google.gerrit.server.update.BatchUpdate; @@ -2372,12 +2373,9 @@ return; } - if (magicBranch.submit) { - err = checkRefPermission(magicBranch.perm, RefPermission.UPDATE_BY_SUBMIT); - if (err.isPresent()) { - rejectProhibited(cmd, err.get()); - return; - } + if (magicBranch.submit + && checkCanUpdateBySubmit(magicBranch.cmd, magicBranch.perm).isPresent()) { + return; } RevCommit tip; @@ -3248,11 +3246,40 @@ submitInput.notifyDetails.put( RecipientType.BCC, new NotifyInfo(magicBranch.notifyBcc.stream().map(Object::toString).collect(toList()))); - op.merge(tipChange, user, false, submitInput, false); + op.merge( + tipChange, + user, + false, + submitInput, + false, + this::checkUpdateChangesBySubmitPermissions); } } } + private Optional<AuthException> checkUpdateChangesBySubmitPermissions( + ChangeSet changeSetForSubmission) throws PermissionBackendException { + PermissionBackend.WithUser withUserPerm = permissionBackend.user(user); + for (BranchNameKey changeBranch : changeSetForSubmission.changesByBranch().keys()) { + Optional<AuthException> err = + checkCanUpdateBySubmit( + magicBranch.cmd, + withUserPerm.project(changeBranch.project()).ref(changeBranch.branch())); + if (err.isPresent()) { + return err; + } + } + return Optional.empty(); + } + + private Optional<AuthException> checkCanUpdateBySubmit( + ReceiveCommand cmd, PermissionBackend.ForRef forChangeDest) + throws PermissionBackendException { + Optional<AuthException> err = checkRefPermission(forChangeDest, RefPermission.UPDATE_BY_SUBMIT); + err.ifPresent(e -> rejectProhibited(cmd, e)); + return err; + } + private void preparePatchSetsForReplace( RevWalk globalRevWalk, ImmutableList<CreateRequest> newChanges) { try (TraceTimer traceTimer =
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index 974a1f7..82efa23 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -285,6 +285,11 @@ } } + @FunctionalInterface + public interface PreMergeChecker { + Optional<AuthException> check(ChangeSet changeSet) throws PermissionBackendException; + } + private final ChangeMessagesUtil cmUtil; private final BatchUpdate.Factory batchUpdateFactory; private final BatchUpdates batchUpdates; @@ -689,18 +694,60 @@ * @throws RestApiException if an error occurred. * @throws PermissionBackendException if permissions can't be checked * @throws IOException an error occurred reading from NoteDb. - * @return the merged change + * @return the merged change or null if the change failed the preMergeChecker */ // TODO: dryrun was introduced in https://gerrit-review.git.corp.google.com/c/gerrit/+/82911 and // has never been used. Consider removing it. Since it was never used and this file has been // through many refactorings since, it's likely that the implementation is broken. @CanIgnoreReturnValue + @Nullable public Change merge( Change change, IdentifiedUser caller, boolean checkSubmitRules, SubmitInput submitInput, boolean dryrun) + throws ConfigInvalidException, + PermissionBackendException, + UpdateException, + IOException, + RestApiException { + return merge(change, caller, checkSubmitRules, submitInput, dryrun, (cs) -> Optional.empty()); + } + + /** + * Merges the given change. + * + * <p>Depending on the server configuration, more changes may be affected, e.g. by submission of a + * topic or via superproject subscriptions. All affected changes are integrated using the projects + * integration strategy. + * + * @param change the change to be merged. + * @param caller the identity of the user that is recorded as the one performing the merge. In + * case of impersonation {@code caller.getRealUser()} contains the user triggering the merge. + * @param checkSubmitRules whether submit rules and submit requirements should be evaluated. + * @param submitInput parameters regarding the merge + * @param dryrun if true, this includes calculating all projects affected by the submission, + * checking for possible submission problems (ACLs, merge conflicts, etc) but not the merge + * itself. + * @param preMergeChecker function for checking if the merge should continue or not + * @throws RestApiException if an error occurred. + * @throws PermissionBackendException if permissions can't be checked + * @throws IOException an error occurred reading from NoteDb. + * @return the merged change or null if the change failed the preMergeChecker + */ + // TODO: dryrun was introduced in https://gerrit-review.git.corp.google.com/c/gerrit/+/82911 and + // has never been used. Consider removing it. Since it was never used and this file has been + // through many refactorings since, it's likely that the implementation is broken. + @CanIgnoreReturnValue + @Nullable + public Change merge( + Change change, + IdentifiedUser caller, + boolean checkSubmitRules, + SubmitInput submitInput, + boolean dryrun, + PreMergeChecker preMergeChecker) throws RestApiException, UpdateException, IOException, @@ -725,6 +772,10 @@ ChangeSet indexBackedChangeSet = completeMergeChangeSetWithRetry(change); + if (preMergeChecker.check(indexBackedChangeSet).isPresent()) { + return null; + } + if (indexBackedChangeSet.furtherHiddenChanges()) { throw new AuthException( "A change to be submitted with " + change.getId() + " is not visible");
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index ba2a6a3..0eff580 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -194,6 +194,7 @@ import com.google.gerrit.server.update.context.RefUpdateContext; import com.google.gerrit.server.util.AccountTemplateUtil; import com.google.gerrit.server.util.CommitMessageUtil; +import com.google.gerrit.server.util.MagicBranch; import com.google.gerrit.server.util.time.TimeUtil; import com.google.gerrit.testing.FakeEmailSender.Message; import com.google.inject.AbstractModule; @@ -4537,6 +4538,68 @@ } @Test + @GerritConfig(name = "change.submitWholeTopic", value = "true") + public void submitTopicFailsWhenUpdateBySubmitMissingOnAnyBranch() throws Exception { + requestScopeOperations.setApiUser(admin.id()); + String allowedSubmitBranch = "allowed-submit-branch"; + String restrictedBranch = "restricted-branch"; + + Project.NameKey restrictedProject = createProjectOverAPI("restricted", allProjects, true, null); + createBranch(BranchNameKey.create(restrictedProject, restrictedBranch)); + + try (TestRepository<InMemoryRepository> testRestrictedRepo = + cloneProject(restrictedProject, admin)) { + createBranch(BranchNameKey.create(project, allowedSubmitBranch)); + AccountGroup.UUID submitGroup = groupOperations.newGroup().create(); + gApi.groups().id(submitGroup.get()).addMembers(admin.username()); + String topic = "submit-topic"; + + projectOperations + .project(allProjects) + .forUpdate() + .add(block(Permission.SUBMIT).ref(RefNames.REFS_HEADS).group(submitGroup)); + projectOperations + .project(restrictedProject) + .forUpdate() + .add( + block(Permission.SUBMIT) + .ref(MagicBranch.NEW_CHANGE + RefNames.fullName(restrictedBranch)) + .group(submitGroup)); + projectOperations + .project(project) + .forUpdate() + .add( + allow(Permission.SUBMIT) + .ref(MagicBranch.NEW_CHANGE + RefNames.fullName(allowedSubmitBranch)) + .group(submitGroup)) + .update(); + + PushOneCommit push1 = + pushFactory.create( + admin.newIdent(), testRestrictedRepo, "Subject", "file.txt", "content"); + PushOneCommit.Result push1Res = + push1.to(MagicBranch.NEW_CHANGE + restrictedBranch + "%topic=" + topic); + push1Res.assertOkStatus(); + + PushOneCommit push2 = + pushFactory.create(admin.newIdent(), testRepo, "Subject", "file.txt", "content"); + PushOneCommit.Result push2Res = + push2.to(MagicBranch.NEW_CHANGE + allowedSubmitBranch + "%topic=" + topic + ",submit"); + push2Res.assertErrorStatus( + String.format( + "prohibited by Gerrit: not permitted: update by submit on %s", + RefNames.fullName(restrictedBranch))); + + assertChangeNotSubmitted(push1Res.getChangeId()); + assertChangeNotSubmitted(push2Res.getChangeId()); + } + } + + private void assertChangeNotSubmitted(String changeId) throws RestApiException { + assertThat(gApi.changes().id(changeId).get().status).isEqualTo(ChangeStatus.NEW); + } + + @Test public void cantSubmitWithInvisibleDependentChange() throws Exception { // create two dependent changes. PushOneCommit.Result r1 = createChange();