* [PATCH v2] fs: clear file privilege bits when mmap writing @ 2015-12-03 0:03 Kees Cook 2015-12-03 0:18 ` Andrew Morton 2015-12-04 1:45 ` yalin wang 0 siblings, 2 replies; 17+ messages in thread From: Kees Cook @ 2015-12-03 0:03 UTC (permalink / raw) To: Andrew Morton Cc: Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, linux-mm, linux-kernel Normally, when a user can modify a file that has setuid or setgid bits, those bits are cleared when they are not the file owner or a member of the group. This is enforced when using write and truncate but not when writing to a shared mmap on the file. This could allow the file writer to gain privileges by changing a binary without losing the setuid/setgid/caps bits. Changing the bits requires holding inode->i_mutex, so it cannot be done during the page fault (due to mmap_sem being held during the fault). Instead, clear the bits if PROT_WRITE is being used at mmap time. Signed-off-by: Kees Cook <keescook@chromium.org> Cc: stable@vger.kernel.org --- v2: - move check from page fault to mmap open --- mm/mmap.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/mm/mmap.c b/mm/mmap.c index 2ce04a649f6b..a27735aabc73 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1340,6 +1340,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr, if (locks_verify_locked(file)) return -EAGAIN; + /* + * If we must remove privs, we do it here since + * doing it during page COW is expensive and + * cannot hold inode->i_mutex. + */ + if (prot & PROT_WRITE && !IS_NOSEC(inode)) { + mutex_lock(&inode->i_mutex); + file_remove_privs(file); + mutex_unlock(&inode->i_mutex); + } + vm_flags |= VM_SHARED | VM_MAYSHARE; if (!(file->f_mode & FMODE_WRITE)) vm_flags &= ~(VM_MAYWRITE | VM_SHARED); -- 1.9.1 -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] fs: clear file privilege bits when mmap writing 2015-12-03 0:03 [PATCH v2] fs: clear file privilege bits when mmap writing Kees Cook @ 2015-12-03 0:18 ` Andrew Morton 2015-12-04 1:45 ` yalin wang 1 sibling, 0 replies; 17+ messages in thread From: Andrew Morton @ 2015-12-03 0:18 UTC (permalink / raw) To: Kees Cook Cc: Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, linux-mm, linux-kernel On Wed, 2 Dec 2015 16:03:42 -0800 Kees Cook <keescook@chromium.org> wrote: > Normally, when a user can modify a file that has setuid or setgid bits, > those bits are cleared when they are not the file owner or a member > of the group. This is enforced when using write and truncate but not > when writing to a shared mmap on the file. This could allow the file > writer to gain privileges by changing a binary without losing the > setuid/setgid/caps bits. > > Changing the bits requires holding inode->i_mutex, so it cannot be done > during the page fault (due to mmap_sem being held during the fault). > Instead, clear the bits if PROT_WRITE is being used at mmap time. > > ... > > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1340,6 +1340,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr, > if (locks_verify_locked(file)) > return -EAGAIN; > > + /* > + * If we must remove privs, we do it here since > + * doing it during page COW is expensive and > + * cannot hold inode->i_mutex. > + */ > + if (prot & PROT_WRITE && !IS_NOSEC(inode)) { > + mutex_lock(&inode->i_mutex); > + file_remove_privs(file); > + mutex_unlock(&inode->i_mutex); > + } > + Still ignoring the file_remove_privs() return value. If this is deliberate then a description of the reasons should be included? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] fs: clear file privilege bits when mmap writing @ 2015-12-03 0:18 ` Andrew Morton 0 siblings, 0 replies; 17+ messages in thread From: Andrew Morton @ 2015-12-03 0:18 UTC (permalink / raw) To: Kees Cook Cc: Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, linux-mm, linux-kernel On Wed, 2 Dec 2015 16:03:42 -0800 Kees Cook <keescook@chromium.org> wrote: > Normally, when a user can modify a file that has setuid or setgid bits, > those bits are cleared when they are not the file owner or a member > of the group. This is enforced when using write and truncate but not > when writing to a shared mmap on the file. This could allow the file > writer to gain privileges by changing a binary without losing the > setuid/setgid/caps bits. > > Changing the bits requires holding inode->i_mutex, so it cannot be done > during the page fault (due to mmap_sem being held during the fault). > Instead, clear the bits if PROT_WRITE is being used at mmap time. > > ... > > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1340,6 +1340,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr, > if (locks_verify_locked(file)) > return -EAGAIN; > > + /* > + * If we must remove privs, we do it here since > + * doing it during page COW is expensive and > + * cannot hold inode->i_mutex. > + */ > + if (prot & PROT_WRITE && !IS_NOSEC(inode)) { > + mutex_lock(&inode->i_mutex); > + file_remove_privs(file); > + mutex_unlock(&inode->i_mutex); > + } > + Still ignoring the file_remove_privs() return value. If this is deliberate then a description of the reasons should be included? -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] fs: clear file privilege bits when mmap writing 2015-12-03 0:18 ` Andrew Morton @ 2015-12-03 16:07 ` Kees Cook -1 siblings, 0 replies; 17+ messages in thread From: Kees Cook @ 2015-12-03 16:07 UTC (permalink / raw) To: Andrew Morton Cc: Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML On Wed, Dec 2, 2015 at 4:18 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 2 Dec 2015 16:03:42 -0800 Kees Cook <keescook@chromium.org> wrote: > >> Normally, when a user can modify a file that has setuid or setgid bits, >> those bits are cleared when they are not the file owner or a member >> of the group. This is enforced when using write and truncate but not >> when writing to a shared mmap on the file. This could allow the file >> writer to gain privileges by changing a binary without losing the >> setuid/setgid/caps bits. >> >> Changing the bits requires holding inode->i_mutex, so it cannot be done >> during the page fault (due to mmap_sem being held during the fault). >> Instead, clear the bits if PROT_WRITE is being used at mmap time. >> >> ... >> >> --- a/mm/mmap.c >> +++ b/mm/mmap.c >> @@ -1340,6 +1340,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr, >> if (locks_verify_locked(file)) >> return -EAGAIN; >> >> + /* >> + * If we must remove privs, we do it here since >> + * doing it during page COW is expensive and >> + * cannot hold inode->i_mutex. >> + */ >> + if (prot & PROT_WRITE && !IS_NOSEC(inode)) { >> + mutex_lock(&inode->i_mutex); >> + file_remove_privs(file); >> + mutex_unlock(&inode->i_mutex); >> + } >> + > > Still ignoring the file_remove_privs() return value. If this is > deliberate then a description of the reasons should be included? Argh, yes, sorry. I will send a v3. -Kees -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] fs: clear file privilege bits when mmap writing @ 2015-12-03 16:07 ` Kees Cook 0 siblings, 0 replies; 17+ messages in thread From: Kees Cook @ 2015-12-03 16:07 UTC (permalink / raw) To: Andrew Morton Cc: Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML On Wed, Dec 2, 2015 at 4:18 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 2 Dec 2015 16:03:42 -0800 Kees Cook <keescook@chromium.org> wrote: > >> Normally, when a user can modify a file that has setuid or setgid bits, >> those bits are cleared when they are not the file owner or a member >> of the group. This is enforced when using write and truncate but not >> when writing to a shared mmap on the file. This could allow the file >> writer to gain privileges by changing a binary without losing the >> setuid/setgid/caps bits. >> >> Changing the bits requires holding inode->i_mutex, so it cannot be done >> during the page fault (due to mmap_sem being held during the fault). >> Instead, clear the bits if PROT_WRITE is being used at mmap time. >> >> ... >> >> --- a/mm/mmap.c >> +++ b/mm/mmap.c >> @@ -1340,6 +1340,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr, >> if (locks_verify_locked(file)) >> return -EAGAIN; >> >> + /* >> + * If we must remove privs, we do it here since >> + * doing it during page COW is expensive and >> + * cannot hold inode->i_mutex. >> + */ >> + if (prot & PROT_WRITE && !IS_NOSEC(inode)) { >> + mutex_lock(&inode->i_mutex); >> + file_remove_privs(file); >> + mutex_unlock(&inode->i_mutex); >> + } >> + > > Still ignoring the file_remove_privs() return value. If this is > deliberate then a description of the reasons should be included? Argh, yes, sorry. I will send a v3. -Kees -- Kees Cook Chrome OS & Brillo Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] fs: clear file privilege bits when mmap writing 2015-12-03 0:18 ` Andrew Morton @ 2015-12-03 18:19 ` Kees Cook -1 siblings, 0 replies; 17+ messages in thread From: Kees Cook @ 2015-12-03 18:19 UTC (permalink / raw) To: Andrew Morton Cc: Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML On Wed, Dec 2, 2015 at 4:18 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 2 Dec 2015 16:03:42 -0800 Kees Cook <keescook@chromium.org> wrote: > >> Normally, when a user can modify a file that has setuid or setgid bits, >> those bits are cleared when they are not the file owner or a member >> of the group. This is enforced when using write and truncate but not >> when writing to a shared mmap on the file. This could allow the file >> writer to gain privileges by changing a binary without losing the >> setuid/setgid/caps bits. >> >> Changing the bits requires holding inode->i_mutex, so it cannot be done >> during the page fault (due to mmap_sem being held during the fault). >> Instead, clear the bits if PROT_WRITE is being used at mmap time. >> >> ... >> >> --- a/mm/mmap.c >> +++ b/mm/mmap.c >> @@ -1340,6 +1340,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr, >> if (locks_verify_locked(file)) >> return -EAGAIN; >> >> + /* >> + * If we must remove privs, we do it here since >> + * doing it during page COW is expensive and >> + * cannot hold inode->i_mutex. >> + */ >> + if (prot & PROT_WRITE && !IS_NOSEC(inode)) { >> + mutex_lock(&inode->i_mutex); >> + file_remove_privs(file); >> + mutex_unlock(&inode->i_mutex); >> + } >> + > > Still ignoring the file_remove_privs() return value. If this is > deliberate then a description of the reasons should be included? Actually, there is a bigger problem: https://lists.01.org/pipermail/lkp/2015-December/003185.html [ 37.741286] trinity-c0/742 is trying to acquire lock: [ 37.741982] (&sb->s_type->i_mutex_key#8){+.+.+.}, at: [<811c3b34>] do_mmap+0x544/0x670 [ 37.752562] [ 37.752562] but task is already holding lock: [ 37.753442] (&mm->mmap_sem){++++++}, at: [<811c3d70>] SyS_remap_file_pages+0xe0/0x350 Jan, any thoughts on avoiding this? -Kees -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] fs: clear file privilege bits when mmap writing @ 2015-12-03 18:19 ` Kees Cook 0 siblings, 0 replies; 17+ messages in thread From: Kees Cook @ 2015-12-03 18:19 UTC (permalink / raw) To: Andrew Morton Cc: Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML On Wed, Dec 2, 2015 at 4:18 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 2 Dec 2015 16:03:42 -0800 Kees Cook <keescook@chromium.org> wrote: > >> Normally, when a user can modify a file that has setuid or setgid bits, >> those bits are cleared when they are not the file owner or a member >> of the group. This is enforced when using write and truncate but not >> when writing to a shared mmap on the file. This could allow the file >> writer to gain privileges by changing a binary without losing the >> setuid/setgid/caps bits. >> >> Changing the bits requires holding inode->i_mutex, so it cannot be done >> during the page fault (due to mmap_sem being held during the fault). >> Instead, clear the bits if PROT_WRITE is being used at mmap time. >> >> ... >> >> --- a/mm/mmap.c >> +++ b/mm/mmap.c >> @@ -1340,6 +1340,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr, >> if (locks_verify_locked(file)) >> return -EAGAIN; >> >> + /* >> + * If we must remove privs, we do it here since >> + * doing it during page COW is expensive and >> + * cannot hold inode->i_mutex. >> + */ >> + if (prot & PROT_WRITE && !IS_NOSEC(inode)) { >> + mutex_lock(&inode->i_mutex); >> + file_remove_privs(file); >> + mutex_unlock(&inode->i_mutex); >> + } >> + > > Still ignoring the file_remove_privs() return value. If this is > deliberate then a description of the reasons should be included? Actually, there is a bigger problem: https://lists.01.org/pipermail/lkp/2015-December/003185.html [ 37.741286] trinity-c0/742 is trying to acquire lock: [ 37.741982] (&sb->s_type->i_mutex_key#8){+.+.+.}, at: [<811c3b34>] do_mmap+0x544/0x670 [ 37.752562] [ 37.752562] but task is already holding lock: [ 37.753442] (&mm->mmap_sem){++++++}, at: [<811c3d70>] SyS_remap_file_pages+0xe0/0x350 Jan, any thoughts on avoiding this? -Kees -- Kees Cook Chrome OS & Brillo Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] clear file privilege bits when mmap writing 2015-12-03 0:03 [PATCH v2] fs: clear file privilege bits when mmap writing Kees Cook @ 2015-12-04 1:45 ` yalin wang 2015-12-04 1:45 ` yalin wang 1 sibling, 0 replies; 17+ messages in thread From: yalin wang @ 2015-12-04 1:45 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, linux-mm, linux-kernel > On Dec 2, 2015, at 16:03, Kees Cook <keescook@chromium.org> wrote: > > Normally, when a user can modify a file that has setuid or setgid bits, > those bits are cleared when they are not the file owner or a member > of the group. This is enforced when using write and truncate but not > when writing to a shared mmap on the file. This could allow the file > writer to gain privileges by changing a binary without losing the > setuid/setgid/caps bits. > > Changing the bits requires holding inode->i_mutex, so it cannot be done > during the page fault (due to mmap_sem being held during the fault). > Instead, clear the bits if PROT_WRITE is being used at mmap time. > > Signed-off-by: Kees Cook <keescook@chromium.org> > Cc: stable@vger.kernel.org > — is this means mprotect() sys call also need add this check? mprotect() can change to PROT_WRITE, then it can write to a read only map again , also a secure hole here . Thanks ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] clear file privilege bits when mmap writing @ 2015-12-04 1:45 ` yalin wang 0 siblings, 0 replies; 17+ messages in thread From: yalin wang @ 2015-12-04 1:45 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, linux-mm, linux-kernel > On Dec 2, 2015, at 16:03, Kees Cook <keescook@chromium.org> wrote: > > Normally, when a user can modify a file that has setuid or setgid bits, > those bits are cleared when they are not the file owner or a member > of the group. This is enforced when using write and truncate but not > when writing to a shared mmap on the file. This could allow the file > writer to gain privileges by changing a binary without losing the > setuid/setgid/caps bits. > > Changing the bits requires holding inode->i_mutex, so it cannot be done > during the page fault (due to mmap_sem being held during the fault). > Instead, clear the bits if PROT_WRITE is being used at mmap time. > > Signed-off-by: Kees Cook <keescook@chromium.org> > Cc: stable@vger.kernel.org > — is this means mprotect() sys call also need add this check? mprotect() can change to PROT_WRITE, then it can write to a read only map again , also a secure hole here . Thanks -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] clear file privilege bits when mmap writing 2015-12-04 1:45 ` yalin wang @ 2015-12-07 22:42 ` Kees Cook -1 siblings, 0 replies; 17+ messages in thread From: Kees Cook @ 2015-12-07 22:42 UTC (permalink / raw) To: yalin wang Cc: Andrew Morton, Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML On Thu, Dec 3, 2015 at 5:45 PM, yalin wang <yalin.wang2010@gmail.com> wrote: > >> On Dec 2, 2015, at 16:03, Kees Cook <keescook@chromium.org> wrote: >> >> Normally, when a user can modify a file that has setuid or setgid bits, >> those bits are cleared when they are not the file owner or a member >> of the group. This is enforced when using write and truncate but not >> when writing to a shared mmap on the file. This could allow the file >> writer to gain privileges by changing a binary without losing the >> setuid/setgid/caps bits. >> >> Changing the bits requires holding inode->i_mutex, so it cannot be done >> during the page fault (due to mmap_sem being held during the fault). >> Instead, clear the bits if PROT_WRITE is being used at mmap time. >> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> Cc: stable@vger.kernel.org >> — > > is this means mprotect() sys call also need add this check? > mprotect() can change to PROT_WRITE, then it can write to a > read only map again , also a secure hole here . Yes, good point. This needs to be added. I will send a new patch. Thanks! -Kees -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] clear file privilege bits when mmap writing @ 2015-12-07 22:42 ` Kees Cook 0 siblings, 0 replies; 17+ messages in thread From: Kees Cook @ 2015-12-07 22:42 UTC (permalink / raw) To: yalin wang Cc: Andrew Morton, Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML On Thu, Dec 3, 2015 at 5:45 PM, yalin wang <yalin.wang2010@gmail.com> wrote: > >> On Dec 2, 2015, at 16:03, Kees Cook <keescook@chromium.org> wrote: >> >> Normally, when a user can modify a file that has setuid or setgid bits, >> those bits are cleared when they are not the file owner or a member >> of the group. This is enforced when using write and truncate but not >> when writing to a shared mmap on the file. This could allow the file >> writer to gain privileges by changing a binary without losing the >> setuid/setgid/caps bits. >> >> Changing the bits requires holding inode->i_mutex, so it cannot be done >> during the page fault (due to mmap_sem being held during the fault). >> Instead, clear the bits if PROT_WRITE is being used at mmap time. >> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> Cc: stable@vger.kernel.org >> — > > is this means mprotect() sys call also need add this check? > mprotect() can change to PROT_WRITE, then it can write to a > read only map again , also a secure hole here . Yes, good point. This needs to be added. I will send a new patch. Thanks! -Kees -- Kees Cook Chrome OS & Brillo Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] clear file privilege bits when mmap writing 2015-12-07 22:42 ` Kees Cook @ 2015-12-08 0:40 ` Kees Cook -1 siblings, 0 replies; 17+ messages in thread From: Kees Cook @ 2015-12-08 0:40 UTC (permalink / raw) To: yalin wang Cc: Andrew Morton, Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML On Mon, Dec 7, 2015 at 2:42 PM, Kees Cook <keescook@chromium.org> wrote: > On Thu, Dec 3, 2015 at 5:45 PM, yalin wang <yalin.wang2010@gmail.com> wrote: >> >>> On Dec 2, 2015, at 16:03, Kees Cook <keescook@chromium.org> wrote: >>> >>> Normally, when a user can modify a file that has setuid or setgid bits, >>> those bits are cleared when they are not the file owner or a member >>> of the group. This is enforced when using write and truncate but not >>> when writing to a shared mmap on the file. This could allow the file >>> writer to gain privileges by changing a binary without losing the >>> setuid/setgid/caps bits. >>> >>> Changing the bits requires holding inode->i_mutex, so it cannot be done >>> during the page fault (due to mmap_sem being held during the fault). >>> Instead, clear the bits if PROT_WRITE is being used at mmap time. >>> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> Cc: stable@vger.kernel.org >>> — >> >> is this means mprotect() sys call also need add this check? >> mprotect() can change to PROT_WRITE, then it can write to a >> read only map again , also a secure hole here . > > Yes, good point. This needs to be added. I will send a new patch. Thanks! This continues to look worse and worse. So... to check this at mprotect time, I have to know it's MAP_SHARED, but that's in the vma_flags, which I can only see after holding mmap_sem. The best I can think of now is to strip the bits at munmap time, since you can't execute an mmapped file until it closes. Jan, thoughts on this? -Kees -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] clear file privilege bits when mmap writing @ 2015-12-08 0:40 ` Kees Cook 0 siblings, 0 replies; 17+ messages in thread From: Kees Cook @ 2015-12-08 0:40 UTC (permalink / raw) To: yalin wang Cc: Andrew Morton, Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML On Mon, Dec 7, 2015 at 2:42 PM, Kees Cook <keescook@chromium.org> wrote: > On Thu, Dec 3, 2015 at 5:45 PM, yalin wang <yalin.wang2010@gmail.com> wrote: >> >>> On Dec 2, 2015, at 16:03, Kees Cook <keescook@chromium.org> wrote: >>> >>> Normally, when a user can modify a file that has setuid or setgid bits, >>> those bits are cleared when they are not the file owner or a member >>> of the group. This is enforced when using write and truncate but not >>> when writing to a shared mmap on the file. This could allow the file >>> writer to gain privileges by changing a binary without losing the >>> setuid/setgid/caps bits. >>> >>> Changing the bits requires holding inode->i_mutex, so it cannot be done >>> during the page fault (due to mmap_sem being held during the fault). >>> Instead, clear the bits if PROT_WRITE is being used at mmap time. >>> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> Cc: stable@vger.kernel.org >>> — >> >> is this means mprotect() sys call also need add this check? >> mprotect() can change to PROT_WRITE, then it can write to a >> read only map again , also a secure hole here . > > Yes, good point. This needs to be added. I will send a new patch. Thanks! This continues to look worse and worse. So... to check this at mprotect time, I have to know it's MAP_SHARED, but that's in the vma_flags, which I can only see after holding mmap_sem. The best I can think of now is to strip the bits at munmap time, since you can't execute an mmapped file until it closes. Jan, thoughts on this? -Kees -- Kees Cook Chrome OS & Brillo Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] clear file privilege bits when mmap writing 2015-12-08 0:40 ` Kees Cook @ 2015-12-09 8:26 ` Jan Kara -1 siblings, 0 replies; 17+ messages in thread From: Jan Kara @ 2015-12-09 8:26 UTC (permalink / raw) To: Kees Cook Cc: yalin wang, Andrew Morton, Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML, Al Viro On Mon 07-12-15 16:40:14, Kees Cook wrote: > On Mon, Dec 7, 2015 at 2:42 PM, Kees Cook <keescook@chromium.org> wrote: > > On Thu, Dec 3, 2015 at 5:45 PM, yalin wang <yalin.wang2010@gmail.com> wrote: > >> > >>> On Dec 2, 2015, at 16:03, Kees Cook <keescook@chromium.org> wrote: > >>> > >>> Normally, when a user can modify a file that has setuid or setgid bits, > >>> those bits are cleared when they are not the file owner or a member > >>> of the group. This is enforced when using write and truncate but not > >>> when writing to a shared mmap on the file. This could allow the file > >>> writer to gain privileges by changing a binary without losing the > >>> setuid/setgid/caps bits. > >>> > >>> Changing the bits requires holding inode->i_mutex, so it cannot be done > >>> during the page fault (due to mmap_sem being held during the fault). > >>> Instead, clear the bits if PROT_WRITE is being used at mmap time. > >>> > >>> Signed-off-by: Kees Cook <keescook@chromium.org> > >>> Cc: stable@vger.kernel.org > >>> — > >> > >> is this means mprotect() sys call also need add this check? > >> mprotect() can change to PROT_WRITE, then it can write to a > >> read only map again , also a secure hole here . > > > > Yes, good point. This needs to be added. I will send a new patch. Thanks! > > This continues to look worse and worse. > > So... to check this at mprotect time, I have to know it's MAP_SHARED, > but that's in the vma_flags, which I can only see after holding > mmap_sem. > > The best I can think of now is to strip the bits at munmap time, since > you can't execute an mmapped file until it closes. > > Jan, thoughts on this? Umm, so we actually refuse to execute a file while someone has it open for writing (deny_write_access() in do_open_execat()). So dropping the suid / sgid bits when closing file for writing could be plausible. Grabbing i_mutex from __fput() context is safe (it gets called from task_work context when returning to userspace). That way we could actually remove the checks done for each write. To avoid unexpected removal of suid/sgid bits when someone just opens & closes the file, we could mark the file as needing suid/sgid treatment by a flag in inode->i_flags when file gets written to or mmaped and then check for this in __fput(). I've added Al Viro to CC just in case he is aware of some issues with this... Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] clear file privilege bits when mmap writing @ 2015-12-09 8:26 ` Jan Kara 0 siblings, 0 replies; 17+ messages in thread From: Jan Kara @ 2015-12-09 8:26 UTC (permalink / raw) To: Kees Cook Cc: yalin wang, Andrew Morton, Jan Kara, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML, Al Viro On Mon 07-12-15 16:40:14, Kees Cook wrote: > On Mon, Dec 7, 2015 at 2:42 PM, Kees Cook <keescook@chromium.org> wrote: > > On Thu, Dec 3, 2015 at 5:45 PM, yalin wang <yalin.wang2010@gmail.com> wrote: > >> > >>> On Dec 2, 2015, at 16:03, Kees Cook <keescook@chromium.org> wrote: > >>> > >>> Normally, when a user can modify a file that has setuid or setgid bits, > >>> those bits are cleared when they are not the file owner or a member > >>> of the group. This is enforced when using write and truncate but not > >>> when writing to a shared mmap on the file. This could allow the file > >>> writer to gain privileges by changing a binary without losing the > >>> setuid/setgid/caps bits. > >>> > >>> Changing the bits requires holding inode->i_mutex, so it cannot be done > >>> during the page fault (due to mmap_sem being held during the fault). > >>> Instead, clear the bits if PROT_WRITE is being used at mmap time. > >>> > >>> Signed-off-by: Kees Cook <keescook@chromium.org> > >>> Cc: stable@vger.kernel.org > >>> a?? > >> > >> is this means mprotect() sys call also need add this check? > >> mprotect() can change to PROT_WRITE, then it can write to a > >> read only map again , also a secure hole here . > > > > Yes, good point. This needs to be added. I will send a new patch. Thanks! > > This continues to look worse and worse. > > So... to check this at mprotect time, I have to know it's MAP_SHARED, > but that's in the vma_flags, which I can only see after holding > mmap_sem. > > The best I can think of now is to strip the bits at munmap time, since > you can't execute an mmapped file until it closes. > > Jan, thoughts on this? Umm, so we actually refuse to execute a file while someone has it open for writing (deny_write_access() in do_open_execat()). So dropping the suid / sgid bits when closing file for writing could be plausible. Grabbing i_mutex from __fput() context is safe (it gets called from task_work context when returning to userspace). That way we could actually remove the checks done for each write. To avoid unexpected removal of suid/sgid bits when someone just opens & closes the file, we could mark the file as needing suid/sgid treatment by a flag in inode->i_flags when file gets written to or mmaped and then check for this in __fput(). I've added Al Viro to CC just in case he is aware of some issues with this... Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] clear file privilege bits when mmap writing 2015-12-09 8:26 ` Jan Kara @ 2015-12-09 22:52 ` Kees Cook -1 siblings, 0 replies; 17+ messages in thread From: Kees Cook @ 2015-12-09 22:52 UTC (permalink / raw) To: Jan Kara Cc: yalin wang, Andrew Morton, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML, Al Viro On Wed, Dec 9, 2015 at 12:26 AM, Jan Kara <jack@suse.cz> wrote: > On Mon 07-12-15 16:40:14, Kees Cook wrote: >> On Mon, Dec 7, 2015 at 2:42 PM, Kees Cook <keescook@chromium.org> wrote: >> > On Thu, Dec 3, 2015 at 5:45 PM, yalin wang <yalin.wang2010@gmail.com> wrote: >> >> >> >>> On Dec 2, 2015, at 16:03, Kees Cook <keescook@chromium.org> wrote: >> >>> >> >>> Normally, when a user can modify a file that has setuid or setgid bits, >> >>> those bits are cleared when they are not the file owner or a member >> >>> of the group. This is enforced when using write and truncate but not >> >>> when writing to a shared mmap on the file. This could allow the file >> >>> writer to gain privileges by changing a binary without losing the >> >>> setuid/setgid/caps bits. >> >>> >> >>> Changing the bits requires holding inode->i_mutex, so it cannot be done >> >>> during the page fault (due to mmap_sem being held during the fault). >> >>> Instead, clear the bits if PROT_WRITE is being used at mmap time. >> >>> >> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >> >>> Cc: stable@vger.kernel.org >> >>> — >> >> >> >> is this means mprotect() sys call also need add this check? >> >> mprotect() can change to PROT_WRITE, then it can write to a >> >> read only map again , also a secure hole here . >> > >> > Yes, good point. This needs to be added. I will send a new patch. Thanks! >> >> This continues to look worse and worse. >> >> So... to check this at mprotect time, I have to know it's MAP_SHARED, >> but that's in the vma_flags, which I can only see after holding >> mmap_sem. >> >> The best I can think of now is to strip the bits at munmap time, since >> you can't execute an mmapped file until it closes. >> >> Jan, thoughts on this? > > Umm, so we actually refuse to execute a file while someone has it open for > writing (deny_write_access() in do_open_execat()). So dropping the suid / > sgid bits when closing file for writing could be plausible. Grabbing > i_mutex from __fput() context is safe (it gets called from task_work > context when returning to userspace). > > That way we could actually remove the checks done for each write. To avoid > unexpected removal of suid/sgid bits when someone just opens & closes the > file, we could mark the file as needing suid/sgid treatment by a flag in > inode->i_flags when file gets written to or mmaped and then check for this > in __fput(). Yeah, this is ultimately where I ended up for the v4 (and fixed up in v5). I added the flag to file, though, not inode. Sending v5 now... -Kees > > I've added Al Viro to CC just in case he is aware of some issues with > this... > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] clear file privilege bits when mmap writing @ 2015-12-09 22:52 ` Kees Cook 0 siblings, 0 replies; 17+ messages in thread From: Kees Cook @ 2015-12-09 22:52 UTC (permalink / raw) To: Jan Kara Cc: yalin wang, Andrew Morton, Willy Tarreau, Eric W. Biederman, Kirill A. Shutemov, Oleg Nesterov, Rik van Riel, Chen Gang, Davidlohr Bueso, Andrea Arcangeli, Linux-MM, LKML, Al Viro On Wed, Dec 9, 2015 at 12:26 AM, Jan Kara <jack@suse.cz> wrote: > On Mon 07-12-15 16:40:14, Kees Cook wrote: >> On Mon, Dec 7, 2015 at 2:42 PM, Kees Cook <keescook@chromium.org> wrote: >> > On Thu, Dec 3, 2015 at 5:45 PM, yalin wang <yalin.wang2010@gmail.com> wrote: >> >> >> >>> On Dec 2, 2015, at 16:03, Kees Cook <keescook@chromium.org> wrote: >> >>> >> >>> Normally, when a user can modify a file that has setuid or setgid bits, >> >>> those bits are cleared when they are not the file owner or a member >> >>> of the group. This is enforced when using write and truncate but not >> >>> when writing to a shared mmap on the file. This could allow the file >> >>> writer to gain privileges by changing a binary without losing the >> >>> setuid/setgid/caps bits. >> >>> >> >>> Changing the bits requires holding inode->i_mutex, so it cannot be done >> >>> during the page fault (due to mmap_sem being held during the fault). >> >>> Instead, clear the bits if PROT_WRITE is being used at mmap time. >> >>> >> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >> >>> Cc: stable@vger.kernel.org >> >>> — >> >> >> >> is this means mprotect() sys call also need add this check? >> >> mprotect() can change to PROT_WRITE, then it can write to a >> >> read only map again , also a secure hole here . >> > >> > Yes, good point. This needs to be added. I will send a new patch. Thanks! >> >> This continues to look worse and worse. >> >> So... to check this at mprotect time, I have to know it's MAP_SHARED, >> but that's in the vma_flags, which I can only see after holding >> mmap_sem. >> >> The best I can think of now is to strip the bits at munmap time, since >> you can't execute an mmapped file until it closes. >> >> Jan, thoughts on this? > > Umm, so we actually refuse to execute a file while someone has it open for > writing (deny_write_access() in do_open_execat()). So dropping the suid / > sgid bits when closing file for writing could be plausible. Grabbing > i_mutex from __fput() context is safe (it gets called from task_work > context when returning to userspace). > > That way we could actually remove the checks done for each write. To avoid > unexpected removal of suid/sgid bits when someone just opens & closes the > file, we could mark the file as needing suid/sgid treatment by a flag in > inode->i_flags when file gets written to or mmaped and then check for this > in __fput(). Yeah, this is ultimately where I ended up for the v4 (and fixed up in v5). I added the flag to file, though, not inode. Sending v5 now... -Kees > > I've added Al Viro to CC just in case he is aware of some issues with > this... > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR -- Kees Cook Chrome OS & Brillo Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2015-12-09 22:52 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-03 0:03 [PATCH v2] fs: clear file privilege bits when mmap writing Kees Cook 2015-12-03 0:18 ` Andrew Morton 2015-12-03 0:18 ` Andrew Morton 2015-12-03 16:07 ` Kees Cook 2015-12-03 16:07 ` Kees Cook 2015-12-03 18:19 ` Kees Cook 2015-12-03 18:19 ` Kees Cook 2015-12-04 1:45 ` [PATCH v2] " yalin wang 2015-12-04 1:45 ` yalin wang 2015-12-07 22:42 ` Kees Cook 2015-12-07 22:42 ` Kees Cook 2015-12-08 0:40 ` Kees Cook 2015-12-08 0:40 ` Kees Cook 2015-12-09 8:26 ` Jan Kara 2015-12-09 8:26 ` Jan Kara 2015-12-09 22:52 ` Kees Cook 2015-12-09 22:52 ` Kees Cook
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.