[libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action
John Ferlan
jferlan at redhat.com
Mon Jul 23 21:01:21 UTC 2018
On 07/23/2018 04:44 AM, Michal Prívozník wrote:
> 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", ...)
Well, thanks for the missing details... Guess I could have read the last
sentence of the comment a lot more closely or perhaps maybe the whole
thing needs to be more obvious as to what the options are.
So maybe rather than:
# following formats:
#
# $path: $progname: $testname
# $path: $action: $progname: $testname
it's:
# following two accepted formats:
#
# $path: $progname: $testname
# or
# $path: $action: $progname: $testname
...
and
Rather than
# 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".
it's:
# Moreover, $action, $progname and $testname can be empty, in which
# case $path is allowed for all tests.
#
# If $action is specified, then it must be one of "open", "fopen",
# "access", "stat", "lstat", or "connect" followed by $progname and
# $testname.
#
# The $progname and $testname specify...
[BTW: explained much better than I can even attempt to retry below and I
think perfectly reasonable to put this entirely in the file].
>
> 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'.
Huh, hmm, say what? Interesting. I've learned something new I think.
I'm good for the week now, right?
> True, we are missing a lot of rules there. But I have an idea how to
> fill them out ;-)
>
And as as noted previously, my scripting review is not really that good
and throw in regex, and well I'm out of my comfort zone. Still using the
"pattern" from the previous code for "$path: $progname: $testname" where
$1 is $path, $3 is $progname, and $5 is testname, what I don't
understand then is why the above for "$path: $action: $progname:
$testname" has $1 for $path, $3 for $action, $4 for $progname, and $6
for $testname. Speaking only from the "pattern" POV I would have though
$5 for $progname and $7 for $testname.
Similarly for the other file it was $1, $2, $4 and now it's $1, $2, $3,
and $5.
But beyond adding some doc wording, that's about as good as it gets from
me for any sort of perl script... sorry I cannot be more help.
John
More information about the libvir-list
mailing list