Necessity of reviewing every mojo change for WebUI

[Yuheng Huang]

Hi, mojo/security owners

This is Yuheng Huang from the desktop UI team. We're working on the Tab Search feature targeting M87.

We're currently working on a retrospective to make our workflow more efficient. One thing that comes to our mind is the necessity of reviewing every mojo change.

According to this document https://source.chromium.org/chromium/chromium/src/+/master:ipc/SECURITY_OWNERS

every IPC needs to go through security owners review.

Since we are using mojom as IPC it also applies to us. Our argument is that WebUI page is also highly privileged and has its own restriction (See the document from here https://chromium.googlesource.com/chromium/src/+/master/docs/webui_explainer.md)

We wonder if we can make an exception for WebUI not needing to go through security review for every change. If there are concerns about it, please let us know what they are and give us some examples that we can understand better.

The traditional WebUI IPC through chrome.send etc. does not require security review for every change. The rule that works for traditional WebUI IPC should also work for mojom.

Mojom is working so great for us, if we could make it as approachable as chrome.send() it will help us to adapt the tech to other WebUI features as well.

Thanks,

Yuheng

[Daniel Cheng]

I think the fact that traditional WebUI *doesn't* require this sort of review is something we're hoping to eliminate over time.

While WebUI is highly-privileged, it's ultimately still not the same as running code in the browser process, and WebUI itself has been used as a way to escalate privileges in past exploits.

I don't think we should drop the security review requirement.

Daniel

[Daniel Cheng]

I went back through some of the reviews, and my concrete suggestion would be to add an IPC reviewer sooner rather than later. I noticed that in many of the reviews, an IPC reviewer wasn't added until after CL itself was already +1'ed by someone else. Given that the SLA for code reviews is 24 hours, this can easily add up to a lot of additional delay.

Daniel

[Yuheng Huang]

Thanks for the suggestion Daniel. Having a IPC reviewer sooner than later will definitely help!

[Demetrios Papadopoulos]

+Tommy Li  +Nasko Oskov who are possibly interested in this discussion.

[Tommy Li]

Thanks Demetrios.

As mentioned also on the WebUI omnibox doc, go/webui-omnibox -- it's kind of a pain in the butt to always get an IPC SECURITY reviewer, and I think it leads to developers preferring to stick with the traditional WebUI IPC techniques for velocity's sake. Sticking with the traditional methods... I don't think that's a security win.

It would be ideal to me if, when the WebUI developer types "git cl owners", it says a message saying:

• Warning, the WebUI owners can approve mojom changes, but when passing sensitive data, introducing substantive new APIs, or otherwise when in doubt, please get a security review.

Or, it would be fine if a Bot printed that in Gerrit too.

I think maybe 90% of CLs are relatively trivial mojom security wise, and should be able to be approved by the regular owners. Both reviewer and author should be able to call in a security review by exercising their judgement.

Thanks,

Tommy

[Nasko Oskov]

On Tue, Nov 10, 2020 at 10:50 AM Tommy Li <tomm...@chromium.org> wrote:

Thanks Demetrios.

As mentioned also on the WebUI omnibox doc, go/webui-omnibox -- it's kind of a pain in the butt to always get an IPC SECURITY reviewer, and I think it leads to developers preferring to stick with the traditional WebUI IPC techniques for velocity's sake. Sticking with the traditional methods... I don't think that's a security win.

I would agree that sticking with the old methods is not a security win, so I'd like us to find a workable solution.

 

It would be ideal to me if, when the WebUI developer types "git cl owners", it says a message saying:

• Warning, the WebUI owners can approve mojom changes, but when passing sensitive data, introducing substantive new APIs, or otherwise when in doubt, please get a security review.

Or, it would be fine if a Bot printed that in Gerrit too.

I think maybe 90% of CLs are relatively trivial mojom security wise, and should be able to be approved by the regular owners. Both reviewer and author should be able to call in a security review by exercising their judgement.

Let's brainstorm some potential solutions. Here is one idea that is based on what you suggested - would it be workable to have a set of folks within the WebUI teams that are the reviewers for mojom changes within WebUI? Those folks will be responsible for knowing what is a scary change that needs reviewing further and what is a trivial change that doesn't need security involvement. 

[Yuheng Huang]

It would be nice if you can graduate some of us to IPC reviewers. Is it sufficient to read through this doc to be one?

https://www.chromium.org/Home/chromium-security

[Nasko Oskov]

There is a more specific link on that page - https://www.chromium.org/Home/chromium-security/ipc-security-reviews.

[Nasko Oskov]

And just to re-emphasize one thing dcheng@ pointed out to ensure it is not lost in this long thread - add IPC reviewers early on. This will lead to a quicker path to landing a CL. If folks wait for all other reviews to complete, they are adding explicit serialization which *always* adds delay.

[Tommy Li]

Thanks for that info. I agree with Nasko / Yuheng's proposal.

It would be good to "graduate" a few WebUI reviewers to also being able to review WebUI IPCs, after they've confirmed they have reviewed and are comfortable enforcing the IPC guidelines that Yuheng linked.

It looks like the process is to express interest, and then partner review 3 CLs, which seems reasonable.