New issue
Advanced search Search tips

Issue 801315 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug



Sign in to add a comment

Newly added mojom files bypass security review enforcement

Project Member Reported by dcheng@chromium.org, Jan 11 2018

Issue description

This is because of https://crrev.com/d0573ec067f852c3a967b8e6aa38ad07615a6dc9, which does OWNERS checks with OldContents(). If a CL adds OWNERS rules for a mojom file and a new mojom file in the same CL, owners.py will use the version of the OWNERS files that doesn't enforce security review. Doh!

One workaround might be to extend the _CheckIpcOwners() check to verify that at least one security reviewer is added...
 

Comment 1 by dcheng@chromium.org, Jan 11 2018

Labels: -Pri-3 OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows Pri-1

Comment 2 by dcheng@chromium.org, Jan 11 2018

One idea:
When doing OWNERS checks, only use override_files for checking changes to OWNERS files themselves.

Unfortunately, this means that if you remove a directory, the removed files would use NewContents() as the check and you'd need parent OWNER approval...

Comment 3 by jochen@chromium.org, Jan 12 2018

ouch :/
Status: Started (was: Assigned)
https://chromium-review.googlesource.com/c/chromium/src/+/1137103 is an example of a CL that got missed due to this bug.
OK, I've been thinking about this more.

Would it be reasonable to tweak the OWNERS check so that we use OldContents() when determining owners for changes to OWNERS files... and NewContents() otherwise? To me, the main situation we want to avoid is someone being able to add themselves to an OWNERS file *and* be able to +1 it.

It's not a big issue for non-OWNERS files: either there are no changes to OWNERS, and an existing reviewer will look at it--or the CL also includes a change to OWNERS, at which point, a pre-existing OWNER will have to look at the CL anyway. 

Of course, this gets tricky due to file: includes... so I'd also propose restricting file: includes for OWNERS to require that the included file end in a suffix of "OWNERS".

dpranke, WDYT of this proposal since jochen is OOO? I explored several other ways to approach this, including having a seperate presubmit to try to ensure IPC security reviewers are included as needed--but it ends up being kind of hard to support all the edge cases properly.
@dcheng - the proposal in #c6 seems fine.
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/74fda7147c1b547d343e3323e5babdd304ad4f98

commit 74fda7147c1b547d343e3323e5babdd304ad4f98
Author: Daniel Cheng <dcheng@chromium.org>
Date: Wed Sep 05 03:56:39 2018

[owners.py] require stricter naming conventions for file: includes

Files that control ownership have special ownership rules, to disallow
someone from self-approving a CL that adds themselves to a file that
controls ownership. owners.py now requires that they be named OWNERS or
end with a suffix of _OWNERS to ensure the special ownership rules are
correctly applied.

Bug: 801315
Change-Id: I083a2d15bdc9c749e3838fb1c983286d5a7c4af9
Reviewed-on: https://chromium-review.googlesource.com/1204917
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>

[modify] https://crrev.com/74fda7147c1b547d343e3323e5babdd304ad4f98/owners.py
[modify] https://crrev.com/74fda7147c1b547d343e3323e5babdd304ad4f98/tests/owners_unittest.py

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/02958afbab8ca4e117639434e311f3c744ed3e4c

commit 02958afbab8ca4e117639434e311f3c744ed3e4c
Author: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Wed Sep 05 09:13:21 2018

Roll src/third_party/depot_tools f215ae6ed0ce..74fda7147c1b (1 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/f215ae6ed0ce..74fda7147c1b


git log f215ae6ed0ce..74fda7147c1b --date=short --no-merges --format='%ad %ae %s'
2018-09-05 dcheng@chromium.org [owners.py] require stricter naming conventions for file: includes


Created with:
  gclient setdep -r src/third_party/depot_tools@74fda7147c1b

The AutoRoll server is located here: https://autoroll.skia.org/r/depot-tools-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG=chromium:801315
TBR=agable@chromium.org

Change-Id: I91dee5ab9d4c5b0d83f4ae6669a475689f265e01
Reviewed-on: https://chromium-review.googlesource.com/1205617
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#588802}
[modify] https://crrev.com/02958afbab8ca4e117639434e311f3c744ed3e4c/DEPS

Sign in to add a comment