My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 3586: User with "Create Reference" can bypass code review
1 person starred this issue and may be notified of changes. Back to list
Status:  New
Owner:  ----


Sign in to add a comment
 
Project Member Reported by dougk....@gmail.com, Sep 28, 2015
Affected Version: 2.10.6

What steps will reproduce the problem?
1. Grant a user explicit "create reference" under some portion of the repo (i.e. refs/heads/development/*)
2. Create a commit not on any branch
3. Push the commit directly to a new branch (i.e. refs/heads/development/test)

What is the expected output? What do you see instead?
The change is merged directly to the repository without review, but the change should require review.


Please provide any additional information below.
The issue seems to be on line 264 on of RefControl.java (from 2.10.6):
      return admin
          || (owner && !isBlocked(Permission.CREATE))
          || (canPerform(Permission.CREATE) && (!existsOnServer && canUpdate() || projectControl
              .canReadCommit(rw, (RevCommit) object)));

Looking at this, I'm not sure if it's an order of operations issue (but the order of operations is certainly unclear in that third "or" clause), but I believe this should read:
      return admin
          || (owner && !isBlocked(Permission.CREATE))
          || (canPerform(Permission.CREATE) && ((!existsOnServer && canUpdate()) || (
              (existsOnServer && projectControl.canReadCommit(rw, (RevCommit) object)));
Sep 28, 2015
Project Member #1 dougk....@gmail.com
Correction, I meant to write:
      return admin
          || (owner && !isBlocked(Permission.CREATE))
          || (canPerform(Permission.CREATE) && ((!existsOnServer && canUpdate()) ||
              (existsOnServer && projectControl.canReadCommit(rw, (RevCommit) object))));

Sep 28, 2015
Project Member #2 dougk....@gmail.com
One last correction, and it *appears* to be working right in my test environment.

ReceiveCommits.java line 1006:
    if (ctl.canCreate(rp.getRevWalk(), obj, allRefs.values().contains(obj))) {

should now read:
    if (ctl.canCreate(rp.getRevWalk(), obj, rp.getAdvertisedObjects().contains(obj))) {

Upon doing this, existence on the server is evaluated properly *but* it doesn't check to ensure the change is merged into a branch.  Previously, existsOnServer was always evaluating to false.  If the change is pushed for review with only the modifications here, a user can again bypass code review.
Sep 28, 2015
Project Member #3 dougk....@gmail.com
This ugly blob seems to behave in a way I imagine would be "sane":

    boolean containsObject = false;
    RevWalk rw = rp.getRevWalk();
    for (Ref ref : allRefs.values()) {
      if (ref.getName().startsWith(REFS_CHANGES)
          || ref.getName().startsWith(RefNames.REFS_CACHE_AUTOMERGE)) {
        continue;
      }
      try {
        containsObject = containsObject || rw.isMergedInto(rw.parseCommit(obj.getId()), rw.parseCommit(ref.getObjectId()));
      } catch (IOException e) {
        // Ignore exceptions.
      }
    }

Clearly not ideal.  But works.
Sep 29, 2015
Project Member #4 dougk....@gmail.com
OK, going back and looking at this on the advice of David Pursehouse, it seems this was fixed in 2.11.  Should this be noted accordingly for older versions?
Labels: FixedIn-2.11
Sep 29, 2015
Project Member #5 david.pu...@sonymobile.com
Looks like it was fixed by these commits which went into 2.11

https://gerrit-review.googlesource.com/#/c/59043/
https://gerrit-review.googlesource.com/#/c/59081/

Labels: -NonPublic
Sign in to add a comment

Powered by Google Project Hosting