Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10837156:
Seccomp-bpf: first pass at a non controverial policy cleanup. (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 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSeccomp-bpf: policy cleanup.
We do some relatively minor changes to our policies and adopt
a baseline on which the GPU and Flash policies are now based.
We allow a few more syscalls than we used to to make our sandbox
more robust against library updates or rarely exercised codepaths.
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150914
Patch Set 1 : #
Total comments: 17
Patch Set 2 : Use a baseline policy. #Patch Set 3 : Rebase on 10834219. #Patch Set 4 : Relocate IsFutex #Patch Set 5 : Add comments about errno for filesystem access system calls. #
Total comments: 18
Patch Set 6 : Address comments from Chris. #Messages
Total messages: 12 (0 generated)
https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:204: case __NR_chmod: All of these will ENOENT. I have a TODO to return the optimal errno for any syscall. https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:284: case __NR_getgroups: None of these should cause any problem. https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:353: case __NR_dup2: This adds very little attack surface and makes us compatible with newer code/glibc. https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:383: case __NR_wait4: Adds very little attack surface and it's really the right thing to do when we allow clone. https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:452: case __NR_mlock: little attack surface, and the amount of locked memory is limited anyway. https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:474: case __NR_ppoll: Some attack surface, but relatively well tested and widely used functions. We already had to allow poll() in some of the policies. https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:477: case __NR_pselect6: Same here. https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:482: case __NR_recvfrom: // Could specify source. recvfrom allows to specify the source, but so does recvmsg which we already allowed. https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:489: case __NR_sendto: // Could specify destination. Same as for recvfrom: we allow sendmsg anyway. https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:515: case __NR_pause: harmless. https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:646: case __NR_eventfd2: newest glibc actually uses eventfd2.
This is a first pass at cleaning-up / rationalizing our policies. (This is based on https://chromiumcodereview.appspot.com/10834219/). I have added comments in the review tool to explain some of those changes.
https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:383: case __NR_wait4: On 2012/08/08 01:29:57, Julien Tinnes wrote: > Adds very little attack surface and it's really the right thing to do when we > allow clone. I tend to agree with your assessment about the attack surface. But clone() is not necessarily a good justification for enabling this system call. If we only allow clone() for creating threads, then waitXXX() is not very useful. You have to use futexes to wait for threads. The waitXXX() functions don't usually work for thread ids. Also, if you enable wait4() and waitid(), please enable waitpid() where available (i.e. x86-32) https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:489: case __NR_sendto: // Could specify destination. On 2012/08/08 01:29:57, Julien Tinnes wrote: > Same as for recvfrom: we allow sendmsg anyway. Allowing sendmsg() is pretty crazy. And yes, if you do that, allowing all the other functions doesn't add any more attack surface. And of course, none of this works on x86-32 anyway, as it uses socketcall()
https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:383: case __NR_wait4: On 2012/08/08 10:08:42, Markus (顧孟勤) wrote: > On 2012/08/08 01:29:57, Julien Tinnes wrote: > > Adds very little attack surface and it's really the right thing to do when we > > allow clone. > > I tend to agree with your assessment about the attack surface. > > But clone() is not necessarily a good justification for enabling this system > call. If we only allow clone() for creating threads, then waitXXX() is not very > useful. You have to use futexes to wait for threads. The waitXXX() functions > don't usually work for thread ids. Do you think I should put futex and the robust_list syscalls in this group ? > Also, if you enable wait4() and waitid(), please enable waitpid() where > available (i.e. x86-32) This is x86-64 only ATM. https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:489: case __NR_sendto: // Could specify destination. On 2012/08/08 10:08:42, Markus (顧孟勤) wrote: > On 2012/08/08 01:29:57, Julien Tinnes wrote: > > Same as for recvfrom: we allow sendmsg anyway. > > Allowing sendmsg() is pretty crazy. And yes, if you do that, allowing all the > other functions doesn't add any more attack surface. That might be something we'll want to broker out. In the meantime it's nice to have a more consistent policy. > And of course, none of this works on x86-32 anyway, as it uses socketcall() This is x86-64 only for now.
https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:383: case __NR_wait4: That would probably work. It's not a perfect fit, but it is definitely very close. On 2012/08/08 18:57:28, Julien Tinnes wrote: > On 2012/08/08 10:08:42, Markus (顧孟勤) wrote: > > On 2012/08/08 01:29:57, Julien Tinnes wrote: > > > Adds very little attack surface and it's really the right thing to do when > we > > > allow clone. > > > > I tend to agree with your assessment about the attack surface. > > > > But clone() is not necessarily a good justification for enabling this system > > call. If we only allow clone() for creating threads, then waitXXX() is not > very > > useful. You have to use futexes to wait for threads. The waitXXX() functions > > don't usually work for thread ids. > > Do you think I should put futex and the robust_list syscalls in this group ? > > > Also, if you enable wait4() and waitid(), please enable waitpid() where > > available (i.e. x86-32) > > This is x86-64 only ATM.
https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10837156/diff/2002/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:383: case __NR_wait4: On 2012/08/08 21:55:35, Markus (顧孟勤) wrote: > That would probably work. It's not a perfect fit, but it is definitely very > close. IsFutex is a relatively well-defined set so as a compromise I decided to add a comment and move IsFutex() just belox IsAllowedProcessStartOrDeath(). What do you think ?
lgtm
https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:213: case __NR_getdents64: getdents() is not a filesystem syscall. It takes an fd, no? https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:242: case __NR_utimensat: // New. Will this compile on Ubuntu 10.04 ? Could you quickly check everything against the newest syscall defined in 10.04 header files? https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:279: bool IsGetProcessIdSyscall(int sysno) { Not sure about the name. It's getting process ids _and_ uid/gid, etc. Maybe "IsGetSimpleIdSyscall" ? https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:356: case __NR_fcntl: Might as well add the TODO on fcntl() arg lockdown whilst we're here? https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:378: case __NR_exit_group: Just an observation: in the generic case, it'll probably be highly desirable to have exit() separated out from the other stuff. https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:437: bool IsNetworkSocketInformation(int sysno) { Did the old policies really allow all of these syscalls below? Some of them are awful. https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:442: case __NR_setsockopt: Ew, need to add a TODO to restrict get/setsockopt. https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:456: case __NR_mmap: TODO: restrict flags etc :) https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:484: case __NR_recvfrom: // Could specify source. Did you mean to enable recvfrom?
Thanks, PTAL! https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:213: case __NR_getdents64: On 2012/08/09 07:09:15, Chris Evans wrote: > getdents() is not a filesystem syscall. It takes an fd, no? Oh, wow, I missed that, thanks! https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:242: case __NR_utimensat: // New. On 2012/08/09 07:09:15, Chris Evans wrote: > Will this compile on Ubuntu 10.04 ? > Could you quickly check everything against the newest syscall defined in 10.04 > header files? This problem was getting annoying, so I solved it in a previous CL: we now have our own header that lists all system calls. https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:279: bool IsGetProcessIdSyscall(int sysno) { On 2012/08/09 07:09:15, Chris Evans wrote: > Not sure about the name. It's getting process ids _and_ uid/gid, etc. Maybe > "IsGetSimpleIdSyscall" ? Done. https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:356: case __NR_fcntl: On 2012/08/09 07:09:15, Chris Evans wrote: > Might as well add the TODO on fcntl() arg lockdown whilst we're here? Yeah, not on the top of the list but worth investigating. https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:378: case __NR_exit_group: On 2012/08/09 07:09:15, Chris Evans wrote: > Just an observation: in the generic case, it'll probably be highly desirable to > have exit() separated out from the other stuff. I would think the only one we may want to restrict that is not restricted yet would be clone, wouldn't it ? Maybe clone/fork/get_thread*/set_*/vfork below really belong somewhere else? I think wait* are harmless and should be allow even when clone isn't, after all we could have some pre-warming that forks a new process. We may even want our own helper process ((for open etc..) to be a child of the main process). I think we should probably allow the get_/set_ functions here also, but I wanted to do more research first. https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:437: bool IsNetworkSocketInformation(int sysno) { On 2012/08/09 07:09:15, Chris Evans wrote: > Did the old policies really allow all of these syscalls below? Some of them are > awful. None of them is allowed at the moment. https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:442: case __NR_setsockopt: On 2012/08/09 07:09:15, Chris Evans wrote: > Ew, need to add a TODO to restrict get/setsockopt. It's not allowed. It's in the "Watched" list. https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:456: case __NR_mmap: On 2012/08/09 07:09:15, Chris Evans wrote: > TODO: restrict flags etc :) Done. https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:484: case __NR_recvfrom: // Could specify source. On 2012/08/09 07:09:15, Chris Evans wrote: > Did you mean to enable recvfrom? There is a comment in the very first iteration of this CL, where I explain some of my choices. The rationale here was that since recvmsg/sendmsg are allowed, there is really nothing we can do here.
LGTM One future idea I had is to later add some rules which speculatively defend against kernel bugs. e.g. cap the max length passed to certain syscalls such as write(), writev(), etc. These sorts of rules would be different from "restriction" rules which exist to ban certain flags or option values. Cheers Chris On Thu, Aug 9, 2012 at 9:59 AM, <jln@chromium.org> wrote: > Thanks, PTAL! > > > > https://chromiumcodereview.**appspot.com/10837156/diff/** > 6004/content/common/sandbox_**seccomp_bpf_linux.cc<https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sandbox_seccomp_bpf_linux.cc> > File content/common/sandbox_**seccomp_bpf_linux.cc (right): > > https://chromiumcodereview.**appspot.com/10837156/diff/** > 6004/content/common/sandbox_**seccomp_bpf_linux.cc#**newcode213<https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sandbox_seccomp_bpf_linux.cc#newcode213> > content/common/sandbox_**seccomp_bpf_linux.cc:213: case __NR_getdents64: > On 2012/08/09 07:09:15, Chris Evans wrote: > >> getdents() is not a filesystem syscall. It takes an fd, no? >> > > Oh, wow, I missed that, thanks! > > > https://chromiumcodereview.**appspot.com/10837156/diff/** > 6004/content/common/sandbox_**seccomp_bpf_linux.cc#**newcode242<https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sandbox_seccomp_bpf_linux.cc#newcode242> > content/common/sandbox_**seccomp_bpf_linux.cc:242: case __NR_utimensat: > // New. > On 2012/08/09 07:09:15, Chris Evans wrote: > >> Will this compile on Ubuntu 10.04 ? >> Could you quickly check everything against the newest syscall defined >> > in 10.04 > >> header files? >> > > This problem was getting annoying, so I solved it in a previous CL: we > now have our own header that lists all system calls. > > > https://chromiumcodereview.**appspot.com/10837156/diff/** > 6004/content/common/sandbox_**seccomp_bpf_linux.cc#**newcode279<https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sandbox_seccomp_bpf_linux.cc#newcode279> > content/common/sandbox_**seccomp_bpf_linux.cc:279: bool > IsGetProcessIdSyscall(int sysno) { > On 2012/08/09 07:09:15, Chris Evans wrote: > >> Not sure about the name. It's getting process ids _and_ uid/gid, etc. >> > Maybe > >> "IsGetSimpleIdSyscall" ? >> > > Done. > > > https://chromiumcodereview.**appspot.com/10837156/diff/** > 6004/content/common/sandbox_**seccomp_bpf_linux.cc#**newcode356<https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sandbox_seccomp_bpf_linux.cc#newcode356> > content/common/sandbox_**seccomp_bpf_linux.cc:356: case __NR_fcntl: > On 2012/08/09 07:09:15, Chris Evans wrote: > >> Might as well add the TODO on fcntl() arg lockdown whilst we're here? >> > > Yeah, not on the top of the list but worth investigating. > > > https://chromiumcodereview.**appspot.com/10837156/diff/** > 6004/content/common/sandbox_**seccomp_bpf_linux.cc#**newcode378<https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sandbox_seccomp_bpf_linux.cc#newcode378> > content/common/sandbox_**seccomp_bpf_linux.cc:378: case __NR_exit_group: > On 2012/08/09 07:09:15, Chris Evans wrote: > >> Just an observation: in the generic case, it'll probably be highly >> > desirable to > >> have exit() separated out from the other stuff. >> > > I would think the only one we may want to restrict that is not > restricted yet would be clone, wouldn't it ? > > Maybe clone/fork/get_thread*/set_*/**vfork below really belong somewhere > else? > Yeah, they seem like a different capability from "exiting". Can be resolved in a later CL. > I think wait* are harmless and should be allow even when clone isn't, > after all we could have some pre-warming that forks a new process. We > may even want our own helper process ((for open etc..) to be a child of > the main process). > > I think we should probably allow the get_/set_ functions here also, but > I wanted to do more research first. > > > https://chromiumcodereview.**appspot.com/10837156/diff/** > 6004/content/common/sandbox_**seccomp_bpf_linux.cc#**newcode437<https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sandbox_seccomp_bpf_linux.cc#newcode437> > content/common/sandbox_**seccomp_bpf_linux.cc:437: bool > IsNetworkSocketInformation(int sysno) { > On 2012/08/09 07:09:15, Chris Evans wrote: > >> Did the old policies really allow all of these syscalls below? Some of >> > them are > >> awful. >> > > None of them is allowed at the moment. > > > https://chromiumcodereview.**appspot.com/10837156/diff/** > 6004/content/common/sandbox_**seccomp_bpf_linux.cc#**newcode442<https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sandbox_seccomp_bpf_linux.cc#newcode442> > content/common/sandbox_**seccomp_bpf_linux.cc:442: case __NR_setsockopt: > On 2012/08/09 07:09:15, Chris Evans wrote: > >> Ew, need to add a TODO to restrict get/setsockopt. >> > > It's not allowed. It's in the "Watched" list. > > > https://chromiumcodereview.**appspot.com/10837156/diff/** > 6004/content/common/sandbox_**seccomp_bpf_linux.cc#**newcode456<https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sandbox_seccomp_bpf_linux.cc#newcode456> > content/common/sandbox_**seccomp_bpf_linux.cc:456: case __NR_mmap: > On 2012/08/09 07:09:15, Chris Evans wrote: > >> TODO: restrict flags etc :) >> > > Done. > > > https://chromiumcodereview.**appspot.com/10837156/diff/** > 6004/content/common/sandbox_**seccomp_bpf_linux.cc#**newcode484<https://chromiumcodereview.appspot.com/10837156/diff/6004/content/common/sandbox_seccomp_bpf_linux.cc#newcode484> > content/common/sandbox_**seccomp_bpf_linux.cc:484: case __NR_recvfrom: // > Could specify source. > On 2012/08/09 07:09:15, Chris Evans wrote: > >> Did you mean to enable recvfrom? >> > > There is a comment in the very first iteration of this CL, where I > explain some of my choices. > > The rationale here was that since recvmsg/sendmsg are allowed, there is > really nothing we can do here. > I re-looked and yeah seems fine. > > https://chromiumcodereview.**appspot.com/10837156/<https://chromiumcodereview... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/10837156/11003
Change committed as 150914 |
