Skip to content

Commit

Permalink
patch 8.0.1263: others can read the swap file if a user is careless
Browse files Browse the repository at this point in the history
Problem:    Others can read the swap file if a user is careless with his
            primary group.
Solution:   If the group permission allows for reading but the world
            permissions doesn't, make sure the group is right.
  • Loading branch information
brammool committed Nov 4, 2017
1 parent 7dd88c5 commit 5a73e0c
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 40 deletions.
1 change: 1 addition & 0 deletions src/Makefile
Expand Up @@ -2259,6 +2259,7 @@ test_arglist \
test_stat \
test_statusline \
test_substitute \
test_swap \
test_syn_attr \
test_syntax \
test_system \
Expand Down
24 changes: 23 additions & 1 deletion src/fileio.c
Expand Up @@ -716,7 +716,29 @@ readfile(
/* Set swap file protection bits after creating it. */
if (swap_mode > 0 && curbuf->b_ml.ml_mfp != NULL
&& curbuf->b_ml.ml_mfp->mf_fname != NULL)
(void)mch_setperm(curbuf->b_ml.ml_mfp->mf_fname, (long)swap_mode);
{
char_u *swap_fname = curbuf->b_ml.ml_mfp->mf_fname;

/*
* If the group-read bit is set but not the world-read bit, then
* the group must be equal to the group of the original file. If
* we can't make that happen then reset the group-read bit. This
* avoids making the swap file readable to more users when the
* primary group of the user is too permissive.
*/
if ((swap_mode & 044) == 040)
{
stat_T swap_st;

if (mch_stat((char *)swap_fname, &swap_st) >= 0
&& st.st_gid != swap_st.st_gid
&& fchown(curbuf->b_ml.ml_mfp->mf_fd, -1, st.st_gid)
== -1)
swap_mode &= 0600;
}

(void)mch_setperm(swap_fname, (long)swap_mode);
}
#endif
}

Expand Down
112 changes: 73 additions & 39 deletions src/testdir/test_swap.vim
@@ -1,48 +1,82 @@
" Tests for the swap feature

