Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10826254:
Seccomp: merge i386 and x86_64 architectures in system call sets. (Closed)
|
Created:
8 years, 4 months ago by jln (very slow on Chromium) Modified:
8 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, Chris Evans Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSeccomp: merge i386 and x86_64 architectures in system call sets.
BUG=142030
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151423
Patch Set 1 : #Patch Set 2 : Rebase #
Total comments: 8
Patch Set 3 : Address Markus' comments. #Patch Set 4 : Adapt some comments. #
Total comments: 2
Messages
Total messages: 11 (0 generated)
Finally! We're very close from being able to design a reasonable i386 seccomp policy. Even though socketcall() and ipc() will be a pain and create big holes. (I didn't do an actuall i386 policy in this change, this CL concerns the defining on the set only, so that they are available on both architectures).
https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:179: #if defined(__i386__) I would generally prefer if you did something like: #if defined(__NR_ftime) instead of #if defined(__i386__) Feel free to still leave a comment in there saying that this symbol is only defined on __i386__ (and possibly others). If we do this right, we should have a much easier time porting to things like ARM. There are a good number of subtle changes between system call lists. But on a per-system-call basis, there are usually very few different options. Typically, either the system call exists or it doesn't. It should ultimately be possible to come up with policies that work correctly for all Linux platforms. https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:557: bool IsSocketCall(int sysno) { We need a big comment that this is an incomplete and insecure solution. https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:594: case __NR_mmap2: // Might need to be enabled but should be audited. mmap2() is the new one isn't it? mmap() is the old and broken API. It is probably harmless, but we could disallow mmap(), if mmap2() exists. https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:895: bool IsSystemVIpc(int sysno) { Again, we probably want a comment that this is incomplete and not particularly safe.
Thanks, PTAL! https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:179: #if defined(__i386__) On 2012/08/11 00:58:40, Markus (顧孟勤) wrote: > I would generally prefer if you did something like: > > #if defined(__NR_ftime) > > instead of > > #if defined(__i386__) In general that seems like a good idea. In practice I'm not 100% convinced yet, because we need to develop and test policies on specific platform, and it's really very useful to immediately see that this or is not available on i386. Yeah comments can partly solve it I suppose. But more importantly: there are big blocks (like the set*id32) where having one big #if defined(__i386__) is super useful. In the policies themselves I suspect that being able to regroup a bunch of things under if defined(__i386__) will be nice. Maybe it's something we'll want to consider a bit later when we're more generic and abstracted? I really hope we can get to a point where we have an API to start a sandbox that takes a few options where you state your intentions and that's all we need. Also it makes things a bit fragile in case of some bug in how we include headers, but that could be solved via tests and a few carefully crafted #error. What do you think? https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:557: bool IsSocketCall(int sysno) { On 2012/08/11 00:58:40, Markus (顧孟勤) wrote: > We need a big comment that this is an incomplete and insecure solution. Well there is really no security issue per se. It is "socketcall". Right now it's filtered *out* by default in the baseline (and there really is no i386 policy yet). Perhaps the comment will belong to the callsite when we make i386 policies ? I have nonetheless added a comment above the function signature. https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:594: case __NR_mmap2: // Might need to be enabled but should be audited. On 2012/08/11 00:58:40, Markus (顧孟勤) wrote: > mmap2() is the new one isn't it? mmap() is the old and broken API. It is > probably harmless, but we could disallow mmap(), if mmap2() exists. Ohh yeah, good point, glibc converts the offset argument to mmap2 indeed. Done. https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:895: bool IsSystemVIpc(int sysno) { On 2012/08/11 00:58:40, Markus (顧孟勤) wrote: > Again, we probably want a comment that this is incomplete and not particularly > safe. I moved the comment below above the function signature.
On 2012/08/11 01:36:38, Julien Tinnes wrote: > Thanks, PTAL! > > https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... > File content/common/sandbox_seccomp_bpf_linux.cc (right): > > https://chromiumcodereview.appspot.com/10826254/diff/5002/content/common/sand... > content/common/sandbox_seccomp_bpf_linux.cc:179: #if defined(__i386__) > On 2012/08/11 00:58:40, Markus (顧孟勤) wrote: > > I would generally prefer if you did something like: > > > > #if defined(__NR_ftime) > > > > instead of > > > > #if defined(__i386__) > > In general that seems like a good idea. In practice I'm not 100% convinced yet, > because we need to develop and test policies on specific platform, and it's > really very useful to immediately see that this or is not available on i386. > Yeah comments can partly solve it I suppose. > > But more importantly: there are big blocks (like the set*id32) where having one > big #if defined(__i386__) is super useful. In the policies themselves I suspect > that being able to regroup a bunch of things under if defined(__i386__) will be > nice. > > Maybe it's something we'll want to consider a bit later when we're more generic > and abstracted? I really hope we can get to a point where we have an API to > start a sandbox that takes a few options where you state your intentions and > that's all we need. > > Also it makes things a bit fragile in case of some bug in how we include > headers, but that could be solved via tests and a few carefully crafted #error. > > What do you think? Also to add to that. It's perfectly possible than on a different architecture name collisions will be unfortunate. Maybe a new architecture would have mmap3 and it's really what we'll want to allow while mmap2 should be filtered out. On i386 we'll want to filter out mmap and use mmap2. I think it's more conservative to keep it as is. Abstracting the architecture is a nice thing in general, but I think we need to do it at a much higher level.
lgtm I still think that in the long run you'll be happier to check symbol names rather than architectures (in other words, feature tests almost always work better than version tests). But I won't stop your CL on this. If need be, we can still fix things up later.
lgtm with a question. I found Markus' comment interesting, but I think I would agree that we might not want to do the arch abstraction at this level. https://chromiumcodereview.appspot.com/10826254/diff/12002/content/common/san... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10826254/diff/12002/content/common/san... content/common/sandbox_seccomp_bpf_linux.cc:244: case __NR_lstat64: Broke alphabetical ordering to keep lstat close to its friends?
https://chromiumcodereview.appspot.com/10826254/diff/12002/content/common/san... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10826254/diff/12002/content/common/san... content/common/sandbox_seccomp_bpf_linux.cc:244: case __NR_lstat64: On 2012/08/13 23:55:12, Jorge Lucangeli Obes wrote: > Broke alphabetical ordering to keep lstat close to its friends? Yes, I thought it would make sense and improve readability.
On 2012/08/14 00:22:54, Julien Tinnes wrote: > https://chromiumcodereview.appspot.com/10826254/diff/12002/content/common/san... > File content/common/sandbox_seccomp_bpf_linux.cc (right): > > https://chromiumcodereview.appspot.com/10826254/diff/12002/content/common/san... > content/common/sandbox_seccomp_bpf_linux.cc:244: case __NR_lstat64: > On 2012/08/13 23:55:12, Jorge Lucangeli Obes wrote: > > Broke alphabetical ordering to keep lstat close to its friends? > > Yes, I thought it would make sense and improve readability. Completely agree.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/10826254/12002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/10826254/12002
Change committed as 151423 |
