[Android] Allow __NR_rt_tgsigqueueinfo under seccomp.

[Android] Allow __NR_rt_tgsigqueueinfo under seccomp.

BUG=644759
Committed: https://crrev.com/a52c715618a5f2ad02f507c6ad3ef6282c112bc7
Cr-Commit-Position: refs/heads/master@{#417419}

[Robert Sesek, 2016-09-07 21:33:42]

The CQ bit was checked by rsesek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 21:34:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 22:36:42]

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

[commit-bot: I haz the power, 2016-09-07 22:36:42]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2016-09-07 22:37:03]

rsesek@chromium.org changed reviewers:
+ jln@chromium.org

[jln (very slow on Chromium), 2016-09-07 23:22:05]

lgtm

https://codereview.chromium.org/2313393003/diff/1/content/common/sandbox_linu...
File content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc
(right):

https://codereview.chromium.org/2313393003/diff/1/content/common/sandbox_linu...
content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:119: //
and then ptrace the parent.
Drive-by: do we have any plans to move away from this? Are we confident that
there is no process with the same gid that is not subject to this seccomp
policy? It's a risk to the "consistency" of the policy.

https://codereview.chromium.org/2313393003/diff/1/content/common/sandbox_linu...
content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:128:
return If(tgid == getpid(), Allow())
Do you want to just capture the current pid as a member variable of the policy
(see policy_pid_ in baseline_policy.h). It's good practice to make the condition
of If() as "constant" as possible.

Note: the uinfo argument of this syscall sadly allows to spoof the sender PIDs,
which has been a security issue in the past. Fixing this with seccomp is very
convoluted, so I don't think we want to go there.
(I fixed it in Linux in
http://www.openwall.com/lists/oss-security/2011/03/22/13, but it regressed
later).

[Robert Sesek, 2016-09-08 18:31:51]

The CQ bit was checked by rsesek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 18:32:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Robert Sesek, 2016-09-08 18:33:02]

Thanks for the review!

https://codereview.chromium.org/2313393003/diff/1/content/common/sandbox_linu...
File content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc
(right):

https://codereview.chromium.org/2313393003/diff/1/content/common/sandbox_linu...
content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:119: //
and then ptrace the parent.
On 2016/09/07 23:22:05, jln (very slow on Chromium) wrote:
> Drive-by: do we have any plans to move away from this? Are we confident that
> there is no process with the same gid that is not subject to this seccomp
> policy? It's a risk to the "consistency" of the policy.

Yes, because we only apply this to isolatedProcess services, we know that the
UID and GID are the same and from the ephemeral isolated UID pool.

https://codereview.chromium.org/2313393003/diff/1/content/common/sandbox_linu...
content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:128:
return If(tgid == getpid(), Allow())
On 2016/09/07 23:22:05, jln (very slow on Chromium) wrote:
> Do you want to just capture the current pid as a member variable of the policy
> (see policy_pid_ in baseline_policy.h). It's good practice to make the
condition
> of If() as "constant" as possible.

Done.

> Note: the uinfo argument of this syscall sadly allows to spoof the sender
PIDs,
> which has been a security issue in the past. Fixing this with seccomp is very
> convoluted, so I don't think we want to go there.
> (I fixed it in Linux in
> http://www.openwall.com/lists/oss-security/2011/03/22/13, but it regressed
> later).

That is unfortunate. However I think the risk is minimal here, given that we at
worst we would signal ourselves.

[commit-bot: I haz the power, 2016-09-08 19:22:16]

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

[commit-bot: I haz the power, 2016-09-08 19:22:17]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2016-09-08 22:27:32]

The CQ bit was checked by rsesek@chromium.org

[Robert Sesek, 2016-09-08 22:27:32]

The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org
Link to the patchset: https://codereview.chromium.org/2313393003/#ps20001
(title: "const pid member")

[commit-bot: I haz the power, 2016-09-08 22:28:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 22:34:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-08 22:37:25]

Description was changed from

==========
[Android] Allow __NR_rt_tgsigqueueinfo under seccomp.

BUG=644759
==========

to

==========
[Android] Allow __NR_rt_tgsigqueueinfo under seccomp.

BUG=644759

Committed: https://crrev.com/a52c715618a5f2ad02f507c6ad3ef6282c112bc7
Cr-Commit-Position: refs/heads/master@{#417419}
==========

[commit-bot: I haz the power, 2016-09-08 22:37:26]

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