Start replicating sandbox flags for OOPIF

Start replicating sandbox flags for OOPIF.

This CL adds plumbing to replicate sandbox flags through the browser process:
* Introduce a new enum to refer to sandbox flags in content.
* Pass sandbox flags as part of FrameHostMsg_CreateChildFrame and store them in a FrameTreeNode's FrameReplicationState.  Keep the older version of WebFrameClient::createChildFrame around until Blink switches over to the new version.
* Pass sandbox flags in FrameMsg_NewFrame, so that a local frame's blink::SecurityContext can be initialized with proper sandbox flags.  Also initialize sandbox flags for remote frames using FrameReplicationState already passed to new RenderFrameProxies.

The corresponding Blink-side CL is: https://codereview.chromium.org/793493003/

After this CL, there will be a second Blink CL (https://codereview.chromium.org/838903002/) that will plumb correct sandbox flags into WebFrameClient::createChildFrame(); this will actually enable the replication.  Then, another Chromium CL (https://codereview.chromium.org/797813006/) will remove the old createChildFrame and add browsertests.

This CL doesn't cover the case where a frame's sandbox flags are modified through JavaScript and the frame is re-navigated.  In this case, the renderer should tell the browser process about the updated sandbox flags with a separate IPC; this is left for a future CL.

Skipping presubmit due to a warning about IPC_ENUM_TRAITS, which was discussed and approved in comments.

BUG=426512
NOPRESUBMIT=true

Committed: https://crrev.com/e48b1df9331ba5a366c09f86032e824d59d517ac
Cr-Commit-Position: refs/heads/master@{#311808}

[alexmos, 2015-01-08 01:51:40]

alexmos@chromium.org changed reviewers:
+ creis@chromium.org

[alexmos, 2015-01-08 01:51:41]

Charlie, could you please review this CL?  This has most of my attempt at
sandbox flags replication at the Chromium side.  It doesn't include tests, which
I moved to a follow-up CL because they will need another Blink CL to land after
this to start working.

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
content/common/frame_messages.h:47: IPC_ENUM_TRAITS(content::SandboxFlags)  //
Bitmask.
This results in a presubmit warning about IPC_ENUM_TRAITS being deprecated, but
I'm not sure what the right thing is for bitmasks.  Other places in the code
seem to use this.

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
content/common/frame_messages.h:394: content::FrameReplicationState /*
replication_state */)
Not actually sure whether to pass just the sandbox flags or the whole
FrameReplicationState here, which I originally intended for remote frames, not
local frames.  An advantage of using FrameReplicationState is that we can also
use it to pass things like the frame's name later.

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_rep...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_rep...
content/common/frame_replication_state.h:15: enum class SandboxFlags : int {
Made this an "enum class" so I could forward-declare it in mock_render_thread.h,
where presubmit rules didn't let me include frame_replication_state.h.

[Charlie Reis, 2015-01-08 22:17:25]

creis@chromium.org changed reviewers:
+ dcheng@chromium.org

[Charlie Reis, 2015-01-08 22:17:26]

[+dcheng for C++11 enum class sanity check.]

Looks pretty good; just a few comments below.

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
content/common/frame_messages.h:47: IPC_ENUM_TRAITS(content::SandboxFlags)  //
Bitmask.
On 2015/01/08 01:51:41, alexmos wrote:
> This results in a presubmit warning about IPC_ENUM_TRAITS being deprecated,
but
> I'm not sure what the right thing is for bitmasks.  Other places in the code
> seem to use this.

The deprecation warning points you to use IPC_ENUM_TRAITS_MIN_MAX_VALUE instead:
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
content/common/frame_messages.h:394: content::FrameReplicationState /*
replication_state */)
On 2015/01/08 01:51:41, alexmos wrote:
> Not actually sure whether to pass just the sandbox flags or the whole
> FrameReplicationState here, which I originally intended for remote frames, not
> local frames.  An advantage of using FrameReplicationState is that we can also
> use it to pass things like the frame's name later.

I suppose it depends on what we expect to end up in FrameReplicationState.  It
does seem intended for remote frames, but if most of its members are needed when
doing a remote -> local swap, then maybe it's the right thing.  I admit that it
feels odd to pass origin and sizing info to a new local frame, when those are
ignored and handled in other ways.

If the answer is unclear, I'd suggest passing just the SandboxFlags for now.

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_rep...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_rep...
content/common/frame_replication_state.h:13: // Must be kept in sync with
blink::WebSandboxFlags.  Enforced in
We should document what this is for, since it could easily be confused with
Chrome's sandbox.

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_rep...
content/common/frame_replication_state.h:15: enum class SandboxFlags : int {
On 2015/01/08 01:51:41, alexmos wrote:
> Made this an "enum class" so I could forward-declare it in
mock_render_thread.h,
> where presubmit rules didn't let me include frame_replication_state.h.

There was a big C++11 discussion thread on this and flags, which got too
bikeshedded for me to follow:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Q5WmkAImanc

This SGTM for the forward declare property, if there aren't issues with using it
for flags.  Let's just have dcheng verify.

https://codereview.chromium.org/837283003/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/837283003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:480: // Check that blink::WebSandboxFlags
is kept in sync with
I haven't dealt with these much before, but it looks like other similar cases
provide a conversion function along with the macros, such as ui::TextInputType
RenderWidget::WebKitToUiTextInputType.  Maybe we should do that as well?

[dcheng, 2015-01-09 07:56:57]

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
content/common/frame_messages.h:47: IPC_ENUM_TRAITS(content::SandboxFlags)  //
Bitmask.
On 2015/01/08 22:17:26, Charlie Reis wrote:
> On 2015/01/08 01:51:41, alexmos wrote:
> > This results in a presubmit warning about IPC_ENUM_TRAITS being deprecated,
> but
> > I'm not sure what the right thing is for bitmasks.  Other places in the code
> > seem to use this.
> 
> The deprecation warning points you to use IPC_ENUM_TRAITS_MIN_MAX_VALUE
instead:
> http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

It's fine to use IPC_ENUM_TRAITS in this situation
(https://groups.google.com/a/chromium.org/d/msg/chromium-reviews/ew61eyctSfw/s...
for a reference that's not just me =). Obviously, code that uses this will want
to be careful when using this value. I'm almost tempted to say we should have
another IPC_ENUM_TRAITS macro for this that at least checks that you don't pass
any out-of-range bits... but IIRC, SandboxFlags defines the -1 flag to be ALL,
so it doesn't really matter.

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_rep...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_rep...
content/common/frame_replication_state.h:15: enum class SandboxFlags : int {
On 2015/01/08 22:17:26, Charlie Reis wrote:
> On 2015/01/08 01:51:41, alexmos wrote:
> > Made this an "enum class" so I could forward-declare it in
> mock_render_thread.h,
> > where presubmit rules didn't let me include frame_replication_state.h.
> 
> There was a big C++11 discussion thread on this and flags, which got too
> bikeshedded for me to follow:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Q5WmkAImanc
> 
> This SGTM for the forward declare property, if there aren't issues with using
it
> for flags.  Let's just have dcheng verify.

Yep, this is fine =)

[alexmos, 2015-01-09 20:43:25]

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
content/common/frame_messages.h:394: content::FrameReplicationState /*
replication_state */)
On 2015/01/08 22:17:26, Charlie Reis wrote:
> On 2015/01/08 01:51:41, alexmos wrote:
> > Not actually sure whether to pass just the sandbox flags or the whole
> > FrameReplicationState here, which I originally intended for remote frames,
not
> > local frames.  An advantage of using FrameReplicationState is that we can
also
> > use it to pass things like the frame's name later.
> 
> I suppose it depends on what we expect to end up in FrameReplicationState.  It
> does seem intended for remote frames, but if most of its members are needed
when
> doing a remote -> local swap, then maybe it's the right thing.  I admit that
it
> feels odd to pass origin and sizing info to a new local frame, when those are
> ignored and handled in other ways.
> 
> If the answer is unclear, I'd suggest passing just the SandboxFlags for now.

After tinkering with window.name replication, it may be a bit easier to keep
this as a FrameReplicationState, since otherwise I'd need to update this again
to also take the frame name.

Maybe down the road we should have separate structs, LocalFrameReplicationState
and RemoteFrameReplicationState, if we end up with several things like origin
that are not going to be used?

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_rep...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_rep...
content/common/frame_replication_state.h:13: // Must be kept in sync with
blink::WebSandboxFlags.  Enforced in
On 2015/01/08 22:17:26, Charlie Reis wrote:
> We should document what this is for, since it could easily be confused with
> Chrome's sandbox.

Done.

https://codereview.chromium.org/837283003/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/837283003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:480: // Check that blink::WebSandboxFlags
is kept in sync with
On 2015/01/08 22:17:26, Charlie Reis wrote:
> I haven't dealt with these much before, but it looks like other similar cases
> provide a conversion function along with the macros, such as ui::TextInputType
> RenderWidget::WebKitToUiTextInputType.  Maybe we should do that as well?

Done.  I put them as static functions in RenderFrameImpl, so that
RenderFrameProxy could use them; not sure if there's a better place.

[Charlie Reis, 2015-01-12 20:25:12]

Thanks!  LGTM with nits.

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/837283003/diff/20001/content/common/frame_mes...
content/common/frame_messages.h:394: content::FrameReplicationState /*
replication_state */)
On 2015/01/09 20:43:25, alexmos wrote:
> On 2015/01/08 22:17:26, Charlie Reis wrote:
> > On 2015/01/08 01:51:41, alexmos wrote:
> > > Not actually sure whether to pass just the sandbox flags or the whole
> > > FrameReplicationState here, which I originally intended for remote frames,
> not
> > > local frames.  An advantage of using FrameReplicationState is that we can
> also
> > > use it to pass things like the frame's name later.
> > 
> > I suppose it depends on what we expect to end up in FrameReplicationState. 
It
> > does seem intended for remote frames, but if most of its members are needed
> when
> > doing a remote -> local swap, then maybe it's the right thing.  I admit that
> it
> > feels odd to pass origin and sizing info to a new local frame, when those
are
> > ignored and handled in other ways.
> > 
> > If the answer is unclear, I'd suggest passing just the SandboxFlags for now.
> 
> After tinkering with window.name replication, it may be a bit easier to keep
> this as a FrameReplicationState, since otherwise I'd need to update this again
> to also take the frame name.
> 
> Maybe down the road we should have separate structs,
LocalFrameReplicationState
> and RemoteFrameReplicationState, if we end up with several things like origin
> that are not going to be used?

Yes, let's keep that in mind, in case we end up with multiple mutually exclusive
fields.  Better not to use the same struct for that.

https://codereview.chromium.org/837283003/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/837283003/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:389: // to replace the proxy on commit.
nit: We should document the new parameter.

https://codereview.chromium.org/837283003/diff/80001/content/common/frame_rep...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/837283003/diff/80001/content/common/frame_rep...
content/common/frame_replication_state.h:37: // RenderFrame to any of its
associated RenderFrameProxies.
If we're going to send this both ways (including remote -> local), then we
should update this comment.

https://codereview.chromium.org/837283003/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/837283003/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:496: content::SandboxFlags::NONE),
nit: No need for content:: here since we're already in that namespace.  You can
also use ContentToWebSandboxFlags rather than the static_cast, so this should
fit on one fewer line.

[alexmos, 2015-01-14 19:35:19]

Thanks!

https://codereview.chromium.org/837283003/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/837283003/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:389: // to replace the proxy on commit.
On 2015/01/12 20:25:12, Charlie Reis wrote:
> nit: We should document the new parameter.

Done, PTAL.

https://codereview.chromium.org/837283003/diff/80001/content/common/frame_rep...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/837283003/diff/80001/content/common/frame_rep...
content/common/frame_replication_state.h:37: // RenderFrame to any of its
associated RenderFrameProxies.
On 2015/01/12 20:25:12, Charlie Reis wrote:
> If we're going to send this both ways (including remote -> local), then we
> should update this comment.

Done.

https://codereview.chromium.org/837283003/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/837283003/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:496: content::SandboxFlags::NONE),
On 2015/01/12 20:25:12, Charlie Reis wrote:
> nit: No need for content:: here since we're already in that namespace.  You
can
> also use ContentToWebSandboxFlags rather than the static_cast, so this should
> fit on one fewer line.

Done.  Removed unnecessary content::, but couldn't use ContentToWebSandboxFlags
as it can't be resolved at compile time (and C++11's constexpr is banned on
https://chromium-cpp.appspot.com/).

[alexmos, 2015-01-14 19:45:39]

alexmos@chromium.org changed reviewers:
+ nasko@chromium.org, thestig@chromium.org

[alexmos, 2015-01-14 19:45:39]

Owner's approval time:

nasko@: please review content/common/frame_messages.h
thestig@: please review chrome/renderer/printing/

[Charlie Reis, 2015-01-14 19:46:44]

creis@chromium.org changed reviewers:
- nasko@chromium.org, thestig@chromium.org

[Charlie Reis, 2015-01-14 19:46:45]

LGTM.

These compile asserts are done elsewhere, so I assume there must be a way to add
a value to them in Blink without breaking the Chrome compile?  (Updating the
Chrome side would have to be a separate CL.)

https://codereview.chromium.org/837283003/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/837283003/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:496: content::SandboxFlags::NONE),
On 2015/01/14 19:35:19, alexmos wrote:
> On 2015/01/12 20:25:12, Charlie Reis wrote:
> > nit: No need for content:: here since we're already in that namespace.  You
> can
> > also use ContentToWebSandboxFlags rather than the static_cast, so this
should
> > fit on one fewer line.
> 
> Done.  Removed unnecessary content::, but couldn't use
ContentToWebSandboxFlags
> as it can't be resolved at compile time (and C++11's constexpr is banned on
> https://chromium-cpp.appspot.com/).

Huh, I guess that's why people cast both sides to int elsewhere.  Oh well.

[Lei Zhang, 2015-01-14 22:45:34]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2015-01-14 22:45:35]

