Re: [PATCH 1/3] fs: add O_BENEATH flag to openat(2)
On Wed, Nov 5, 2014 at 9:21 AM, David Drysdale wrote: > On Tue, Nov 4, 2014 at 9:40 AM, David Drysdale wrote: >> On Mon, Nov 3, 2014 at 5:22 PM, Eric W.Biederman >> wrote: >>> On November 3, 2014 7:42:58 AM PST, Andy Lutomirski >>> wrote: On Mon, Nov 3, 2014 at 7:20 AM, Al Viro wrote: > On Mon, Nov 03, 2014 at 11:48:23AM +, David Drysdale wrote: >> Add a new O_BENEATH flag for openat(2) which restricts the >> provided path, rejecting (with -EACCES) paths that are not beneath >> the provided dfd. In particular, reject: >> - paths that contain .. components >> - paths that begin with / >> - symlinks that have paths as above. > > Yecch... The degree of usefulness aside (and I'm not convinced that it > is non-zero), This is extremely useful in conjunction with seccomp. > WTF pass one bit out of nameidata->flags in a separate argument? > Through the mutual recursion, no less... And then you are not even attempting > to detect symlinks that are not followed by interpretation of _any_ pathname. How many symlinks like that are there? Is there anything except nd_jump_link users? All of those are in /proc. Arguably O_BENEATH should prevent traversal of all of those links. >>> >>> Not commenting on the sanity of this one way or another, and I haven't read >>> the patch. There is an absolutely trivial implementation of this. >>> >>> After the path is resolved, walk backwards along d_parent and the mount >>> tree, and see if you come to the file or directory dfd refers to. >>> >>> That can handle magic proc symlinks, and does not need to disallow .. or / >>> explicitly so it should be much simpler code. >>> >>> My gut says that if Al says blech when looking at your code it is too >>> complex to give you a security guarantee. >>> >>> Eric >> >> Well, the 'yecch' was deserved for the unnecessary duplication of the >> flags. Without that, the patch looks much simpler -- I'll send out a v2 >> with those changes for discussion, and think about your alternative >> implementation suggestion (thanks!) separately. > > One concern with the "walk upwards and see if you get back where you > started" approach -- it will allow use of a symlink that lives outside the > original directory, but which points back inside it. That's going to be > slightly surprising behaviour for users, and I worry that there's the > potential for unexpected information leakage from it. > > (BTW, size-wise my initial naive implementation of the walk-upward > approach is only marginally smaller than the v2 patch.) It has another problem. Since we still haven't fixed the eternal /proc/PID/fd-doesn't-respect-file-mode issue, you can have a read-only fd somewhere and reopen it read-write using O_BENEATH on a different fd. For example: fd 3 points to /sandbox /sandbox/blocked has mode 0700 and isn't owned by us /sandbox/blocked/foo has mode 0666 fd 4 points to /sandbox/blocked/foo, O_RDONLY openat(3, "/proc/self/fd/4", O_RDWR | O_BENEATH) will get a read-write descriptor pointing at /sandbox/blocked/foo, which should have been impossible. Also, I really don't like the information leak. The result of asking a server for "/home/victim/compromising-directory/../../www/index.html" should not reveal whether compromising-directory exists. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] fs: add O_BENEATH flag to openat(2)
On Tue, Nov 4, 2014 at 9:40 AM, David Drysdale wrote: > On Mon, Nov 3, 2014 at 5:22 PM, Eric W.Biederman > wrote: >> On November 3, 2014 7:42:58 AM PST, Andy Lutomirski >> wrote: >>>On Mon, Nov 3, 2014 at 7:20 AM, Al Viro >>>wrote: On Mon, Nov 03, 2014 at 11:48:23AM +, David Drysdale wrote: > Add a new O_BENEATH flag for openat(2) which restricts the > provided path, rejecting (with -EACCES) paths that are not beneath > the provided dfd. In particular, reject: > - paths that contain .. components > - paths that begin with / > - symlinks that have paths as above. Yecch... The degree of usefulness aside (and I'm not convinced that >>>it is non-zero), >>> >>>This is extremely useful in conjunction with seccomp. >>> WTF pass one bit out of nameidata->flags in a separate argument? Through the mutual recursion, no less... And then you are not even >>>attempting to detect symlinks that are not followed by interpretation of _any_ >>>pathname. >>> >>>How many symlinks like that are there? Is there anything except >>>nd_jump_link users? All of those are in /proc. Arguably O_BENEATH >>>should prevent traversal of all of those links. >> >> Not commenting on the sanity of this one way or another, and I haven't read >> the patch. There is an absolutely trivial implementation of this. >> >> After the path is resolved, walk backwards along d_parent and the mount >> tree, and see if you come to the file or directory dfd refers to. >> >> That can handle magic proc symlinks, and does not need to disallow .. or / >> explicitly so it should be much simpler code. >> >> My gut says that if Al says blech when looking at your code it is too >> complex to give you a security guarantee. >> >> Eric > > Well, the 'yecch' was deserved for the unnecessary duplication of the > flags. Without that, the patch looks much simpler -- I'll send out a v2 > with those changes for discussion, and think about your alternative > implementation suggestion (thanks!) separately. One concern with the "walk upwards and see if you get back where you started" approach -- it will allow use of a symlink that lives outside the original directory, but which points back inside it. That's going to be slightly surprising behaviour for users, and I worry that there's the potential for unexpected information leakage from it. (BTW, size-wise my initial naive implementation of the walk-upward approach is only marginally smaller than the v2 patch.) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] fs: add O_BENEATH flag to openat(2)
On Mon, Nov 3, 2014 at 5:22 PM, Eric W.Biederman wrote: > On November 3, 2014 7:42:58 AM PST, Andy Lutomirski > wrote: >>On Mon, Nov 3, 2014 at 7:20 AM, Al Viro >>wrote: >>> On Mon, Nov 03, 2014 at 11:48:23AM +, David Drysdale wrote: Add a new O_BENEATH flag for openat(2) which restricts the provided path, rejecting (with -EACCES) paths that are not beneath the provided dfd. In particular, reject: - paths that contain .. components - paths that begin with / - symlinks that have paths as above. >>> >>> Yecch... The degree of usefulness aside (and I'm not convinced that >>it >>> is non-zero), >> >>This is extremely useful in conjunction with seccomp. >> >>> WTF pass one bit out of nameidata->flags in a separate argument? >>> Through the mutual recursion, no less... And then you are not even >>attempting >>> to detect symlinks that are not followed by interpretation of _any_ >>pathname. >> >>How many symlinks like that are there? Is there anything except >>nd_jump_link users? All of those are in /proc. Arguably O_BENEATH >>should prevent traversal of all of those links. > > Not commenting on the sanity of this one way or another, and I haven't read > the patch. There is an absolutely trivial implementation of this. > > After the path is resolved, walk backwards along d_parent and the mount tree, > and see if you come to the file or directory dfd refers to. > > That can handle magic proc symlinks, and does not need to disallow .. or / > explicitly so it should be much simpler code. > > My gut says that if Al says blech when looking at your code it is too complex > to give you a security guarantee. > > Eric Well, the 'yecch' was deserved for the unnecessary duplication of the flags. Without that, the patch looks much simpler -- I'll send out a v2 with those changes for discussion, and think about your alternative implementation suggestion (thanks!) separately. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] fs: add O_BENEATH flag to openat(2)
On Mon, Nov 3, 2014 at 10:25 AM, Julien Tinnes wrote: > On Mon, Nov 3, 2014 at 9:37 AM, David Drysdale wrote: >> >> On Mon, Nov 3, 2014 at 3:42 PM, Andy Lutomirski >> wrote: >> > On Mon, Nov 3, 2014 at 7:20 AM, Al Viro wrote: >> >> On Mon, Nov 03, 2014 at 11:48:23AM +, David Drysdale wrote: >> >>> Add a new O_BENEATH flag for openat(2) which restricts the >> >>> provided path, rejecting (with -EACCES) paths that are not beneath >> >>> the provided dfd. In particular, reject: >> >>> - paths that contain .. components >> >>> - paths that begin with / >> >>> - symlinks that have paths as above. >> >> >> >> Yecch... The degree of usefulness aside (and I'm not convinced that it >> >> is non-zero), >> > >> > This is extremely useful in conjunction with seccomp. >> >> Yes, that was my understanding of how the Chrome[OS] folk wanted >> to use it. > > > Yes, exactly. Without this, if we want to give a sandboxed process A access > to a directory, we need to: > 1. Create a new 'broker" process B > 2. Make sure to have an IPC channel between A and B. > 3. SIGSYS open() and openat() in A via seccomp-bpf > 4. Have an async-signal-safe handler that can IPC open / openat. You can do this with user namespaces, too. But this is way more complicated than it should be, and it has a lot more overhead. --Andy > > There is a lot of hidden complexity in such a set-up. For instance, if you > need to prevent contention, the number of threads in the broker B should > scale automatically. > > This is 'fine' (but undesirable) for a big beast such as Chromium which > needs such a complex set-ups anyways, but David's patch would make it a lot > easier to build a sandbox and whitelist directories for everyone, simply by > enforcing O_BENEATH in seccomp and whitelisting open directory file > descriptors in the sandboxed process. > > Julien -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] fs: add O_BENEATH flag to openat(2)
On Mon, Nov 3, 2014 at 9:37 AM, David Drysdale wrote: > On Mon, Nov 3, 2014 at 3:42 PM, Andy Lutomirski wrote: >> This is extremely useful in conjunction with seccomp. > > Yes, that was my understanding of how the Chrome[OS] folk wanted > to use it. Yes, exactly. Without this, if we want to give a sandboxed process A access to a directory, we need to: 1. Create a new 'broker" process B 2. Make sure to have an IPC channel between A and B. 3. SIGSYS open() and openat() in A via seccomp-bpf 4. Have an async-signal-safe handler that can IPC open / openat. There is a lot of hidden complexity in such a set-up. For instance, if you need to prevent contention, the number of threads in the broker B should scale automatically. This is 'fine' (but undesirable) for a big beast such as Chromium which needs such a complex set-ups anyways, but David's patch would make it a lot easier to build a sandbox and whitelist directories for everyone, simply by enforcing O_BENEATH in seccomp and whitelisting open directory file descriptors in the sandboxed process. Julien -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] fs: add O_BENEATH flag to openat(2)
On Mon, Nov 3, 2014 at 3:42 PM, Andy Lutomirski wrote: > On Mon, Nov 3, 2014 at 7:20 AM, Al Viro wrote: >> On Mon, Nov 03, 2014 at 11:48:23AM +, David Drysdale wrote: >>> Add a new O_BENEATH flag for openat(2) which restricts the >>> provided path, rejecting (with -EACCES) paths that are not beneath >>> the provided dfd. In particular, reject: >>> - paths that contain .. components >>> - paths that begin with / >>> - symlinks that have paths as above. >> >> Yecch... The degree of usefulness aside (and I'm not convinced that it >> is non-zero), > > This is extremely useful in conjunction with seccomp. Yes, that was my understanding of how the Chrome[OS] folk wanted to use it. >> WTF pass one bit out of nameidata->flags in a separate argument? I'll shift to using nd->flags; not sure what I was thinking of there. (It *might* have made more sense in the full patchset this was extracted from but it certainly doesn't look sensible in this narrower context.) >> Through the mutual recursion, no less... And then you are not even >> attempting >> to detect symlinks that are not followed by interpretation of _any_ pathname. > > How many symlinks like that are there? Is there anything except > nd_jump_link users? All of those are in /proc. Arguably O_BENEATH > should prevent traversal of all of those links. > > --Andy On a quick search, the 2 users of nd_jump_link (namely proc_pid_follow_link and proc_ns_follow_link) seem to be the only implementations of inode_operations->follow_link that don't just call nd_set_link(). So disallowing that for O_BENEATH might give sensible behaviour. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] fs: add O_BENEATH flag to openat(2)
On November 3, 2014 7:42:58 AM PST, Andy Lutomirski wrote: >On Mon, Nov 3, 2014 at 7:20 AM, Al Viro >wrote: >> On Mon, Nov 03, 2014 at 11:48:23AM +, David Drysdale wrote: >>> Add a new O_BENEATH flag for openat(2) which restricts the >>> provided path, rejecting (with -EACCES) paths that are not beneath >>> the provided dfd. In particular, reject: >>> - paths that contain .. components >>> - paths that begin with / >>> - symlinks that have paths as above. >> >> Yecch... The degree of usefulness aside (and I'm not convinced that >it >> is non-zero), > >This is extremely useful in conjunction with seccomp. > >> WTF pass one bit out of nameidata->flags in a separate argument? >> Through the mutual recursion, no less... And then you are not even >attempting >> to detect symlinks that are not followed by interpretation of _any_ >pathname. > >How many symlinks like that are there? Is there anything except >nd_jump_link users? All of those are in /proc. Arguably O_BENEATH >should prevent traversal of all of those links. Not commenting on the sanity of this one way or another, and I haven't read the patch. There is an absolutely trivial implementation of this. After the path is resolved, walk backwards along d_parent and the mount tree, and see if you come to the file or directory dfd refers to. That can handle magic proc symlinks, and does not need to disallow .. or / explicitly so it should be much simpler code. My gut says that if Al says blech when looking at your code it is too complex to give you a security guarantee. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] fs: add O_BENEATH flag to openat(2)
On Mon, Nov 3, 2014 at 7:20 AM, Al Viro wrote: > On Mon, Nov 03, 2014 at 11:48:23AM +, David Drysdale wrote: >> Add a new O_BENEATH flag for openat(2) which restricts the >> provided path, rejecting (with -EACCES) paths that are not beneath >> the provided dfd. In particular, reject: >> - paths that contain .. components >> - paths that begin with / >> - symlinks that have paths as above. > > Yecch... The degree of usefulness aside (and I'm not convinced that it > is non-zero), This is extremely useful in conjunction with seccomp. > WTF pass one bit out of nameidata->flags in a separate argument? > Through the mutual recursion, no less... And then you are not even attempting > to detect symlinks that are not followed by interpretation of _any_ pathname. How many symlinks like that are there? Is there anything except nd_jump_link users? All of those are in /proc. Arguably O_BENEATH should prevent traversal of all of those links. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] fs: add O_BENEATH flag to openat(2)
On Mon, Nov 03, 2014 at 11:48:23AM +, David Drysdale wrote: > Add a new O_BENEATH flag for openat(2) which restricts the > provided path, rejecting (with -EACCES) paths that are not beneath > the provided dfd. In particular, reject: > - paths that contain .. components > - paths that begin with / > - symlinks that have paths as above. Yecch... The degree of usefulness aside (and I'm not convinced that it is non-zero), WTF pass one bit out of nameidata->flags in a separate argument? Through the mutual recursion, no less... And then you are not even attempting to detect symlinks that are not followed by interpretation of _any_ pathname. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
