|
|
Created:
5 years, 8 months ago by jln (very slow on Chromium) Modified:
5 years, 8 months ago CC:
chromium-reviews, rickyz+watch_chromium.org, jln+watch_chromium.org, mdempsky, Kees Cook Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux sandbox: workaround colliding system call.
Some Android kernels have system calls that collide with sys_seccomp,
try to detect this situation.
BUG=478891
Committed: https://crrev.com/c08969c1b8b723494569c893b15c9b2e634ef0fc
Cr-Commit-Position: refs/heads/master@{#326667}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Better comments #
Total comments: 2
Patch Set 3 : Add comma. #Messages
Total messages: 20 (4 generated)
jln@chromium.org changed reviewers: + rsesek@chromium.org
A potential issue even with this probing is that in sys_set_media_ext(), we'll still end clearing savedfileExtList since the memset to zero is hit unconditionally. Not sure there's a way to probe non-destructive.
On 2015/04/20 20:44:49, Robert Sesek wrote: > A potential issue even with this probing is that in sys_set_media_ext(), we'll > still end clearing savedfileExtList since the memset to zero is hit > unconditionally. Not sure there's a way to probe non-destructive. Yeah frustrating! There is another syscall it clobbers too but thats unconditionally destructive too.
palmer@chromium.org changed reviewers: + palmer@chromium.org
I think it's important to be as clear as is possible in these comments, since what is happening is pretty out of the ordinary. https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:62: // LG introduced a buggy syscall, sys_set_media_ext with the same number as Typo: s/ /, / https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:70: // This has to be seen as a NULL pointer by sys_set_media_ext to not crash, Nit: I think this comment could be stated more clearly. One idea: sys_set_media_ext will see this as NULL, which should be a safe (non-crashing) way to invoke it. A genuine seccomp syscall will see it as |SECCOMP_SET_MODE_STRICT|, which is also safe. https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:76: // A genuine kernel would EINVAL, or at the very least return some kind of "...would return -1 and set errno to EINVAL. Any other behavior indicates that whatever code received our syscall was not the real seccomp."
Based on the internal discussion, I think I'm OK moving forward with this patch once Chris' nits are addressed.
PTAL! https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:62: // LG introduced a buggy syscall, sys_set_media_ext with the same number as On 2015/04/21 03:28:43, palmer wrote: > Typo: s/ /, / Done. https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:70: // This has to be seen as a NULL pointer by sys_set_media_ext to not crash, On 2015/04/21 03:28:43, palmer wrote: > Nit: I think this comment could be stated more clearly. One idea: > > sys_set_media_ext will see this as NULL, which should be a safe (non-crashing) > way to invoke it. A genuine seccomp syscall will see it as > |SECCOMP_SET_MODE_STRICT|, which is also safe. Done. https://chromiumcodereview.appspot.com/1095133003/diff/1/sandbox/linux/seccom... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:76: // A genuine kernel would EINVAL, or at the very least return some kind of On 2015/04/21 03:28:44, palmer wrote: > "...would return -1 and set errno to EINVAL. Any other behavior indicates that > whatever code received our syscall was not the real seccomp." Done.
Do we want to restrict this even further, like limiting it to kernel 3.10 (which is where we know this collides)? It'd be even better if we could limit it to some kind of vendor tag, though I don't know if that's available to us without JNI on Android.
On 2015/04/23 17:54:03, Robert Sesek wrote: > Do we want to restrict this even further, like limiting it to kernel 3.10 (which > is where we know this collides)? It'd be even better if we could limit it to > some kind of vendor tag, though I don't know if that's available to us without > JNI on Android. I'm not too worried about it given that we're doing SECCOMP_SET_MODE_STRICT, which is unlikely to ever evolve. Moreover, the seccomp system call is well designed insofar as it rejects unknown flags (which is a much more future way to do things). So in a way this is a pretty reasonable way to detect seccomp squatting in a fairly generic way. Matthew also suggested that we could try to see if uname(2) gives us some lg-specific info. Or we could try and pin the version indeed.
On 2015/04/23 17:59:59, jln wrote: > Moreover, the seccomp system call is well > designed insofar as it rejects unknown flags (which is a much more future way to > do things). So in a way this is a pretty reasonable way to detect seccomp > squatting in a fairly generic way. s/future/future-proof/
LGTM with absurdly tiny nit :) https://chromiumcodereview.appspot.com/1095133003/diff/20001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/1095133003/diff/20001/sandbox/linux/se... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:62: // LG introduced a buggy syscall, sys_set_media_ext with the same number as Nit: "...syscall, sys_set_media_ext, with..."
https://chromiumcodereview.appspot.com/1095133003/diff/20001/sandbox/linux/se... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/1095133003/diff/20001/sandbox/linux/se... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:62: // LG introduced a buggy syscall, sys_set_media_ext with the same number as On 2015/04/23 18:20:23, palmer wrote: > Nit: "...syscall, sys_set_media_ext, with..." Done.
Robert: Matthew seems fine with this. Let's go?
LGTM
The CQ bit was checked by jln@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1095133003/#ps40001 (title: "Add comma.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1095133003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c08969c1b8b723494569c893b15c9b2e634ef0fc Cr-Commit-Position: refs/heads/master@{#326667} |