print_web_view_helper.cc lgtm

[alexmos, 2015-01-15 18:59:24]

alexmos@chromium.org changed reviewers:
+ nasko@chromium.org

[alexmos, 2015-01-15 18:59:25]

Readding nasko@ who was somehow dropped off the list.  

Nasko: please review content/common/frame_messages.h

[nasko, 2015-01-15 19:59:12]

Just a couple of nits. Overall LGTM.

https://codereview.chromium.org/837283003/diff/180001/components/printing/ren...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/837283003/diff/180001/components/printing/ren...
components/printing/renderer/print_web_view_helper.cc:728: return
createChildFrame(parent, name, blink::WebSandboxFlags::None);
I wonder if printing rendering allows for executing JS or other things that
sandbox flags will prevent and can be bypassed by passing None here.

https://codereview.chromium.org/837283003/diff/180001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/837283003/diff/180001/content/common/frame_re...
content/common/frame_replication_state.h:14: // attribute in the renderer and
forwarded to the browser process, which
nit: s/renderer/renderer process/

[alexmos, 2015-01-15 21:53:51]

Thanks!

https://codereview.chromium.org/837283003/diff/180001/components/printing/ren...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/837283003/diff/180001/components/printing/ren...
components/printing/renderer/print_web_view_helper.cc:728: return
createChildFrame(parent, name, blink::WebSandboxFlags::None);
On 2015/01/15 19:59:11, nasko wrote:
> I wonder if printing rendering allows for executing JS or other things that
> sandbox flags will prevent and can be bypassed by passing None here.

Well, this method will go away once the next Blink CL lands, so we should always
pass the right sandbox flags from Blink.  

Actually, how do we expect print rendering to work with --site-per-process?  I
don't know much about how it works currently -- would print rendering of child
frames also use different processes under --site-per-process?

https://codereview.chromium.org/837283003/diff/180001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/837283003/diff/180001/content/common/frame_re...
content/common/frame_replication_state.h:14: // attribute in the renderer and
forwarded to the browser process, which
On 2015/01/15 19:59:11, nasko wrote:
> nit: s/renderer/renderer process/

Done.

[alexmos, 2015-01-15 21:54:02]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-01-15 22:18:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/837283003/200001

[commit-bot: I haz the power, 2015-01-15 23:19:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-15 23:19:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alexmos, 2015-01-16 01:33:02]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-01-16 01:33:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/837283003/200001

[commit-bot: I haz the power, 2015-01-16 01:34:51]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-01-16 01:35:58]

Patchset 11 (id:??) landed as
https://crrev.com/e48b1df9331ba5a366c09f86032e824d59d517ac
Cr-Commit-Position: refs/heads/master@{#311808}