Hi Philip,
Philip Guenther wrote on Sun, Jul 19, 2015 at 11:19:53AM -0700:
> On Sun, Jul 19, 2015 at 11:04 AM, Ingo Schwarze <schwa...@usta.de> wrote:
>> Philip Guenther wrote on Sun, Jul 19, 2015 at 10:28:57AM -0700:
>>> On Sun, Jul 19, 2015 at 10:24 AM, Ingo Schwarze <schwa...@usta.de> wrote:
>>>> I don't think we are vulnerable.
>>>>
>>>> If my analysis is accurate, the only user-controlled files
>>>> we open in security(8) are ~/.rhosts and ~/.shosts
>>>> in check_rhosts_content(). However, there is
>>>>
>>>> next unless -s $filename;
>>>>
>>>> right before the open(), and for fifos, -s returns false:
>>> TOCTOU race there. If they can hit the gap and move a fifo
>>> over a normal file between the test and the open, the open
>>> will hang. Should switch to sysopen() with O_NONBLOCK.
>> Oops, indeed.
>> Index: security
>> ===================================================================
>> RCS file: /cvs/src/libexec/security/security,v
>> retrieving revision 1.35
>> diff -u -p -r1.35 security
>> --- security 21 Apr 2015 10:24:22 -0000 1.35
>> +++ security 19 Jul 2015 18:02:38 -0000
>> @@ -22,7 +22,7 @@ use strict;
>>
>> use Digest::SHA qw(sha256_hex);
>> use Errno qw(ENOENT);
>> -use Fcntl qw(:mode);
>> +use Fcntl qw(O_RDONLY O_NONBLOCK :mode);
>> use File::Basename qw(basename);
>> use File::Compare qw(compare);
>> use File::Copy qw(copy);
>> @@ -371,7 +371,7 @@ sub check_rhosts_content {
>> foreach my $base (qw(rhosts shosts)) {
>> my $filename = "$home/.$base";
>> next unless -s $filename;
>> - nag !open(my $fh, '<', $filename),
>> + nag !sysopen(my $fh, $filename, O_RDONLY | O_NONBLOCK),
>> "open: $filename: $!"
>> and next;
>> local $_;
> You need to then test the resulting file handle to verify it was a
> normal file. I think just
> nag !-f $fh
> ?
I don't think that's very important. If users make their ~/.rhosts
file a fifo, the script won't warn because the -s above the open()
bails. If they go to the length of racing the -s to prevent that,
the above code will do about the same, in particular not warn either,
because the subsequent read won't return any data. So why should
we warn in that exotic case?
Then again, if you insist, nothing much is wrong with the patch
below, even though i'd probably mildly prefer the simpler one above.
Yours,
Ingo
Index: security
===================================================================
RCS file: /cvs/src/libexec/security/security,v
retrieving revision 1.35
diff -u -p -r1.35 security
--- security 21 Apr 2015 10:24:22 -0000 1.35
+++ security 20 Jul 2015 00:48:28 -0000
@@ -22,7 +22,7 @@ use strict;
use Digest::SHA qw(sha256_hex);
use Errno qw(ENOENT);
-use Fcntl qw(:mode);
+use Fcntl qw(O_RDONLY O_NONBLOCK :mode);
use File::Basename qw(basename);
use File::Compare qw(compare);
use File::Copy qw(copy);
@@ -371,8 +371,11 @@ sub check_rhosts_content {
foreach my $base (qw(rhosts shosts)) {
my $filename = "$home/.$base";
next unless -s $filename;
- nag !open(my $fh, '<', $filename),
+ nag !sysopen(my $fh, $filename, O_RDONLY | O_NONBLOCK),
"open: $filename: $!"
+ and next;
+ nag !(-f $fh),
+ "$filename is not a regular file"
and next;
local $_;
nag /^\+\s*$/,
