arbitrary registred users can modify files already modified by open CLs without any restriction to CR or AS flags

#167
Opened by zseri at 2021-12-29T19·40+00

Currently, it is afaik possible to inject (maybe malicious) code into the depot by leveraging existing CLs which have CR+2,CRFO+1 and Autosubmit+1 flags, because it is possible to push a modified patchset to any of these CLs which then (if it doesn't change any additional files besides the ones already changed via the previous, accepted but not yet merged patchset). This case occurs regularly, e.g. when submitted code contains formatting errors catched by depotfmt, which results in the Verified-1 flag. If then a new patchset is pushed which fixes these formatting errors, the Verified-1 flag gets replaced by the Verified+1 flag, and if then all flags are green (as noted above, CR or Autosubmit flags don't get (automatically) removed currently), clbot will submit the changes. The problem is, that nothing seems to currently ensure that the only changes are formatting changes or alternatively made by the original uploader, so new code can be slipped in and will be automatically merged. This should be fixed, and I would prefer (as that would have the least negative impact on my workflow) to just remove the CR+2,CRFO+1 flags when that (last uploader is different than previous uploader) happens.

An example is this: https://cl.tvl.fyi/c/depot/+/4737/2..3 , where it would be very realistic that e.g. an additional ssh key might be slipped into a patchset diff which otherwise just changes formatting.

  1. zseri updated the body of this issue at 2021-12-29T19·42+00
  2. zseri updated the body of this issue at 2021-12-29T19·45+00

  3. This is related to CL/4506, which affects this issue, but didn't solve it.

    patchsets pushed to a change by a non-owning user DO NOT get CRFO

    appears to be wrong if:

    • no additional files were modified
    • instead just files which were already modified by the previous, (potentially still) CR+2 patchset

    It also introduces an unusual edge case: If a patchset was uploaded by somebody other than the owner of the change, and got CRFO from either the new uploader or a third person adding a code review, the rebased patchset may instead gain CRFO from the change owner.

    This appears to be worse than previously thought, because renewing CRFO doesn't seem to be necessary.

    zseri at 2021-12-29T22·55+00

  4. I can't reason my way through how this would happen. CRFO is not a stateful value, it is calculated from the state of the CL at any given point.

    Gerrit tracks who uploaded a change (whose credentials were used), and who owns the change. The only case where CRFO can come from something other than the uploader being an owner, or a reviewer being an owner, is if clbot rebases a CL that was fully approved before (in which case the change owner, which is at least the original uploader, is considered).

    tazjin at 2022-01-22T17·26+00

  5. Okay, we went through this again yesterday and determined this case:

    A CL approved by (i.e. not owned by) someone with owners rights could get auto-submitted if someone else uploaded a new patch set that didn't change the list of files modified, as all approvals were granted in that case.

    We have fixed this in cl/5786 by only allowing autosubmit of a CL if the patchset to be submitted was uploaded by the owner of the change, or by clbot (i.e. if a rebase occured from a patchset uploaded by the owner).

    tazjin at 2022-05-29T11·17+00

  6. tazjin closed this issue at 2022-05-29T11·17+00