" Tests for 'directory' option.
func Test_swap_directory()
"" Tests for 'directory' option.
"func Test_swap_directory()
" if !has("unix")
" return
" endif
" let content = ['start of testfile',
" \ 'line 2 Abcdefghij',
" \ 'line 3 Abcdefghij',
" \ 'end of testfile']
" call writefile(content, 'Xtest1')
"
" " '.', swap file in the same directory as file
" set dir=.,~
"
" " Verify that the swap file doesn't exist in the current directory
" call assert_equal([], glob(".Xtest1*.swp", 1, 1, 1))
" edit Xtest1
" let swfname = split(execute("swapname"))[0]
" call assert_equal([swfname], glob(swfname, 1, 1, 1))
"
" " './dir', swap file in a directory relative to the file
" set dir=./Xtest2,.,~
"
" call mkdir("Xtest2")
" edit Xtest1
" call assert_equal([], glob(swfname, 1, 1, 1))
" let swfname = "Xtest2/Xtest1.swp"
" call assert_equal(swfname, split(execute("swapname"))[0])
" call assert_equal([swfname], glob("Xtest2/*", 1, 1, 1))
"
" " 'dir', swap file in directory relative to the current dir
" set dir=Xtest.je,~
"
" call mkdir("Xtest.je")
" call writefile(content, 'Xtest2/Xtest3')
" edit Xtest2/Xtest3
" call assert_equal(["Xtest2/Xtest3"], glob("Xtest2/*", 1, 1, 1))
" let swfname = "Xtest.je/Xtest3.swp"
" call assert_equal(swfname, split(execute("swapname"))[0])
" call assert_equal([swfname], glob("Xtest.je/*", 1, 1, 1))
"
" set dir&
" call delete("Xtest1")
" call delete("Xtest2", "rf")
" call delete("Xtest.je", "rf")
"endfunc

func Test_swap_group()
if !has("unix")
return
endif
let content = ['start of testfile',
\ 'line 2 Abcdefghij',
\ 'line 3 Abcdefghij',
\ 'end of testfile']
call writefile(content, 'Xtest1')

" '.', swap file in the same directory as file
set dir=.,~

" Verify that the swap file doesn't exist in the current directory
call assert_equal([], glob(".Xtest1*.swp", 1, 1, 1))
edit Xtest1
let swfname = split(execute("swapname"))[0]
call assert_equal([swfname], glob(swfname, 1, 1, 1))

" './dir', swap file in a directory relative to the file
set dir=./Xtest2,.,~

call mkdir("Xtest2")
edit Xtest1
call assert_equal([], glob(swfname, 1, 1, 1))
let swfname = "Xtest2/Xtest1.swp"
call assert_equal(swfname, split(execute("swapname"))[0])
call assert_equal([swfname], glob("Xtest2/*", 1, 1, 1))
let groups = split(system('groups'))
if len(groups) <= 1
throw 'Skipped: need at least two groups, got ' . groups
endif

" 'dir', swap file in directory relative to the current dir
set dir=Xtest.je,~
call delete('Xtest')
split Xtest
call setline(1, 'just some text')
wq
if system('ls -l Xtest') !~ ' ' . groups[0] . ' \d'
throw 'Skipped: test file does not have the first group'
else
silent !chmod 640 Xtest
call system('chgrp ' . groups[1] . ' Xtest')
if system('ls -l Xtest') !~ ' ' . groups[1] . ' \d'
throw 'Skipped: cannot set second group on test file'
else
split Xtest
let swapname = substitute(execute('swapname'), '[[:space:]]', '', 'g')
call assert_match('Xtest', swapname)
" Group of swapfile must now match original file.
call assert_match(' ' . groups[1] . ' \d', system('ls -l ' . swapname))

call mkdir("Xtest.je")
call writefile(content, 'Xtest2/Xtest3')
edit Xtest2/Xtest3
call assert_equal(["Xtest2/Xtest3"], glob("Xtest2/*", 1, 1, 1))
let swfname = "Xtest.je/Xtest3.swp"
call assert_equal(swfname, split(execute("swapname"))[0])
call assert_equal([swfname], glob("Xtest.je/*", 1, 1, 1))
bwipe!
endif
endif

set dir&
call delete("Xtest1")
call delete("Xtest2", "rf")
call delete("Xtest.je", "rf")
call delete('Xtest')
endfunc
2 changes: 2 additions & 0 deletions src/version.c
Expand Up @@ -761,6 +761,8 @@ static char *(features[]) =

static int included_patches[] =
{ /* Add new patch number below this line */
/**/
1263,
/**/
1262,
/**/
Expand Down

5 comments on commit 5a73e0c

@kirotawa
Copy link

@kirotawa kirotawa commented on 5a73e0c Nov 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I did apply this patch (only the part of src/fileio.c)
And tried to reproduce the steps I found here http://www.openwall.com/lists/oss-security/2017/10/31/15

And what I see is that swap files still are created ignoring the umask. Not sure though if those steps are the correct one to test this. Any advice here would be nice. Thanks!

@chrisbra
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, this commit does not talk about respecting the umask. It only changes the group ownership

@Z5T1
Copy link

@Z5T1 Z5T1 commented on 5a73e0c Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been assigned CVE-2017-17087. After careful examination, it was decided that this is a separate vulnerability from CVE-2017-1000382 and has nothing to do with the umask.

@xtspackages
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit does not guard the new use of fchown() in src/fileio.c. Elsewhere fchown is protected inside a "#ifdef HAVE_FCHOWN" block. I've attached a patch to this comment.

vim_guard_fchown.txt

@brammool
Copy link
Contributor Author

@brammool brammool commented on 5a73e0c Apr 19, 2018 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.