[libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action

[Michal Prívozník]

On 07/21/2018 02:57 PM, John Ferlan wrote:
> 
> 
> On 07/12/2018 03:37 AM, Michal Privoznik wrote:
>> The check-file-access.pl script is used to match access list
>> generated by virtestmock against whitelisted rules stored in
>> file_access_whitelist.txt. So far the rules are in form:
>>
>>   $path: $progname: $testname
>>
>> This is not sufficient because the rule does not take into
>> account 'action' that caused $path to appear in the list of
>> accessed files. After this commit the rule can be in new form:
>>
>>   $path: $action: $progname: $testname
>>
>> where $action is one from ("open", "fopen", "access", "stat",
>> "lstat", "connect"). This way the white list can be fine tuned to
>> allow say access() but not connect().
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  tests/check-file-access.pl      | 32 +++++++++++++++++++++++++++-----
>>  tests/file_access_whitelist.txt | 15 ++++++++++-----
>>  2 files changed, 37 insertions(+), 10 deletions(-)
>>
> 
> Perl scripts, not my area of expertise...  Even worse, regexes.
> 
> Still I am unclear about this particular one because you stuffed $action
> in front of something that already existed and it makes me wonder how
> that affects existing files. [1]
> 
> 
>> diff --git a/tests/check-file-access.pl b/tests/check-file-access.pl
>> index 977a2bc533..ea0b7a18a2 100755
>> --- a/tests/check-file-access.pl
>> +++ b/tests/check-file-access.pl
>> @@ -27,18 +27,21 @@ use warnings;
>>  my $access_file = "test_file_access.txt";
>>  my $whitelist_file = "file_access_whitelist.txt";
>>  
>> +my @known_actions = ("open", "fopen", "access", "stat", "lstat", "connect");
>> +
>>  my @files;
>>  my @whitelist;
>>  
>>  open FILE, "<", $access_file or die "Unable to open $access_file: $!";
>>  while (<FILE>) {
>>      chomp;
>> -    if (/^(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
>> +    if (/^(\S*):\s*(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
>>          my %rec;
>>          ${rec}{path} = $1;
>> -        ${rec}{progname} = $2;
>> -        if (defined $4) {
>> -            ${rec}{testname} = $4;
>> +        ${rec}{action} = $2;
>> +        ${rec}{progname} = $3;
>> +        if (defined $5) {
>> +            ${rec}{testname} = $5;
>>          }
>>          push (@files, \%rec);
>>      } else {
>> @@ -52,7 +55,21 @@ while (<FILE>) {
>>      chomp;
>>      if (/^\s*#.*$/) {
>>          # comment
>> +    } elsif (/^(\S*):\s*(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/ and
>> +            grep /^$2$/, @known_actions) {
>> +        # $path: $action: $progname: $testname
>> +        my %rec;
>> +        ${rec}{path} = $1;
>> +        ${rec}{action} = $3;
>> +        if (defined $4) {
>> +            ${rec}{progname} = $4;
>> +        }
>> +        if (defined $6) {
>> +            ${rec}{testname} = $6;
>> +        }
>> +        push (@whitelist, \%rec);
>>      } elsif (/^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/) {
>> +        # $path: $progname: $testname
>>          my %rec;
>>          ${rec}{path} = $1;
>>          if (defined $3) {
>> @@ -79,6 +96,11 @@ for my $file (@files) {
>>              next;
>>          }
>>  
>> +        if (defined %${rule}{action} and
>> +            not %${file}{action} =~ m/^$rule->{action}$/) {
>> +            next;
>> +        }
>> +
>>          if (defined %${rule}{progname} and
>>              not %${file}{progname} =~ m/^$rule->{progname}$/) {
>>              next;
>> @@ -95,7 +117,7 @@ for my $file (@files) {
>>  
>>      if (not $match) {
>>          $error = 1;
>> -        print "$file->{path}: $file->{progname}";
>> +        print "$file->{path}: $file->{action}: $file->{progname}";
>>          print ": $file->{testname}" if defined %${file}{testname};
>>          print "\n";
>>      }
>> diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt
>> index 850b28506e..3fb318cbab 100644
>> --- a/tests/file_access_whitelist.txt
>> +++ b/tests/file_access_whitelist.txt
>> @@ -1,14 +1,17 @@
>>  # This is a whitelist that allows accesses to files not in our
>>  # build directory nor source directory. The records are in the
>> -# following format:
>> +# following formats:
>>  #
>>  #  $path: $progname: $testname
>> +#  $path: $action: $progname: $testname
>>  #
>> -# All these three are evaluated as perl RE. So to allow /dev/sda
>> -# and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
>> +# All these variables are evaluated as perl RE. So to allow
>> +# /dev/sda and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
>>  # /proc/$pid/status you can '/proc/\d+/status' and so on.
>> -# Moreover, $progname and $testname can be empty, in which which
>> -# case $path is allowed for all tests.
>> +# Moreover, $action, $progname and $testname can be empty, in which
>> +# which case $path is allowed for all tests. However, $action (if
>> +# specified) must be one of "open", "fopen", "access", "stat",
>> +# "lstat", "connect".
>>  
>>  /bin/cat: sysinfotest
>>  /bin/dirname: sysinfotest: x86 sysinfo
> 
> [1] For example, why wouldn't sysinfotest in this case relate to $action
> instead of $progname?

Because "sysinfotest" is not one of the array ("open", "fopen",
"access", ...)

The idea is to have two sets of rules:

/some/path: program
/some/path: open: program

The former allows just ANY access to /some/path for program "program",
i.e. it can open, fopen, access, stat, lstat, connect to it.
The latter allows only one action over /some/path. For instance
open("/some/path") is allowed but stat("/some/path") isn't.

To complicate things a bit more, if I wanted to allow /some/path for
every program, instead of having one rule per each program I can only have:

/some/path
/some/path: open

The former allows ANY access to /some/path for ANY program, the latter
allows open("/some/path") for ANY program but disallows other types of
access. Joining types of access is not supported yet. For instance:

/some/path: open stat

has to be written as:

/some/path: open
/some/path: stat

And when I say allowed what I really mean is: reported by 'make
check-access'.
True, we are missing a lot of rules there. But I have an idea how to
fill them out ;-)

Michal