* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <20150124031544.GA1992748@mail.thefacebook.com> @ 2015-01-26 12:47 ` Kirill A. Shutemov [not found] ` <20150126124731.GA26916-nhfs4B5ZimeFUdmeq17FyvUpdFzICT1y@public.gmane.org> 0 siblings, 1 reply; 19+ messages in thread From: Kirill A. Shutemov @ 2015-01-26 12:47 UTC (permalink / raw) To: Calvin Owens Cc: Cyrill Gorcunov, Andrew Morton, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, linux-kernel, kernel-team, Pavel Emelyanov, linux-api On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and > is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface > is very useful for enumerating the files mapped into a process when > the more verbose information in /proc/<pid>/maps is not needed. > > This patch moves the folder out from behind CHECKPOINT_RESTORE, and > removes the CAP_SYS_ADMIN restrictions. Following the links requires > the ability to ptrace the process in question, so this doesn't allow > an attacker to do anything they couldn't already do before. > > Signed-off-by: Calvin Owens <calvinowens@fb.com> Cc +linux-api@ > --- > Changes in v2: Removed the follow_link() stub that returned -EPERM if > the caller didn't have CAP_SYS_ADMIN, since the caller > in my chroot() scenario gets -EACCES anyway. > > fs/proc/base.c | 18 ------------------ > 1 file changed, 18 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 3f3d7ae..67b15ac 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -1632,8 +1632,6 @@ end_instantiate: > return dir_emit(ctx, name, len, 1, DT_UNKNOWN); > } > > -#ifdef CONFIG_CHECKPOINT_RESTORE > - > /* > * dname_to_vma_addr - maps a dentry name into two unsigned longs > * which represent vma start and end addresses. > @@ -1660,11 +1658,6 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags) > if (flags & LOOKUP_RCU) > return -ECHILD; > > - if (!capable(CAP_SYS_ADMIN)) { > - status = -EPERM; > - goto out_notask; > - } > - > inode = dentry->d_inode; > task = get_proc_task(inode); > if (!task) > @@ -1792,10 +1785,6 @@ static struct dentry *proc_map_files_lookup(struct inode *dir, > int result; > struct mm_struct *mm; > > - result = -EPERM; > - if (!capable(CAP_SYS_ADMIN)) > - goto out; > - > result = -ENOENT; > task = get_proc_task(dir); > if (!task) > @@ -1849,10 +1838,6 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx) > struct map_files_info *p; > int ret; > > - ret = -EPERM; > - if (!capable(CAP_SYS_ADMIN)) > - goto out; > - > ret = -ENOENT; > task = get_proc_task(file_inode(file)); > if (!task) > @@ -2040,7 +2025,6 @@ static const struct file_operations proc_timers_operations = { > .llseek = seq_lseek, > .release = seq_release_private, > }; > -#endif /* CONFIG_CHECKPOINT_RESTORE */ > > static int proc_pident_instantiate(struct inode *dir, > struct dentry *dentry, struct task_struct *task, const void *ptr) > @@ -2537,9 +2521,7 @@ static const struct inode_operations proc_task_inode_operations; > static const struct pid_entry tgid_base_stuff[] = { > DIR("task", S_IRUGO|S_IXUGO, proc_task_inode_operations, proc_task_operations), > DIR("fd", S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations), > -#ifdef CONFIG_CHECKPOINT_RESTORE > DIR("map_files", S_IRUSR|S_IXUSR, proc_map_files_inode_operations, proc_map_files_operations), > -#endif > DIR("fdinfo", S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations), > DIR("ns", S_IRUSR|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations), > #ifdef CONFIG_NET > -- > 1.8.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20150126124731.GA26916-nhfs4B5ZimeFUdmeq17FyvUpdFzICT1y@public.gmane.org>]
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <20150126124731.GA26916-nhfs4B5ZimeFUdmeq17FyvUpdFzICT1y@public.gmane.org> @ 2015-01-26 21:00 ` Cyrill Gorcunov 2015-01-26 23:43 ` Andrew Morton 0 siblings, 1 reply; 19+ messages in thread From: Cyrill Gorcunov @ 2015-01-26 21:00 UTC (permalink / raw) To: Kirill A. Shutemov Cc: Calvin Owens, Andrew Morton, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, linux-kernel-u79uwXL29TY76Z2rM5mHXA, kernel-team-b10kYP2dOMg, Pavel Emelyanov, linux-api-u79uwXL29TY76Z2rM5mHXA On Mon, Jan 26, 2015 at 02:47:31PM +0200, Kirill A. Shutemov wrote: > On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: > > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and > > is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface > > is very useful for enumerating the files mapped into a process when > > the more verbose information in /proc/<pid>/maps is not needed. > > > > This patch moves the folder out from behind CHECKPOINT_RESTORE, and > > removes the CAP_SYS_ADMIN restrictions. Following the links requires > > the ability to ptrace the process in question, so this doesn't allow > > an attacker to do anything they couldn't already do before. > > > > Signed-off-by: Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> > > Cc +linux-api@ Looks good to me, thanks! Though I would really appreciate if someone from security camp take a look as well. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable 2015-01-26 21:00 ` Cyrill Gorcunov @ 2015-01-26 23:43 ` Andrew Morton [not found] ` <20150126154346.c63c512e5821e9e0ea31f759-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> 2015-01-28 4:38 ` Calvin Owens 0 siblings, 2 replies; 19+ messages in thread From: Andrew Morton @ 2015-01-26 23:43 UTC (permalink / raw) To: Cyrill Gorcunov Cc: Kirill A. Shutemov, Calvin Owens, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, linux-kernel, kernel-team, Pavel Emelyanov, linux-api, Kees Cook On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <gorcunov@gmail.com> wrote: > On Mon, Jan 26, 2015 at 02:47:31PM +0200, Kirill A. Shutemov wrote: > > On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: > > > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and > > > is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface > > > is very useful for enumerating the files mapped into a process when > > > the more verbose information in /proc/<pid>/maps is not needed. This is the main (actually only) justification for the patch, and it it far too thin. What does "not needed" mean. Why can't people just use /proc/pid/maps? > > > This patch moves the folder out from behind CHECKPOINT_RESTORE, and > > > removes the CAP_SYS_ADMIN restrictions. Following the links requires > > > the ability to ptrace the process in question, so this doesn't allow > > > an attacker to do anything they couldn't already do before. > > > > > > Signed-off-by: Calvin Owens <calvinowens@fb.com> > > > > Cc +linux-api@ > > Looks good to me, thanks! Though I would really appreciate if someone > from security camp take a look as well. hm, who's that. Kees comes to mind. And reviewers' task would be a heck of a lot easier if they knew what /proc/pid/map_files actually does. This: akpm3:/usr/src/25> grep -r map_files Documentation akpm3:/usr/src/25> does not help. The 640708a2cff7f81 changelog says: : This one behaves similarly to the /proc/<pid>/fd/ one - it contains : symlinks one for each mapping with file, the name of a symlink is : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink : results in a file that point exactly to the same inode as them vma's one. : : For example the ls -l of some arbitrary /proc/<pid>/map_files/ : : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so afacit this info is also available in /proc/pid/maps, so things shouldn't get worse if the /proc/pid/map_files permissions are at least as restrictive as the /proc/pid/maps permissions. Is that the case? (Please add to changelog). There's one other problem here: we're assuming that the map_files implementation doesn't have bugs. If it does have bugs then relaxing permissions like this will create new vulnerabilities. And the map_files implementation is surprisingly complex. Is it bug-free? ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20150126154346.c63c512e5821e9e0ea31f759-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>]
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <20150126154346.c63c512e5821e9e0ea31f759-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> @ 2015-01-27 0:15 ` Kees Cook [not found] ` <CAGXu5jLDkg0hJSMm3CdoO-77yiK5GQWHSe3+1h7mq76LERpNBA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-01-27 0:19 ` Kirill A. Shutemov 2015-01-27 6:46 ` Cyrill Gorcunov 2 siblings, 1 reply; 19+ messages in thread From: Kees Cook @ 2015-01-27 0:15 UTC (permalink / raw) To: Andrew Morton Cc: Cyrill Gorcunov, Kirill A. Shutemov, Calvin Owens, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, LKML, kernel-team-b10kYP2dOMg, Pavel Emelyanov, Linux API On Mon, Jan 26, 2015 at 3:43 PM, Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote: > On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <gorcunov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > >> On Mon, Jan 26, 2015 at 02:47:31PM +0200, Kirill A. Shutemov wrote: >> > On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: >> > > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and >> > > is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface >> > > is very useful for enumerating the files mapped into a process when >> > > the more verbose information in /proc/<pid>/maps is not needed. > > This is the main (actually only) justification for the patch, and it it > far too thin. What does "not needed" mean. Why can't people just use > /proc/pid/maps? > >> > > This patch moves the folder out from behind CHECKPOINT_RESTORE, and >> > > removes the CAP_SYS_ADMIN restrictions. Following the links requires >> > > the ability to ptrace the process in question, so this doesn't allow >> > > an attacker to do anything they couldn't already do before. >> > > >> > > Signed-off-by: Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> >> > >> > Cc +linux-api@ >> >> Looks good to me, thanks! Though I would really appreciate if someone >> from security camp take a look as well. > > hm, who's that. Kees comes to mind. > > And reviewers' task would be a heck of a lot easier if they knew what > /proc/pid/map_files actually does. This: > > akpm3:/usr/src/25> grep -r map_files Documentation If akpm's comments weren't clear: this needs to be fixed. Everything in /proc should appear in Documentation. > akpm3:/usr/src/25> > > does not help. > > The 640708a2cff7f81 changelog says: > > : This one behaves similarly to the /proc/<pid>/fd/ one - it contains > : symlinks one for each mapping with file, the name of a symlink is > : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink > : results in a file that point exactly to the same inode as them vma's one. > : > : For example the ls -l of some arbitrary /proc/<pid>/map_files/ > : > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so How is mmap offset represented in this output? > > afacit this info is also available in /proc/pid/maps, so things > shouldn't get worse if the /proc/pid/map_files permissions are at least > as restrictive as the /proc/pid/maps permissions. Is that the case? > (Please add to changelog). Both maps and map_files uses ptrace_may_access (via mm_acces) with PTRACE_MODE_READ, so I'm happy from a info leak perspective. Are mount namespaces handled in this output? > There's one other problem here: we're assuming that the map_files > implementation doesn't have bugs. If it does have bugs then relaxing > permissions like this will create new vulnerabilities. And the > map_files implementation is surprisingly complex. Is it bug-free? -Kees -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CAGXu5jLDkg0hJSMm3CdoO-77yiK5GQWHSe3+1h7mq76LERpNBA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <CAGXu5jLDkg0hJSMm3CdoO-77yiK5GQWHSe3+1h7mq76LERpNBA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-01-27 7:37 ` Cyrill Gorcunov 2015-01-27 19:53 ` Kees Cook 0 siblings, 1 reply; 19+ messages in thread From: Cyrill Gorcunov @ 2015-01-27 7:37 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Kirill A. Shutemov, Calvin Owens, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, LKML, kernel-team-b10kYP2dOMg, Pavel Emelyanov, Linux API On Mon, Jan 26, 2015 at 04:15:26PM -0800, Kees Cook wrote: > > > > akpm3:/usr/src/25> grep -r map_files Documentation > > If akpm's comments weren't clear: this needs to be fixed. Everything > in /proc should appear in Documentation. I'll do that. > > The 640708a2cff7f81 changelog says: > > > > : This one behaves similarly to the /proc/<pid>/fd/ one - it contains > > : symlinks one for each mapping with file, the name of a symlink is > > : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink > > : results in a file that point exactly to the same inode as them vma's one. > > : > > : For example the ls -l of some arbitrary /proc/<pid>/map_files/ > > : > > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so > > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 > > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 > > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so > > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so > > How is mmap offset represented in this output? We're printing vm_area_struct:[vm_start;vm_end] only. > > afacit this info is also available in /proc/pid/maps, so things > > shouldn't get worse if the /proc/pid/map_files permissions are at least > > as restrictive as the /proc/pid/maps permissions. Is that the case? > > (Please add to changelog). > > Both maps and map_files uses ptrace_may_access (via mm_acces) with > PTRACE_MODE_READ, so I'm happy from a info leak perspective. > > Are mount namespaces handled in this output? Could you clarify this moment, i'm not sure i get it. > > > There's one other problem here: we're assuming that the map_files > > implementation doesn't have bugs. If it does have bugs then relaxing > > permissions like this will create new vulnerabilities. And the > > map_files implementation is surprisingly complex. Is it bug-free? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable 2015-01-27 7:37 ` Cyrill Gorcunov @ 2015-01-27 19:53 ` Kees Cook [not found] ` <CAGXu5jJFFib7F7uKYgvX4ecyMnbincd22FaO_bFy=VRVKdFbvA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-01-27 21:46 ` Pavel Emelyanov 0 siblings, 2 replies; 19+ messages in thread From: Kees Cook @ 2015-01-27 19:53 UTC (permalink / raw) To: Cyrill Gorcunov Cc: Andrew Morton, Kirill A. Shutemov, Calvin Owens, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, LKML, kernel-team-b10kYP2dOMg, Pavel Emelyanov, Linux API On Mon, Jan 26, 2015 at 11:37 PM, Cyrill Gorcunov <gorcunov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > On Mon, Jan 26, 2015 at 04:15:26PM -0800, Kees Cook wrote: >> > >> > akpm3:/usr/src/25> grep -r map_files Documentation >> >> If akpm's comments weren't clear: this needs to be fixed. Everything >> in /proc should appear in Documentation. > > I'll do that. > >> > The 640708a2cff7f81 changelog says: >> > >> > : This one behaves similarly to the /proc/<pid>/fd/ one - it contains >> > : symlinks one for each mapping with file, the name of a symlink is >> > : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink >> > : results in a file that point exactly to the same inode as them vma's one. >> > : >> > : For example the ls -l of some arbitrary /proc/<pid>/map_files/ >> > : >> > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so >> > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 >> > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 >> > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so >> > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so >> >> How is mmap offset represented in this output? > > We're printing vm_area_struct:[vm_start;vm_end] only. > >> > afacit this info is also available in /proc/pid/maps, so things >> > shouldn't get worse if the /proc/pid/map_files permissions are at least >> > as restrictive as the /proc/pid/maps permissions. Is that the case? >> > (Please add to changelog). >> >> Both maps and map_files uses ptrace_may_access (via mm_acces) with >> PTRACE_MODE_READ, so I'm happy from a info leak perspective. >> >> Are mount namespaces handled in this output? > > Could you clarify this moment, i'm not sure i get it. I changed how I asked this question in my review of the documentation, but it looks like these symlinks aren't "regular" symlinks (that are up to the follower to have access to the file system path shown), but rather they bypass VFS. As a result, I'm wondering how things like mount namespaces might change this behavior: what is shown, the path from the perspective of the target, or from the viewer (which may be in separate mount namespaces). -Kees > >> >> > There's one other problem here: we're assuming that the map_files >> > implementation doesn't have bugs. If it does have bugs then relaxing >> > permissions like this will create new vulnerabilities. And the >> > map_files implementation is surprisingly complex. Is it bug-free? -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CAGXu5jJFFib7F7uKYgvX4ecyMnbincd22FaO_bFy=VRVKdFbvA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <CAGXu5jJFFib7F7uKYgvX4ecyMnbincd22FaO_bFy=VRVKdFbvA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-01-27 21:35 ` Cyrill Gorcunov 0 siblings, 0 replies; 19+ messages in thread From: Cyrill Gorcunov @ 2015-01-27 21:35 UTC (permalink / raw) To: Kees Cook, Pavel Emelyanov Cc: Andrew Morton, Kirill A. Shutemov, Calvin Owens, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, LKML, kernel-team-b10kYP2dOMg, Linux API On Tue, Jan 27, 2015 at 11:53:19AM -0800, Kees Cook wrote: > >> > >> Are mount namespaces handled in this output? > > > > Could you clarify this moment, i'm not sure i get it. > > I changed how I asked this question in my review of the documentation, > but it looks like these symlinks aren't "regular" symlinks (that are > up to the follower to have access to the file system path shown), but > rather they bypass VFS. As a result, I'm wondering how things like > mount namespaces might change this behavior: what is shown, the path > from the perspective of the target, or from the viewer (which may be > in separate mount namespaces). I must admit I personally didn't investigating how mount namespaces might itercat with map-files. Pavel, could you share the thoughts? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable 2015-01-27 19:53 ` Kees Cook [not found] ` <CAGXu5jJFFib7F7uKYgvX4ecyMnbincd22FaO_bFy=VRVKdFbvA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-01-27 21:46 ` Pavel Emelyanov 1 sibling, 0 replies; 19+ messages in thread From: Pavel Emelyanov @ 2015-01-27 21:46 UTC (permalink / raw) To: Kees Cook, Cyrill Gorcunov Cc: Andrew Morton, Kirill A. Shutemov, Calvin Owens, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, LKML, kernel-team, Pavel Emelyanov, Linux API >>> Are mount namespaces handled in this output? >> >> Could you clarify this moment, i'm not sure i get it. > > I changed how I asked this question in my review of the documentation, > but it looks like these symlinks aren't "regular" symlinks (that are > up to the follower to have access to the file system path shown), but > rather they bypass VFS. As a result, I'm wondering how things like > mount namespaces might change this behavior: what is shown, the path > from the perspective of the target, or from the viewer (which may be > in separate mount namespaces). These work just like the /proc/$pid/fd/$n links do. When you readlink on it the d_path() is called which walks up the dentry/vfsmnt tree until it reaches either current root or the global one. For "another" mount namespace case it produces the path relative to this namespace's root. Thanks, Pavel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <20150126154346.c63c512e5821e9e0ea31f759-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> 2015-01-27 0:15 ` Kees Cook @ 2015-01-27 0:19 ` Kirill A. Shutemov 2015-01-27 6:46 ` Cyrill Gorcunov 2 siblings, 0 replies; 19+ messages in thread From: Kirill A. Shutemov @ 2015-01-27 0:19 UTC (permalink / raw) To: Andrew Morton Cc: Cyrill Gorcunov, Calvin Owens, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, linux-kernel-u79uwXL29TY76Z2rM5mHXA, kernel-team-b10kYP2dOMg, Pavel Emelyanov, linux-api-u79uwXL29TY76Z2rM5mHXA, Kees Cook On Mon, Jan 26, 2015 at 03:43:46PM -0800, Andrew Morton wrote: > On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <gorcunov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > > > On Mon, Jan 26, 2015 at 02:47:31PM +0200, Kirill A. Shutemov wrote: > > > On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: > > > > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and > > > > is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface > > > > is very useful for enumerating the files mapped into a process when > > > > the more verbose information in /proc/<pid>/maps is not needed. > > This is the main (actually only) justification for the patch, and it it > far too thin. What does "not needed" mean. Why can't people just use > /proc/pid/maps? > > > > > This patch moves the folder out from behind CHECKPOINT_RESTORE, and > > > > removes the CAP_SYS_ADMIN restrictions. Following the links requires > > > > the ability to ptrace the process in question, so this doesn't allow > > > > an attacker to do anything they couldn't already do before. > > > > > > > > Signed-off-by: Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> > > > > > > Cc +linux-api@ > > > > Looks good to me, thanks! Though I would really appreciate if someone > > from security camp take a look as well. > > hm, who's that. Kees comes to mind. > > And reviewers' task would be a heck of a lot easier if they knew what > /proc/pid/map_files actually does. This: > > akpm3:/usr/src/25> grep -r map_files Documentation > akpm3:/usr/src/25> > > does not help. > > The 640708a2cff7f81 changelog says: > > : This one behaves similarly to the /proc/<pid>/fd/ one - it contains > : symlinks one for each mapping with file, the name of a symlink is > : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink > : results in a file that point exactly to the same inode as them vma's one. > : > : For example the ls -l of some arbitrary /proc/<pid>/map_files/ > : > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so > > afacit this info is also available in /proc/pid/maps, so things > shouldn't get worse if the /proc/pid/map_files permissions are at least > as restrictive as the /proc/pid/maps permissions. Is that the case? Almost. IIUC, before we haven't had a way to retrieve a file descriptor from mapped file if it was closed and not accessible for direct re-open. Like in chroot case or unlink after close. I'm not sure what security implications this move has, if any. I don't see anything obviously dangerous. -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <20150126154346.c63c512e5821e9e0ea31f759-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> 2015-01-27 0:15 ` Kees Cook 2015-01-27 0:19 ` Kirill A. Shutemov @ 2015-01-27 6:46 ` Cyrill Gorcunov 2015-01-27 6:50 ` Andrew Morton 2 siblings, 1 reply; 19+ messages in thread From: Cyrill Gorcunov @ 2015-01-27 6:46 UTC (permalink / raw) To: Andrew Morton Cc: Kirill A. Shutemov, Calvin Owens, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, linux-kernel-u79uwXL29TY76Z2rM5mHXA, kernel-team-b10kYP2dOMg, Pavel Emelyanov, linux-api-u79uwXL29TY76Z2rM5mHXA, Kees Cook On Mon, Jan 26, 2015 at 03:43:46PM -0800, Andrew Morton wrote: > > > > Looks good to me, thanks! Though I would really appreciate if someone > > from security camp take a look as well. > > hm, who's that. Kees comes to mind. yup, I managed to forget CC him. > > And reviewers' task would be a heck of a lot easier if they knew what > /proc/pid/map_files actually does. This: > > akpm3:/usr/src/25> grep -r map_files Documentation > akpm3:/usr/src/25> > > does not help. Sigh. Imagine, for some reason I though we've the docs for that entry, probably i though that way because of many fdinfo snippets i've putted into /proc docs. my bad, sorry. I'll try to prepare docs today. > The 640708a2cff7f81 changelog says: > > : This one behaves similarly to the /proc/<pid>/fd/ one - it contains > : symlinks one for each mapping with file, the name of a symlink is > : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink > : results in a file that point exactly to the same inode as them vma's one. > : > : For example the ls -l of some arbitrary /proc/<pid>/map_files/ > : > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so > > afacit this info is also available in /proc/pid/maps, so things > shouldn't get worse if the /proc/pid/map_files permissions are at least > as restrictive as the /proc/pid/maps permissions. Is that the case? > (Please add to changelog). > > There's one other problem here: we're assuming that the map_files > implementation doesn't have bugs. If it does have bugs then relaxing > permissions like this will create new vulnerabilities. And the > map_files implementation is surprisingly complex. Is it bug-free? I didn't find any bugs in map-files (and we use it for long time already) so I think it is safe. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable 2015-01-27 6:46 ` Cyrill Gorcunov @ 2015-01-27 6:50 ` Andrew Morton [not found] ` <20150126225023.df63f6ca.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> 0 siblings, 1 reply; 19+ messages in thread From: Andrew Morton @ 2015-01-27 6:50 UTC (permalink / raw) To: Cyrill Gorcunov Cc: Kirill A. Shutemov, Calvin Owens, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, linux-kernel, kernel-team, Pavel Emelyanov, linux-api, Kees Cook On Tue, 27 Jan 2015 09:46:47 +0300 Cyrill Gorcunov <gorcunov@gmail.com> wrote: > > There's one other problem here: we're assuming that the map_files > > implementation doesn't have bugs. If it does have bugs then relaxing > > permissions like this will create new vulnerabilities. And the > > map_files implementation is surprisingly complex. Is it bug-free? > > I didn't find any bugs in map-files (and we use it for long time already) > so I think it is safe. You've been using map_files the way it was supposed to be used so no, any bugs won't show up. What happens if you don your evil black hat and use map_files in ways that weren't anticipated? Attack it? ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20150126225023.df63f6ca.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>]
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <20150126225023.df63f6ca.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> @ 2015-01-27 7:23 ` Cyrill Gorcunov 0 siblings, 0 replies; 19+ messages in thread From: Cyrill Gorcunov @ 2015-01-27 7:23 UTC (permalink / raw) To: Andrew Morton Cc: Kirill A. Shutemov, Calvin Owens, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, linux-kernel-u79uwXL29TY76Z2rM5mHXA, kernel-team-b10kYP2dOMg, Pavel Emelyanov, linux-api-u79uwXL29TY76Z2rM5mHXA, Kees Cook On Mon, Jan 26, 2015 at 10:50:23PM -0800, Andrew Morton wrote: > On Tue, 27 Jan 2015 09:46:47 +0300 Cyrill Gorcunov <gorcunov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > > > > There's one other problem here: we're assuming that the map_files > > > implementation doesn't have bugs. If it does have bugs then relaxing > > > permissions like this will create new vulnerabilities. And the > > > map_files implementation is surprisingly complex. Is it bug-free? > > > > I didn't find any bugs in map-files (and we use it for long time already) > > so I think it is safe. > > You've been using map_files the way it was supposed to be used so no, > any bugs won't show up. What happens if you don your evil black hat > and use map_files in ways that weren't anticipated? Attack it? Hard to say, Andrew. If I found a way to exploit this feature for bad purpose for sure I would patch it out. At the moment I don't see any. Touching another process memory via file descriptor allows one to modify its contents but you have to be granted ptrace-may-access which i consider as enough for security. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable 2015-01-26 23:43 ` Andrew Morton [not found] ` <20150126154346.c63c512e5821e9e0ea31f759-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> @ 2015-01-28 4:38 ` Calvin Owens [not found] ` <20150128043832.GA2266262-ZEWhMxyTXSP95iwofa7G/laTQe2KTcn/@public.gmane.org> 1 sibling, 1 reply; 19+ messages in thread From: Calvin Owens @ 2015-01-28 4:38 UTC (permalink / raw) To: Andrew Morton Cc: Cyrill Gorcunov, Kirill A. Shutemov, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, linux-kernel, kernel-team, Pavel Emelyanov, linux-api, Kees Cook On Monday 01/26 at 15:43 -0800, Andrew Morton wrote: > On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <gorcunov@gmail.com> wrote: > > > On Mon, Jan 26, 2015 at 02:47:31PM +0200, Kirill A. Shutemov wrote: > > > On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: > > > > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and > > > > is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface > > > > is very useful for enumerating the files mapped into a process when > > > > the more verbose information in /proc/<pid>/maps is not needed. > > This is the main (actually only) justification for the patch, and it it > far too thin. What does "not needed" mean. Why can't people just use > /proc/pid/maps? The biggest difference is that if you do something like this: fd = open("/stuff", O_BLAH); map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0); close(fd); unlink("/stuff"); ...then map_files/ gives you a way to get a file descriptor for "/stuff", which you couldn't do with /proc/pid/maps. It's also something of a win if you just want to see what is mapped at a specific address, since you can just readlink() the symlink for the address range you care about and it will go grab the appropriate VMA and give you the answer. /proc/pid/maps requires walking the VMA tree, which is quite expensive for processes with many thousands of threads, even without the O(N^2) issue. (You have to know what address range you want though, since readdir() on map_files/ obviously has to walk the VMA tree just like /proc/N/maps.) > > > > This patch moves the folder out from behind CHECKPOINT_RESTORE, and > > > > removes the CAP_SYS_ADMIN restrictions. Following the links requires > > > > the ability to ptrace the process in question, so this doesn't allow > > > > an attacker to do anything they couldn't already do before. > > > > > > > > Signed-off-by: Calvin Owens <calvinowens@fb.com> > > > > > > Cc +linux-api@ > > > > Looks good to me, thanks! Though I would really appreciate if someone > > from security camp take a look as well. > > hm, who's that. Kees comes to mind. > > And reviewers' task would be a heck of a lot easier if they knew what > /proc/pid/map_files actually does. This: > > akpm3:/usr/src/25> grep -r map_files Documentation > akpm3:/usr/src/25> > > does not help. > > The 640708a2cff7f81 changelog says: > > : This one behaves similarly to the /proc/<pid>/fd/ one - it contains > : symlinks one for each mapping with file, the name of a symlink is > : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink > : results in a file that point exactly to the same inode as them vma's one. > : > : For example the ls -l of some arbitrary /proc/<pid>/map_files/ > : > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so > : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so > > afacit this info is also available in /proc/pid/maps, so things > shouldn't get worse if the /proc/pid/map_files permissions are at least > as restrictive as the /proc/pid/maps permissions. Is that the case? > (Please add to changelog). Yes, the only difference is that you can follow the link as per above. I'll resend with a new message explaining that and the deletion thing. > There's one other problem here: we're assuming that the map_files > implementation doesn't have bugs. If it does have bugs then relaxing > permissions like this will create new vulnerabilities. And the > map_files implementation is surprisingly complex. Is it bug-free? While I was messing with it I used it a good bit and didn't see any issues, although I didn't actively try to fuzz it or anything. I'd be happy to write something to test hammering it in weird ways if you like. I'm also happy to write testcases for namespaces. So far as security issues, as others have pointed out you can't follow the links unless you can ptrace the process in question, which seems like a pretty solid guarantee. As Cyrill pointed out in the discussion about the documentation, that's the same protection as /proc/N/fd/*, and those links function in the same way. Thanks, Calvin ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20150128043832.GA2266262-ZEWhMxyTXSP95iwofa7G/laTQe2KTcn/@public.gmane.org>]
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <20150128043832.GA2266262-ZEWhMxyTXSP95iwofa7G/laTQe2KTcn/@public.gmane.org> @ 2015-01-30 1:30 ` Kees Cook [not found] ` <CAGXu5j+wa2-CCGaktPzDec=HF0CizP__HVVjZKcjGuJJOvLFyg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 19+ messages in thread From: Kees Cook @ 2015-01-30 1:30 UTC (permalink / raw) To: Calvin Owens Cc: Andrew Morton, Cyrill Gorcunov, Kirill A. Shutemov, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, LKML, kernel-team-b10kYP2dOMg, Pavel Emelyanov, Linux API On Tue, Jan 27, 2015 at 8:38 PM, Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> wrote: > On Monday 01/26 at 15:43 -0800, Andrew Morton wrote: >> On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <gorcunov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: >> >> > On Mon, Jan 26, 2015 at 02:47:31PM +0200, Kirill A. Shutemov wrote: >> > > On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: >> > > > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and >> > > > is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface >> > > > is very useful for enumerating the files mapped into a process when >> > > > the more verbose information in /proc/<pid>/maps is not needed. >> >> This is the main (actually only) justification for the patch, and it it >> far too thin. What does "not needed" mean. Why can't people just use >> /proc/pid/maps? > > The biggest difference is that if you do something like this: > > fd = open("/stuff", O_BLAH); > map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0); > close(fd); > unlink("/stuff"); > > ...then map_files/ gives you a way to get a file descriptor for > "/stuff", which you couldn't do with /proc/pid/maps. > > It's also something of a win if you just want to see what is mapped at a > specific address, since you can just readlink() the symlink for the > address range you care about and it will go grab the appropriate VMA and > give you the answer. /proc/pid/maps requires walking the VMA tree, which > is quite expensive for processes with many thousands of threads, even > without the O(N^2) issue. > > (You have to know what address range you want though, since readdir() on > map_files/ obviously has to walk the VMA tree just like /proc/N/maps.) > >> > > > This patch moves the folder out from behind CHECKPOINT_RESTORE, and >> > > > removes the CAP_SYS_ADMIN restrictions. Following the links requires >> > > > the ability to ptrace the process in question, so this doesn't allow >> > > > an attacker to do anything they couldn't already do before. >> > > > >> > > > Signed-off-by: Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> >> > > >> > > Cc +linux-api@ >> > >> > Looks good to me, thanks! Though I would really appreciate if someone >> > from security camp take a look as well. >> >> hm, who's that. Kees comes to mind. >> >> And reviewers' task would be a heck of a lot easier if they knew what >> /proc/pid/map_files actually does. This: >> >> akpm3:/usr/src/25> grep -r map_files Documentation >> akpm3:/usr/src/25> >> >> does not help. >> >> The 640708a2cff7f81 changelog says: >> >> : This one behaves similarly to the /proc/<pid>/fd/ one - it contains >> : symlinks one for each mapping with file, the name of a symlink is >> : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink >> : results in a file that point exactly to the same inode as them vma's one. >> : >> : For example the ls -l of some arbitrary /proc/<pid>/map_files/ >> : >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so >> >> afacit this info is also available in /proc/pid/maps, so things >> shouldn't get worse if the /proc/pid/map_files permissions are at least >> as restrictive as the /proc/pid/maps permissions. Is that the case? >> (Please add to changelog). > > Yes, the only difference is that you can follow the link as per above. > I'll resend with a new message explaining that and the deletion thing. > >> There's one other problem here: we're assuming that the map_files >> implementation doesn't have bugs. If it does have bugs then relaxing >> permissions like this will create new vulnerabilities. And the >> map_files implementation is surprisingly complex. Is it bug-free? > > While I was messing with it I used it a good bit and didn't see any > issues, although I didn't actively try to fuzz it or anything. I'd be > happy to write something to test hammering it in weird ways if you like. > I'm also happy to write testcases for namespaces. > > So far as security issues, as others have pointed out you can't follow > the links unless you can ptrace the process in question, which seems > like a pretty solid guarantee. As Cyrill pointed out in the discussion > about the documentation, that's the same protection as /proc/N/fd/*, and > those links function in the same way. My concern here is that fd/* are connected as streams, and while that has a certain level of badness as an external-to-the-process attacker, PTRACE_MODE_READ is much weaker than PTRACE_MODE_ATTACH (which is required for access to /proc/N/mem). Since these fds are the things mapped into memory on a process, writing to them is a subset of access to /proc/N/mem, and I don't feel that PTRACE_MODE_READ is sufficient. -Kees -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CAGXu5j+wa2-CCGaktPzDec=HF0CizP__HVVjZKcjGuJJOvLFyg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <CAGXu5j+wa2-CCGaktPzDec=HF0CizP__HVVjZKcjGuJJOvLFyg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-01-31 1:58 ` Calvin Owens 2015-02-02 14:01 ` Austin S Hemmelgarn 2015-02-02 20:16 ` Andy Lutomirski 0 siblings, 2 replies; 19+ messages in thread From: Calvin Owens @ 2015-01-31 1:58 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Cyrill Gorcunov, Kirill A. Shutemov, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, LKML, kernel-team-b10kYP2dOMg, Pavel Emelyanov, Linux API On Thursday 01/29 at 17:30 -0800, Kees Cook wrote: > On Tue, Jan 27, 2015 at 8:38 PM, Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> wrote: > > On Monday 01/26 at 15:43 -0800, Andrew Morton wrote: > >> On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <gorcunov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > >> > >> > On Mon, Jan 26, 2015 at 02:47:31PM +0200, Kirill A. Shutemov wrote: > >> > > On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: > >> > > > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and > >> > > > is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface > >> > > > is very useful for enumerating the files mapped into a process when > >> > > > the more verbose information in /proc/<pid>/maps is not needed. > >> > >> This is the main (actually only) justification for the patch, and it it > >> far too thin. What does "not needed" mean. Why can't people just use > >> /proc/pid/maps? > > > > The biggest difference is that if you do something like this: > > > > fd = open("/stuff", O_BLAH); > > map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0); > > close(fd); > > unlink("/stuff"); > > > > ...then map_files/ gives you a way to get a file descriptor for > > "/stuff", which you couldn't do with /proc/pid/maps. > > > > It's also something of a win if you just want to see what is mapped at a > > specific address, since you can just readlink() the symlink for the > > address range you care about and it will go grab the appropriate VMA and > > give you the answer. /proc/pid/maps requires walking the VMA tree, which > > is quite expensive for processes with many thousands of threads, even > > without the O(N^2) issue. > > > > (You have to know what address range you want though, since readdir() on > > map_files/ obviously has to walk the VMA tree just like /proc/N/maps.) > > > >> > > > This patch moves the folder out from behind CHECKPOINT_RESTORE, and > >> > > > removes the CAP_SYS_ADMIN restrictions. Following the links requires > >> > > > the ability to ptrace the process in question, so this doesn't allow > >> > > > an attacker to do anything they couldn't already do before. > >> > > > > >> > > > Signed-off-by: Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> > >> > > > >> > > Cc +linux-api@ > >> > > >> > Looks good to me, thanks! Though I would really appreciate if someone > >> > from security camp take a look as well. > >> > >> hm, who's that. Kees comes to mind. > >> > >> And reviewers' task would be a heck of a lot easier if they knew what > >> /proc/pid/map_files actually does. This: > >> > >> akpm3:/usr/src/25> grep -r map_files Documentation > >> akpm3:/usr/src/25> > >> > >> does not help. > >> > >> The 640708a2cff7f81 changelog says: > >> > >> : This one behaves similarly to the /proc/<pid>/fd/ one - it contains > >> : symlinks one for each mapping with file, the name of a symlink is > >> : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink > >> : results in a file that point exactly to the same inode as them vma's one. > >> : > >> : For example the ls -l of some arbitrary /proc/<pid>/map_files/ > >> : > >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so > >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 > >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 > >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so > >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so > >> > >> afacit this info is also available in /proc/pid/maps, so things > >> shouldn't get worse if the /proc/pid/map_files permissions are at least > >> as restrictive as the /proc/pid/maps permissions. Is that the case? > >> (Please add to changelog). > > > > Yes, the only difference is that you can follow the link as per above. > > I'll resend with a new message explaining that and the deletion thing. > > > >> There's one other problem here: we're assuming that the map_files > >> implementation doesn't have bugs. If it does have bugs then relaxing > >> permissions like this will create new vulnerabilities. And the > >> map_files implementation is surprisingly complex. Is it bug-free? > > > > While I was messing with it I used it a good bit and didn't see any > > issues, although I didn't actively try to fuzz it or anything. I'd be > > happy to write something to test hammering it in weird ways if you like. > > I'm also happy to write testcases for namespaces. > > > > So far as security issues, as others have pointed out you can't follow > > the links unless you can ptrace the process in question, which seems > > like a pretty solid guarantee. As Cyrill pointed out in the discussion > > about the documentation, that's the same protection as /proc/N/fd/*, and > > those links function in the same way. > > My concern here is that fd/* are connected as streams, and while that > has a certain level of badness as an external-to-the-process attacker, > PTRACE_MODE_READ is much weaker than PTRACE_MODE_ATTACH (which is > required for access to /proc/N/mem). Since these fds are the things > mapped into memory on a process, writing to them is a subset of access > to /proc/N/mem, and I don't feel that PTRACE_MODE_READ is sufficient. If you haven't done close() on a mmapped file, doesn't fd/* allow the same access to the corresponding regions of memory? Or am I missing something? But that said, I can't think of any reason making it MODE_ATTACH would be a problem. Would you rather that be enforced on follow_link() like the original patch did, or enforce it for the whole directory? Thanks, Calvin > -Kees > > -- > Kees Cook > Chrome OS Security ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable 2015-01-31 1:58 ` Calvin Owens @ 2015-02-02 14:01 ` Austin S Hemmelgarn [not found] ` <54CF832A.7010707-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-02-02 20:16 ` Andy Lutomirski 1 sibling, 1 reply; 19+ messages in thread From: Austin S Hemmelgarn @ 2015-02-02 14:01 UTC (permalink / raw) To: Calvin Owens, Kees Cook Cc: Andrew Morton, Cyrill Gorcunov, Kirill A. Shutemov, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, LKML, kernel-team, Pavel Emelyanov, Linux API [-- Attachment #1: Type: text/plain, Size: 6117 bytes --] On 2015-01-30 20:58, Calvin Owens wrote: > On Thursday 01/29 at 17:30 -0800, Kees Cook wrote: >> On Tue, Jan 27, 2015 at 8:38 PM, Calvin Owens <calvinowens@fb.com> wrote: >>> On Monday 01/26 at 15:43 -0800, Andrew Morton wrote: >>>> On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <gorcunov@gmail.com> wrote: >>>> >>>>> On Mon, Jan 26, 2015 at 02:47:31PM +0200, Kirill A. Shutemov wrote: >>>>>> On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: >>>>>>> Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and >>>>>>> is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface >>>>>>> is very useful for enumerating the files mapped into a process when >>>>>>> the more verbose information in /proc/<pid>/maps is not needed. >>>> >>>> This is the main (actually only) justification for the patch, and it it >>>> far too thin. What does "not needed" mean. Why can't people just use >>>> /proc/pid/maps? >>> >>> The biggest difference is that if you do something like this: >>> >>> fd = open("/stuff", O_BLAH); >>> map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0); >>> close(fd); >>> unlink("/stuff"); >>> >>> ...then map_files/ gives you a way to get a file descriptor for >>> "/stuff", which you couldn't do with /proc/pid/maps. >>> >>> It's also something of a win if you just want to see what is mapped at a >>> specific address, since you can just readlink() the symlink for the >>> address range you care about and it will go grab the appropriate VMA and >>> give you the answer. /proc/pid/maps requires walking the VMA tree, which >>> is quite expensive for processes with many thousands of threads, even >>> without the O(N^2) issue. >>> >>> (You have to know what address range you want though, since readdir() on >>> map_files/ obviously has to walk the VMA tree just like /proc/N/maps.) >>> >>>>>>> This patch moves the folder out from behind CHECKPOINT_RESTORE, and >>>>>>> removes the CAP_SYS_ADMIN restrictions. Following the links requires >>>>>>> the ability to ptrace the process in question, so this doesn't allow >>>>>>> an attacker to do anything they couldn't already do before. >>>>>>> >>>>>>> Signed-off-by: Calvin Owens <calvinowens@fb.com> >>>>>> >>>>>> Cc +linux-api@ >>>>> >>>>> Looks good to me, thanks! Though I would really appreciate if someone >>>>> from security camp take a look as well. >>>> >>>> hm, who's that. Kees comes to mind. >>>> >>>> And reviewers' task would be a heck of a lot easier if they knew what >>>> /proc/pid/map_files actually does. This: >>>> >>>> akpm3:/usr/src/25> grep -r map_files Documentation >>>> akpm3:/usr/src/25> >>>> >>>> does not help. >>>> >>>> The 640708a2cff7f81 changelog says: >>>> >>>> : This one behaves similarly to the /proc/<pid>/fd/ one - it contains >>>> : symlinks one for each mapping with file, the name of a symlink is >>>> : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink >>>> : results in a file that point exactly to the same inode as them vma's one. >>>> : >>>> : For example the ls -l of some arbitrary /proc/<pid>/map_files/ >>>> : >>>> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so >>>> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 >>>> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 >>>> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so >>>> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so >>>> >>>> afacit this info is also available in /proc/pid/maps, so things >>>> shouldn't get worse if the /proc/pid/map_files permissions are at least >>>> as restrictive as the /proc/pid/maps permissions. Is that the case? >>>> (Please add to changelog). >>> >>> Yes, the only difference is that you can follow the link as per above. >>> I'll resend with a new message explaining that and the deletion thing. >>> >>>> There's one other problem here: we're assuming that the map_files >>>> implementation doesn't have bugs. If it does have bugs then relaxing >>>> permissions like this will create new vulnerabilities. And the >>>> map_files implementation is surprisingly complex. Is it bug-free? >>> >>> While I was messing with it I used it a good bit and didn't see any >>> issues, although I didn't actively try to fuzz it or anything. I'd be >>> happy to write something to test hammering it in weird ways if you like. >>> I'm also happy to write testcases for namespaces. >>> >>> So far as security issues, as others have pointed out you can't follow >>> the links unless you can ptrace the process in question, which seems >>> like a pretty solid guarantee. As Cyrill pointed out in the discussion >>> about the documentation, that's the same protection as /proc/N/fd/*, and >>> those links function in the same way. >> >> My concern here is that fd/* are connected as streams, and while that >> has a certain level of badness as an external-to-the-process attacker, >> PTRACE_MODE_READ is much weaker than PTRACE_MODE_ATTACH (which is >> required for access to /proc/N/mem). Since these fds are the things >> mapped into memory on a process, writing to them is a subset of access >> to /proc/N/mem, and I don't feel that PTRACE_MODE_READ is sufficient. > > If you haven't done close() on a mmapped file, doesn't fd/* allow the > same access to the corresponding regions of memory? Or am I missing > something? > > But that said, I can't think of any reason making it MODE_ATTACH would > be a problem. Would you rather that be enforced on follow_link() like > the original patch did, or enforce it for the whole directory? > Whole directory would probably be better, as even just the mapped ranges could be considered sensitive information. Ideally, the check should be done on both follow_link(), and the directory itself. [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 2455 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <54CF832A.7010707-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <54CF832A.7010707-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2015-02-04 3:53 ` Calvin Owens 0 siblings, 0 replies; 19+ messages in thread From: Calvin Owens @ 2015-02-04 3:53 UTC (permalink / raw) To: Austin S Hemmelgarn Cc: Kees Cook, Andrew Morton, Cyrill Gorcunov, Kirill A. Shutemov, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, LKML, kernel-team-b10kYP2dOMg, Pavel Emelyanov, Linux API On Monday 02/02 at 09:01 -0500, Austin S Hemmelgarn wrote: > On 2015-01-30 20:58, Calvin Owens wrote: > >On Thursday 01/29 at 17:30 -0800, Kees Cook wrote: > >>On Tue, Jan 27, 2015 at 8:38 PM, Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> wrote: > >>>On Monday 01/26 at 15:43 -0800, Andrew Morton wrote: > >>>>On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <gorcunov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > >>>> > >>>>>On Mon, Jan 26, 2015 at 02:47:31PM +0200, Kirill A. Shutemov wrote: > >>>>>>On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: > >>>>>>>Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and > >>>>>>>is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface > >>>>>>>is very useful for enumerating the files mapped into a process when > >>>>>>>the more verbose information in /proc/<pid>/maps is not needed. > >>>> > >>>>This is the main (actually only) justification for the patch, and it it > >>>>far too thin. What does "not needed" mean. Why can't people just use > >>>>/proc/pid/maps? > >>> > >>>The biggest difference is that if you do something like this: > >>> > >>> fd = open("/stuff", O_BLAH); > >>> map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0); > >>> close(fd); > >>> unlink("/stuff"); > >>> > >>>...then map_files/ gives you a way to get a file descriptor for > >>>"/stuff", which you couldn't do with /proc/pid/maps. > >>> > >>>It's also something of a win if you just want to see what is mapped at a > >>>specific address, since you can just readlink() the symlink for the > >>>address range you care about and it will go grab the appropriate VMA and > >>>give you the answer. /proc/pid/maps requires walking the VMA tree, which > >>>is quite expensive for processes with many thousands of threads, even > >>>without the O(N^2) issue. > >>> > >>>(You have to know what address range you want though, since readdir() on > >>>map_files/ obviously has to walk the VMA tree just like /proc/N/maps.) > >>> > >>>>>>>This patch moves the folder out from behind CHECKPOINT_RESTORE, and > >>>>>>>removes the CAP_SYS_ADMIN restrictions. Following the links requires > >>>>>>>the ability to ptrace the process in question, so this doesn't allow > >>>>>>>an attacker to do anything they couldn't already do before. > >>>>>>> > >>>>>>>Signed-off-by: Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> > >>>>>> > >>>>>>Cc +linux-api@ > >>>>> > >>>>>Looks good to me, thanks! Though I would really appreciate if someone > >>>>>from security camp take a look as well. > >>>> > >>>>hm, who's that. Kees comes to mind. > >>>> > >>>>And reviewers' task would be a heck of a lot easier if they knew what > >>>>/proc/pid/map_files actually does. This: > >>>> > >>>>akpm3:/usr/src/25> grep -r map_files Documentation > >>>>akpm3:/usr/src/25> > >>>> > >>>>does not help. > >>>> > >>>>The 640708a2cff7f81 changelog says: > >>>> > >>>>: This one behaves similarly to the /proc/<pid>/fd/ one - it contains > >>>>: symlinks one for each mapping with file, the name of a symlink is > >>>>: "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink > >>>>: results in a file that point exactly to the same inode as them vma's one. > >>>>: > >>>>: For example the ls -l of some arbitrary /proc/<pid>/map_files/ > >>>>: > >>>>: | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so > >>>>: | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 > >>>>: | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 > >>>>: | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so > >>>>: | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so > >>>> > >>>>afacit this info is also available in /proc/pid/maps, so things > >>>>shouldn't get worse if the /proc/pid/map_files permissions are at least > >>>>as restrictive as the /proc/pid/maps permissions. Is that the case? > >>>>(Please add to changelog). > >>> > >>>Yes, the only difference is that you can follow the link as per above. > >>>I'll resend with a new message explaining that and the deletion thing. > >>> > >>>>There's one other problem here: we're assuming that the map_files > >>>>implementation doesn't have bugs. If it does have bugs then relaxing > >>>>permissions like this will create new vulnerabilities. And the > >>>>map_files implementation is surprisingly complex. Is it bug-free? > >>> > >>>While I was messing with it I used it a good bit and didn't see any > >>>issues, although I didn't actively try to fuzz it or anything. I'd be > >>>happy to write something to test hammering it in weird ways if you like. > >>>I'm also happy to write testcases for namespaces. > >>> > >>>So far as security issues, as others have pointed out you can't follow > >>>the links unless you can ptrace the process in question, which seems > >>>like a pretty solid guarantee. As Cyrill pointed out in the discussion > >>>about the documentation, that's the same protection as /proc/N/fd/*, and > >>>those links function in the same way. > >> > >>My concern here is that fd/* are connected as streams, and while that > >>has a certain level of badness as an external-to-the-process attacker, > >>PTRACE_MODE_READ is much weaker than PTRACE_MODE_ATTACH (which is > >>required for access to /proc/N/mem). Since these fds are the things > >>mapped into memory on a process, writing to them is a subset of access > >>to /proc/N/mem, and I don't feel that PTRACE_MODE_READ is sufficient. > > > >If you haven't done close() on a mmapped file, doesn't fd/* allow the > >same access to the corresponding regions of memory? Or am I missing > >something? > > > >But that said, I can't think of any reason making it MODE_ATTACH would > >be a problem. Would you rather that be enforced on follow_link() like > >the original patch did, or enforce it for the whole directory? > > > > > Whole directory would probably be better, as even just the mapped > ranges could be considered sensitive information. You can already get the ranges that are mapped from /proc/N/maps with PTRACE_MODE_READ, so that part isn't new information. > Ideally, the check should be done on both follow_link(), and the > directory itself. Oh, I didn't mean restricting readdir(), I meant restricting any access through the directory similar to how the original CAP_SYS_ADMIN check was done. Thanks, Calvin ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable 2015-01-31 1:58 ` Calvin Owens 2015-02-02 14:01 ` Austin S Hemmelgarn @ 2015-02-02 20:16 ` Andy Lutomirski [not found] ` <CALCETrUufe3USocUDpkBdwx6SyGKVgVUTh4rg2H9Xn91u+6iHg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 1 reply; 19+ messages in thread From: Andy Lutomirski @ 2015-02-02 20:16 UTC (permalink / raw) To: Calvin Owens Cc: Kees Cook, Andrew Morton, Cyrill Gorcunov, Kirill A. Shutemov, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, LKML, kernel-team, Pavel Emelyanov, Linux API On Fri, Jan 30, 2015 at 5:58 PM, Calvin Owens <calvinowens@fb.com> wrote: > On Thursday 01/29 at 17:30 -0800, Kees Cook wrote: >> On Tue, Jan 27, 2015 at 8:38 PM, Calvin Owens <calvinowens@fb.com> wrote: >> > On Monday 01/26 at 15:43 -0800, Andrew Morton wrote: >> >> On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <gorcunov@gmail.com> wrote: >> >> >> >> > On Mon, Jan 26, 2015 at 02:47:31PM +0200, Kirill A. Shutemov wrote: >> >> > > On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: >> >> > > > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and >> >> > > > is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface >> >> > > > is very useful for enumerating the files mapped into a process when >> >> > > > the more verbose information in /proc/<pid>/maps is not needed. >> >> >> >> This is the main (actually only) justification for the patch, and it it >> >> far too thin. What does "not needed" mean. Why can't people just use >> >> /proc/pid/maps? >> > >> > The biggest difference is that if you do something like this: >> > >> > fd = open("/stuff", O_BLAH); >> > map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0); >> > close(fd); >> > unlink("/stuff"); >> > >> > ...then map_files/ gives you a way to get a file descriptor for >> > "/stuff", which you couldn't do with /proc/pid/maps. >> > >> > It's also something of a win if you just want to see what is mapped at a >> > specific address, since you can just readlink() the symlink for the >> > address range you care about and it will go grab the appropriate VMA and >> > give you the answer. /proc/pid/maps requires walking the VMA tree, which >> > is quite expensive for processes with many thousands of threads, even >> > without the O(N^2) issue. >> > >> > (You have to know what address range you want though, since readdir() on >> > map_files/ obviously has to walk the VMA tree just like /proc/N/maps.) >> > >> >> > > > This patch moves the folder out from behind CHECKPOINT_RESTORE, and >> >> > > > removes the CAP_SYS_ADMIN restrictions. Following the links requires >> >> > > > the ability to ptrace the process in question, so this doesn't allow >> >> > > > an attacker to do anything they couldn't already do before. >> >> > > > >> >> > > > Signed-off-by: Calvin Owens <calvinowens@fb.com> >> >> > > >> >> > > Cc +linux-api@ >> >> > >> >> > Looks good to me, thanks! Though I would really appreciate if someone >> >> > from security camp take a look as well. >> >> >> >> hm, who's that. Kees comes to mind. >> >> >> >> And reviewers' task would be a heck of a lot easier if they knew what >> >> /proc/pid/map_files actually does. This: >> >> >> >> akpm3:/usr/src/25> grep -r map_files Documentation >> >> akpm3:/usr/src/25> >> >> >> >> does not help. >> >> >> >> The 640708a2cff7f81 changelog says: >> >> >> >> : This one behaves similarly to the /proc/<pid>/fd/ one - it contains >> >> : symlinks one for each mapping with file, the name of a symlink is >> >> : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink >> >> : results in a file that point exactly to the same inode as them vma's one. >> >> : >> >> : For example the ls -l of some arbitrary /proc/<pid>/map_files/ >> >> : >> >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so >> >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 >> >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 >> >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so >> >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so >> >> >> >> afacit this info is also available in /proc/pid/maps, so things >> >> shouldn't get worse if the /proc/pid/map_files permissions are at least >> >> as restrictive as the /proc/pid/maps permissions. Is that the case? >> >> (Please add to changelog). >> > >> > Yes, the only difference is that you can follow the link as per above. >> > I'll resend with a new message explaining that and the deletion thing. >> > >> >> There's one other problem here: we're assuming that the map_files >> >> implementation doesn't have bugs. If it does have bugs then relaxing >> >> permissions like this will create new vulnerabilities. And the >> >> map_files implementation is surprisingly complex. Is it bug-free? >> > >> > While I was messing with it I used it a good bit and didn't see any >> > issues, although I didn't actively try to fuzz it or anything. I'd be >> > happy to write something to test hammering it in weird ways if you like. >> > I'm also happy to write testcases for namespaces. >> > >> > So far as security issues, as others have pointed out you can't follow >> > the links unless you can ptrace the process in question, which seems >> > like a pretty solid guarantee. As Cyrill pointed out in the discussion >> > about the documentation, that's the same protection as /proc/N/fd/*, and >> > those links function in the same way. >> >> My concern here is that fd/* are connected as streams, and while that >> has a certain level of badness as an external-to-the-process attacker, >> PTRACE_MODE_READ is much weaker than PTRACE_MODE_ATTACH (which is >> required for access to /proc/N/mem). Since these fds are the things >> mapped into memory on a process, writing to them is a subset of access >> to /proc/N/mem, and I don't feel that PTRACE_MODE_READ is sufficient. > > If you haven't done close() on a mmapped file, doesn't fd/* allow the > same access to the corresponding regions of memory? Or am I missing > something? > But if you have called close(), then you can't currently do things like ftruncate or ioctl on the mapped file. These things don't persist across execve(), but the do persist across calls to setresuid, etc that drop privileges. The latter part makes me a tiny bit nervous. It also might be worth checking for drivers or arch code that creates vmas that are backed by a different struct file than the struct file that was mmapped in the first place. --Andy ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CALCETrUufe3USocUDpkBdwx6SyGKVgVUTh4rg2H9Xn91u+6iHg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable [not found] ` <CALCETrUufe3USocUDpkBdwx6SyGKVgVUTh4rg2H9Xn91u+6iHg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2015-02-04 3:28 ` Calvin Owens 0 siblings, 0 replies; 19+ messages in thread From: Calvin Owens @ 2015-02-04 3:28 UTC (permalink / raw) To: Andy Lutomirski Cc: Kees Cook, Andrew Morton, Cyrill Gorcunov, Kirill A. Shutemov, Alexey Dobriyan, Oleg Nesterov, Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner, Grant Likely, Siddhesh Poyarekar, LKML, kernel-team-b10kYP2dOMg, Pavel Emelyanov, Linux API On Monday 02/02 at 12:16 -0800, Andy Lutomirski wrote: > On Fri, Jan 30, 2015 at 5:58 PM, Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> wrote: > > On Thursday 01/29 at 17:30 -0800, Kees Cook wrote: > >> On Tue, Jan 27, 2015 at 8:38 PM, Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> wrote: > >> > On Monday 01/26 at 15:43 -0800, Andrew Morton wrote: > >> >> On Tue, 27 Jan 2015 00:00:54 +0300 Cyrill Gorcunov <gorcunov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > >> >> > >> >> > On Mon, Jan 26, 2015 at 02:47:31PM +0200, Kirill A. Shutemov wrote: > >> >> > > On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote: > >> >> > > > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and > >> >> > > > is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface > >> >> > > > is very useful for enumerating the files mapped into a process when > >> >> > > > the more verbose information in /proc/<pid>/maps is not needed. > >> >> > >> >> This is the main (actually only) justification for the patch, and it it > >> >> far too thin. What does "not needed" mean. Why can't people just use > >> >> /proc/pid/maps? > >> > > >> > The biggest difference is that if you do something like this: > >> > > >> > fd = open("/stuff", O_BLAH); > >> > map = mmap(NULL, 4096, PROT_BLAH, MAP_SHARED, fd, 0); > >> > close(fd); > >> > unlink("/stuff"); > >> > > >> > ...then map_files/ gives you a way to get a file descriptor for > >> > "/stuff", which you couldn't do with /proc/pid/maps. > >> > > >> > It's also something of a win if you just want to see what is mapped at a > >> > specific address, since you can just readlink() the symlink for the > >> > address range you care about and it will go grab the appropriate VMA and > >> > give you the answer. /proc/pid/maps requires walking the VMA tree, which > >> > is quite expensive for processes with many thousands of threads, even > >> > without the O(N^2) issue. > >> > > >> > (You have to know what address range you want though, since readdir() on > >> > map_files/ obviously has to walk the VMA tree just like /proc/N/maps.) > >> > > >> >> > > > This patch moves the folder out from behind CHECKPOINT_RESTORE, and > >> >> > > > removes the CAP_SYS_ADMIN restrictions. Following the links requires > >> >> > > > the ability to ptrace the process in question, so this doesn't allow > >> >> > > > an attacker to do anything they couldn't already do before. > >> >> > > > > >> >> > > > Signed-off-by: Calvin Owens <calvinowens-b10kYP2dOMg@public.gmane.org> > >> >> > > > >> >> > > Cc +linux-api@ > >> >> > > >> >> > Looks good to me, thanks! Though I would really appreciate if someone > >> >> > from security camp take a look as well. > >> >> > >> >> hm, who's that. Kees comes to mind. > >> >> > >> >> And reviewers' task would be a heck of a lot easier if they knew what > >> >> /proc/pid/map_files actually does. This: > >> >> > >> >> akpm3:/usr/src/25> grep -r map_files Documentation > >> >> akpm3:/usr/src/25> > >> >> > >> >> does not help. > >> >> > >> >> The 640708a2cff7f81 changelog says: > >> >> > >> >> : This one behaves similarly to the /proc/<pid>/fd/ one - it contains > >> >> : symlinks one for each mapping with file, the name of a symlink is > >> >> : "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink > >> >> : results in a file that point exactly to the same inode as them vma's one. > >> >> : > >> >> : For example the ls -l of some arbitrary /proc/<pid>/map_files/ > >> >> : > >> >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so > >> >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 > >> >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 > >> >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so > >> >> : | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so > >> >> > >> >> afacit this info is also available in /proc/pid/maps, so things > >> >> shouldn't get worse if the /proc/pid/map_files permissions are at least > >> >> as restrictive as the /proc/pid/maps permissions. Is that the case? > >> >> (Please add to changelog). > >> > > >> > Yes, the only difference is that you can follow the link as per above. > >> > I'll resend with a new message explaining that and the deletion thing. > >> > > >> >> There's one other problem here: we're assuming that the map_files > >> >> implementation doesn't have bugs. If it does have bugs then relaxing > >> >> permissions like this will create new vulnerabilities. And the > >> >> map_files implementation is surprisingly complex. Is it bug-free? > >> > > >> > While I was messing with it I used it a good bit and didn't see any > >> > issues, although I didn't actively try to fuzz it or anything. I'd be > >> > happy to write something to test hammering it in weird ways if you like. > >> > I'm also happy to write testcases for namespaces. > >> > > >> > So far as security issues, as others have pointed out you can't follow > >> > the links unless you can ptrace the process in question, which seems > >> > like a pretty solid guarantee. As Cyrill pointed out in the discussion > >> > about the documentation, that's the same protection as /proc/N/fd/*, and > >> > those links function in the same way. > >> > >> My concern here is that fd/* are connected as streams, and while that > >> has a certain level of badness as an external-to-the-process attacker, > >> PTRACE_MODE_READ is much weaker than PTRACE_MODE_ATTACH (which is > >> required for access to /proc/N/mem). Since these fds are the things > >> mapped into memory on a process, writing to them is a subset of access > >> to /proc/N/mem, and I don't feel that PTRACE_MODE_READ is sufficient. > > > > If you haven't done close() on a mmapped file, doesn't fd/* allow the > > same access to the corresponding regions of memory? Or am I missing > > something? > > > > But if you have called close(), then you can't currently do things > like ftruncate or ioctl on the mapped file. These things don't > persist across execve(), but the do persist across calls to setresuid, > etc that drop privileges. The latter part makes me a tiny bit > nervous. Hmm, in that scenario you would have to open() the map_files symlink, and since you've dropped privileges that would only succeed if the user you dropped to has permission to access that file anyway, right? In the deleted file case it does actually allow something that used to be impossible, but relying on open/map/close/unlink to prevent a user from opening a file they have permission to open is just buggy in general. But, O_TMPFILE lets you end up in that position without the race. The manpage says that O_TMPFILE files "can never be reached via any pathname", which isn't strictly true since you can get them from fd/* in proc. But if you close() after mapping it they are currently truly inaccessible via any path, and given the language in the manpage it seems reasonable that somebody might rely on that and be lazy with the permissions. I hadn't thought about O_TMPFILE thing: I'm definitely convinced now that PTRACE_MODE_ATTACH is the right thing here. But I think having to reopen the file saves you even if you "leak" maps of files across a call to setresuid/etc. > It also might be worth checking for drivers or arch code that creates > vmas that are backed by a different struct file than the struct file > that was mmapped in the first place. Interesting, I'll look into this before I resend. Thanks, Calvin > --Andy ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-02-04 3:53 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <1421194829-28696-1-git-send-email-calvinowens@fb.com> [not found] ` <20150114152501.GB9820@node.dhcp.inet.fi> [not found] ` <20150114153323.GF2253@moon> [not found] ` <20150114204653.GA26698@mail.thefacebook.com> [not found] ` <20150114211613.GH2253@moon> [not found] ` <20150122024554.GB23762@mail.thefacebook.com> [not found] ` <20150124031544.GA1992748@mail.thefacebook.com> 2015-01-26 12:47 ` [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable Kirill A. Shutemov [not found] ` <20150126124731.GA26916-nhfs4B5ZimeFUdmeq17FyvUpdFzICT1y@public.gmane.org> 2015-01-26 21:00 ` Cyrill Gorcunov 2015-01-26 23:43 ` Andrew Morton [not found] ` <20150126154346.c63c512e5821e9e0ea31f759-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> 2015-01-27 0:15 ` Kees Cook [not found] ` <CAGXu5jLDkg0hJSMm3CdoO-77yiK5GQWHSe3+1h7mq76LERpNBA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-01-27 7:37 ` Cyrill Gorcunov 2015-01-27 19:53 ` Kees Cook [not found] ` <CAGXu5jJFFib7F7uKYgvX4ecyMnbincd22FaO_bFy=VRVKdFbvA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-01-27 21:35 ` Cyrill Gorcunov 2015-01-27 21:46 ` Pavel Emelyanov 2015-01-27 0:19 ` Kirill A. Shutemov 2015-01-27 6:46 ` Cyrill Gorcunov 2015-01-27 6:50 ` Andrew Morton [not found] ` <20150126225023.df63f6ca.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> 2015-01-27 7:23 ` Cyrill Gorcunov 2015-01-28 4:38 ` Calvin Owens [not found] ` <20150128043832.GA2266262-ZEWhMxyTXSP95iwofa7G/laTQe2KTcn/@public.gmane.org> 2015-01-30 1:30 ` Kees Cook [not found] ` <CAGXu5j+wa2-CCGaktPzDec=HF0CizP__HVVjZKcjGuJJOvLFyg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-01-31 1:58 ` Calvin Owens 2015-02-02 14:01 ` Austin S Hemmelgarn [not found] ` <54CF832A.7010707-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-02-04 3:53 ` Calvin Owens 2015-02-02 20:16 ` Andy Lutomirski [not found] ` <CALCETrUufe3USocUDpkBdwx6SyGKVgVUTh4rg2H9Xn91u+6iHg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-02-04 3:28 ` Calvin Owens
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).