New issue
Advanced search Search tips

Issue 9823 link

Starred by 3 users

Issue metadata

Status: Submitted
Owner:
Closed: Oct 17
Cc:
Components:



Sign in to add a comment

Wrong access path on git-receive-pack

Project Member Reported by synton...@gmail.com, Oct 8

Issue description

*****************************************************************
*****                                                       *****
***** !!!! THIS BUG TRACKER IS FOR GERRIT CODE REVIEW !!!!  *****
*****                                                       *****
***** DO NOT SUBMIT BUGS FOR CHROME, ANDROID, CYANOGENMOD,  *****
***** INTERNAL ISSUES WITH YOUR COMPANY'S GERRIT SETUP, ETC.*****
*****                                                       *****
*****   THOSE ISSUES BELONG IN DIFFERENT ISSUE TRACKERS     *****
*****                                                       *****
*****************************************************************

Affected Version: 2.14.x

What steps will reproduce the problem?
1. Create a new project
2. Grant push permission to master branch to Administrators
3. Perform a force push to master over ssh


What is the expected output?

Force push is denied.

What do you see instead?

Force push is allowed.


Please provide any additional information below.

The fix performed by Shawn via 3c4a53edf6242389c97fa19feaa77e735f26f78e is not effective anymore because git over ssh command is now run asynchronously, causing the accessPath to stay as SSH_COMMAND and then the force push evaluated in the wrong way.  

 
Project Member

Comment 1 by luca.mil...@gmail.com, Oct 8

Cc: dborowitz@google.com
Tried with git/http and works as expected:

git push -f http://admin@localhost:8080/main HEAD:master                                                                 ✔  11508  12:56:33
Counting objects: 3, done.
Writing objects: 100% (3/3), 238 bytes | 0 bytes/s, done.
Total 3 (delta 0), reused 0 (delta 0)
remote: Processing changes: done
To http://admin@localhost:8080/main
 ! [rejected]        HEAD -> master (non-fast-forward)

This is really a Git/SSH related problem with all the async-thread delegation mechanism that was introduced to avoid Apache Mina/SSH to kill the thread in case of timeouts or connections broken.

This explains as well why we never noticed on gerrit-review.googlesource.com: we just don't use Git/SSH at all there !

Labels: Security
Presumably at step 3 the push is done by a user other than admin?

Labels: Restrict-View-SecurityIssue
Why isn't this issue caught by ForcePushIT, which as far as I can see is testing this exact use case with SSH? (the test is annotated @NoHttpd which means it uses ssh).
Project Member

Comment 6 by luca.mil...@gmail.com, Oct 8

This is the part of the code that allows users without force-push to perform a force push:

  /** @return true if the user can rewind (force push) the reference. */
  public boolean canForceUpdate() {
    if (!canWrite()) {
      return false;
    }

    if (canPushWithForce()) {
      return true;
    }

    switch (getUser().getAccessPath()) {
      case GIT:
        return false;

      case JSON_RPC:
      case REST_API:
      case SSH_COMMAND:
      case UNKNOWN:
      case WEB_BROWSER:
      default:
        return getUser().getCapabilities().canAdministrateServer()
            || (isOwner() && !isForceBlocked(Permission.PUSH));
    }
  }

When executing commands over Git/SSH, the access path is (wrongly) SSH_COMMAND and thus the permissions is evaluated as:
getUser().getCapabilities().canAdministrateServer() 
OR
isOwner() && !isForceBlocked(Permission.PUSH)

So, yes, if the user is a Project owner and has not been explicitly denied the force push, then it will be allowed as well.
Project Member

Comment 7 by luca.mil...@gmail.com, Oct 8

> Why isn't this issue caught by ForcePushIT, which as far as I can see is testing this exact use case with SSH? (the test is annotated @NoHttpd which means it uses ssh).

The ForcePushIT is not an E2E test because it uses an InMemoryRepository and doesn't go through the full Git/SSH protocol stack.

It then works on the test, but it doesn't E2E with a real Git/SSH protocol stack.
Project Member

Comment 8 by synton...@gmail.com, Oct 8

Status: ChangeUnderReview (was: New)
Project Member

Comment 9 by luca.mil...@gmail.com, Oct 8

> The fix performed by Shawn via 3c4a53edf6242389c97fa19feaa77e735f26f78e is not effective anymore because git over ssh command is now run asynchronously, causing the accessPath to stay as SSH_COMMAND and then the force push evaluated in the wrong way.

This isn't actually true: it was asynchronous as well before but the context was provided by the ThreadLocal and thus it was correctly retrieved.

Now the context gets Guice injected and thus needs to be propagated manually to the delegated thread so that can keep the correct access path.
Labels: -Restrict-View-SecurityIssue FixedIn-2.14.15
Status: Released (was: ChangeUnderReview)
Labels: FixedIn-2.15.5
Labels: -FixedIn-2.15.5 -FixedIn-2.14.15
Status: ChangeUnderReview (was: Released)
https://gerrit-review.googlesource.com/c/gerrit/+/199810

Comment 13 by david.pu...@gmail.com, Oct 17 (2 days ago)

Labels: FixedIn-2.14.16 FixedIn-2.15.5
Status: Submitted (was: ChangeUnderReview)

Sign in to add a comment