[Apparmor-dev] [RFR] apparmor-utils patches for profiling bugs,
network access toggles
Seth Arnold
seth.arnold at suse.de
Thu Jul 19 19:36:57 MDT 2007
On Wed, Jul 18, 2007 at 08:22:02PM -0600, Dominic Reynolds wrote:
> Index: trunk.dev/utils/SubDomain.pm
> ===================================================================
> --- trunk.dev.orig/utils/SubDomain.pm 2007-07-18 15:13:05.000000000 -0600
> +++ trunk.dev/utils/SubDomain.pm 2007-07-18 20:11:38.000000000 -0600
> @@ -1755,9 +1755,10 @@ sub add_audit_event_to_tree ( $$ ) {
> ($profile, $hat) = split /\/\//, $e->{name};
> }
> $hat = $profile if ( !$hat );
> - my @path = split(/\//, $profile);
> - my $prog = pop @path;
> -
> + # TODO - refactor add_to_tree as prog is no longer supplied
> + # HINT is from previous format where prog was not
> + # consistently passed
> + my $prog = "HINT";
>
> if ($e->{operation} eq "exec") {
> add_to_tree( $e->{pid},
> @@ -1861,6 +1862,18 @@ sub add_audit_event_to_tree ( $$ ) {
> $e->{denied_mask},
> $e->{name}
> );
> + } elsif ($e->{operation} eq "clone") {
> + my ($parent, $child) = ($e->{pid}, $e->{task});
> + $profile ||= "null-complain-profile";
> + $hat ||= "null-complain-profile";
> + my $arrayref = [];
> + if (exists $pid{$e->{pid}}) {
> + push @{ $pid{$parent} }, $arrayref;
> + } else {
> + push @log, $arrayref;
> + }
> + $pid{$child} = $arrayref;
> + push @{$arrayref}, [ "fork", $child, $profile, $hat ];
Can the above be re-written like this?
+ } elsif ($e->{operation} eq "clone") {
+ my ($parent, $child) = ($e->{pid}, $e->{task});
+ $profile ||= "null-complain-profile";
+ $hat ||= "null-complain-profile";
+ --> my $arrayref = [ "fork", $child, $profile, $hat ];
+ if (exists $pid{$e->{pid}}) {
+ push @{ $pid{$parent} }, $arrayref;
+ } else {
+ push @log, $arrayref;
+ }
+ $pid{$child} = $arrayref;
> } elsif ($e->{operation} eq "change_hat") {
> add_to_tree($e->{pid}, "unknown_hat", $profile, $hat, $sdmode, $hat);
> } else {
> Index: trunk.dev/utils/SubDomain.pm
> ===================================================================
> --- trunk.dev.orig/utils/SubDomain.pm 2007-07-18 16:05:33.000000000 -0600
> +++ trunk.dev/utils/SubDomain.pm 2007-07-18 16:05:37.000000000 -0600
> @@ -1158,6 +1158,8 @@ my %CMDS = (
> CMD_ASK_LATER => "Ask Me (L)ater",
> CMD_YES => "(Y)es",
> CMD_NO => "(N)o",
> + CMD_ALL_NET => "Allow All (N)etwork",
> + CMD_NET_FAMILY => "Allow Network Fa(m)ily",
> );
>
> sub UI_PromptUser ($) {
> @@ -1677,6 +1679,23 @@ sub handlechildren {
> return if $domainchange eq "change";
> }
> }
> + } elsif ( $type eq "netdomain" ) {
> + my ($pid, $p, $h, $prog, $sdmode, $family, $sock_type, $protocol) =
> + @entry;
> +
> + if ( ($p !~ /null(-complain)*-profile/)
> + && ($h !~ /null(-complain)*-profile/))
Jesse, John, do we still have a null-profile?
> + {
> + $profile = $p;
> + $hat = $h;
> + }
> +
> + # print "$pid $profile $hat $prog $sdmode capability $capability\n";
> +
> + next unless $profile && $hat;
Could be re-written as:
+ if ( ($p !~ /null(-complain)*-profile/)
+ && ($h !~ /null(-complain)*-profile/))
+ {
+ $profile = $p;
+ $hat = $h;
+ } else {
+ next; # any debug needed?
+ }
> + $prelog{$sdmode}{$profile}{$hat}{netdomain}{$family}{$sock_type} = 1;
> +
> }
> }
> }
> @@ -1684,10 +1703,11 @@ sub handlechildren {
>
> sub add_to_tree ($@) {
> my ($pid, $type, @event) = @_;
> - my $eventmsg = Data::Dumper->Dump([@event], [qw(*event)]);
> - $eventmsg =~ s/\n/ /g;
> - $DEBUGGING && debug " ADD_TO_TREE: pid [$pid] type [$type] event [ $eventmsg
> - ]";
> + if ( $DEBUGGING ) {
> + my $eventmsg = Data::Dumper->Dump([@event], [qw(*event)]);
> + $eventmsg =~ s/\n/ /g;
> + debug " add_to_tree: pid [$pid] type [$type] event [ $eventmsg ]";
> + }
>
> unless (exists $pid{$pid}) {
> my $arrayref = [];
> @@ -1874,10 +1894,24 @@ sub add_audit_event_to_tree ( $$ ) {
> }
> $pid{$child} = $arrayref;
> push @{$arrayref}, [ "fork", $child, $profile, $hat ];
> + } elsif ($e->{operation} =~ "socket_") {
Should that be m/^socket_.*/ ? m/socket_/ ? similar..?
> + add_to_tree( $e->{pid},
> + "netdomain",
> + $profile,
> + $hat,
> + $prog,
> + $sdmode,
> + $e->{family},
Odd spacing here. I sure hope this list is right :)
> + $e->{sock_type},
> + $e->{protocol},
> + );
> } elsif ($e->{operation} eq "change_hat") {
> add_to_tree($e->{pid}, "unknown_hat", $profile, $hat, $sdmode, $hat);
> } else {
> - $DEBUGGING && debug "UNHANDLED: $_";
> + if ( $DEBUGGING ) {
> + my $msg = Data::Dumper->Dump([$e], [qw(*event)]);
> + debug "UNHANDLED: $msg";
> + }
> }
> }
>
> @@ -2641,6 +2675,88 @@ sub ask_the_questions {
> }
> }
> }
> +
> + # and then step through all of the netdomain entries...
> + for my $family (sort keys %{$log{$sdmode}
> + {$profile}
> + {$hat}
> + {netdomain}}) {
> +
> + # TODO - severity handling for net toggles
> + #my $severity = $sevdb->rank();
uhoh :)
> + for my $sock_type (sort keys %{$log{$sdmode}
> + {$profile}
> + {$hat}
> + {netdomain}
> + {$family}}) {
> +
> + # we don't care about it if we've already added it to the
> + # profile
> + next if ( network_access_check( $profile,
> + $hat,
> + $family,
> + $sock_type
> + )
> + );
> +
> + my $q = {};
> + $q->{headers} = [];
> + push @{ $q->{headers} },
> + gettext("Profile"),
> + combine_name($profile, $hat);
> + push @{ $q->{headers} },
> + gettext("Network Family"),
> + $family;
> + push @{ $q->{headers} },
> + gettext("Socket Type"),
> + $sock_type;
> +
> + $q->{functions} = [
> + "CMD_ALLOW",
> + "CMD_DENY",
> + "CMD_ABORT",
> + "CMD_FINISHED"
> + ];
> +
> + # complain-mode events default to allow - enforce defaults
> + # to deny
> + $q->{default} = ($sdmode eq "PERMITTING") ? "CMD_ALLOW" :
> + "CMD_DENY";
> +
> + $seenevents++;
> +
> + # what did the grand exalted master tell us to do?
> + my $ans = UI_PromptUser($q);
> +
> + if ($ans eq "CMD_ALLOW") {
> +
> + # they picked (a)llow, so...
> +
> + # stick the whole rule into the profile
> + $sd{$profile}{$hat}{netdomain}{$family}{$sock_type} = 1;
> +
> + # mark this profile as changed
> + $changed{$profile} = 1;
> +
> + # give a little feedback to the user
> + UI_Info(sprintf(
> + gettext('Adding network rule %s %s to profile.'),
> + $family,
> + $sock_type
> + )
> + );
> + } elsif ($ans eq "CMD_DENY") {
> + UI_Info(sprintf(
> + gettext('Denying network rule %s %s to profile.'),
Extra space, but feels a little strange regardless of the space..
"Denying network rule stream tcp to profile"
Perhaps "next rule..." or "not adding rule" or something would make more
sense, even if it is less explicit.
> + $family,
> + $sock_type
> + )
> + );
> + } else {
> + redo;
> + }
> + }
> + }
> }
> }
> }
> @@ -3447,6 +3563,45 @@ sub collapselog () {
> $log{$sdmode}{$profile}{$hat}{capability}{$capability} = 1;
> }
> }
> +
> + # Network toggle handling - let the hideous formatting begin
> + for my $family ( keys %{$prelog{$sdmode}
> + {$profile}
> + {$hat}
> + {netdomain} } ) {
> + for my $sock_type ( keys %{$prelog{$sdmode}
> + {$profile}
> + {$hat}
> + {netdomain}
> + {$family} } ) {
> + unless ( defined $sd{$profile}{$hat}{netdomain}{all} ||
> + ( defined $sd{$profile}
> + {$hat}
> + {netdomain}
> + {$family} &&
> + $sd{$profile}{$hat}{netdomain}{$family} == 1
> + ) ||
> + ( defined $sd{$profile}
> + {$hat}
> + {netdomain}
> + {$family}
> + {$sock_type} &&
> + $sd{$profile}
> + {$hat}
> + {netdomain}
> + {$family}
> + {$sock_type} == 1
> + )
> + ) {
Yikes. Would you mind adding a comment before the unless that describes
what is being checked? or perhaps add a few variables like this:
$all_enabled = defined $sd{$profile}{$hat}{netdomain}{all};
$family_enabled = defined $sd{$profile}{$hat}{netdomain}{$family} &&
($sd{$profile}{$hat}{netdomain}{$family} == 1)
...
Then the test at the end could be:
if ($all_enabled or $Family_enbled or ...) {
...
}
It's hard to figure out what it is supposed to do here ;)
> + $log{$sdmode}
> + {$profile}
> + {$hat}
> + {netdomain}
> + {$family}
> + {$sock_type}=1;
All that to assign a 1 someplace :)
> + }
> + }
> + }
> }
> }
> }
> @@ -3661,7 +3816,7 @@ sub parse_profile_data {
> $profile_data->{$profile}{$hat}{flags} = $flags;
> }
>
> - $profile_data->{$profile}{$hat}{netdomain} = [];
> + $profile_data->{$profile}{$hat}{netdomain} = { };
This is strange.. Did anything touch {netdomain} before? If so, were
they all updated?
> $profile_data->{$profile}{$hat}{path} = { };
>
> # store off initial comment if they have one
> @@ -3750,6 +3905,22 @@ sub parse_profile_data {
>
> return $ret if ( $ret != 0 );
>
> + } elsif (/^\s*network/) {
> + if (not $profile) {
> + die sprintf(gettext('%s contains syntax errors.'), $file) . "\n";
> + }
> +
> + unless ($profile_data->{$profile}{$hat}{netdomain}) {
> + $profile_data->{$profile}{$hat}{netdomain} = { };
> + }
> +
> + if ( /^\s*network\s+(\S+)\s*,\s*$/ ) {
> + $profile_data->{$profile}{$hat}{netdomain}{$1} = 1;
> + } elsif ( /^\s*network\s+(\S+)\s+(\S+)\s*,\s*$/ ) {
> + $profile_data->{$profile}{$hat}{netdomain}{$1}{$2} = 1;
> + } else {
> + $profile_data->{$profile}{$hat}{netdomain}{all} = 1;
> + }
I sure hope these are correct... it'd be nice to have cut-n-pastes of
either the format strings from the kernel, or perhaps (with our new
logging mechanism :) cut-n-pastes of output.
> } elsif (/^\s*(tcp_connect|tcp_accept|udp_send|udp_receive)/) {
> if (not $profile) {
> die sprintf(gettext('%s contains syntax errors.'), $file) . "\n";
> @@ -3795,7 +3966,7 @@ sub parse_profile_data {
> }
>
> $profile_data->{$profile}{$hat}{path} = { };
> - $profile_data->{$profile}{$hat}{netdomain} = [];
> + $profile_data->{$profile}{$hat}{netdomain} = { };
>
> # store off initial comment if they have one
> $profile_data->{$profile}{$hat}{initial_comment} = $initial_comment
> @@ -3930,12 +4101,21 @@ sub writenetdomain ($) {
> my @data;
> # dump out the netdomain entries...
> if (exists $profile_data->{netdomain}) {
> - for my $nd (sort @{$profile_data->{netdomain}}) {
> - push @data, " $nd,";
> + if ( $profile_data->{netdomain} == 1 ) {
> + push @data, " network,";
> + } else {
> + for my $fam (keys %{$profile_data->{netdomain}}) {
> + if ( $profile_data->{netdomain}{$fam} == 1 ) {
> + push @data, " network $fam,";
> + } else {
> + for my $type (keys %{$profile_data->{netdomain}{$fam} }) {
> + push @data, " network $fam $type,";
> + }
> + }
> + }
> }
> - push @data, "" if @{$profile_data->{netdomain}};
> + #push @data, "" if %{$profile_data->{netdomain}};
What was this here for? :)
> }
> -
> return @data;
> }
>
> @@ -4098,6 +4278,18 @@ sub matchliteral {
> return $matches;
> }
>
> +sub network_access_check ($$$$) {
> + my ($profile, $hat, $family, $sock_type) = @_;
> + if ( defined $sd{$profile}{$hat}{netdomain}{all} ||
> + ( defined $sd{$profile}{$hat}{netdomain}{$family} &&
> + $sd{$profile}{$hat}{netdomain}{$family} == 1 ) ||
Maybe we should just use the 'defined' as the test always -- checking
for the == 1 is a real eyesore after checking for the definition in the
first place.
> + defined $sd{$profile}{$hat}{netdomain}{$family}{$sock_type} ) {
... exactly as done here! ;)
> + return 1;
> + } else {
> + return 0;
> + }
> +}
> +
> sub reload ($) {
> my $bin = shift;
>
Thanks Dominic!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://forge.novell.com/pipermail/apparmor-dev/attachments/20070719/5bdaff82/attachment.pgp
More information about the Apparmor-dev
mailing list