[Apparmor-dev] Re: apparmor-utils patches for profiling bugs,
network access toggles
Dominic Reynolds
dreynolds at suse.de
Thu Jul 19 21:06:53 MDT 2007
+++ Seth Arnold [19/07/07 18:36 -0700]:
> 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;
Yes. I'll make this change.
>
> > } 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 :)
>
Bad indent - I'll fix that.
> > + $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"
Access denied for network operation: tcp stream ?
>
> 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)
> ...
Thanks - thats a great idea I'll do that.
>
> 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?
The old rules were stored in a list and I changed it to a hash - I made the
changes in other parts of the code.
>
> > $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.
This is parsing the profile data so the format is
network [family] [stream]
>
> > } 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? :)
this is a bug that jesse pointed out too - I'll uncomment this.
>
> > }
> > -
> > 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.
Hmm. It can be:
- not defined (in the case of a rule "network" for all access)
- a value of 1 (in the case of rule "network FAMILY" for all sock types in
FAMILY)
- a reference to hash containing one or more types ( in case the profile has
one or more "network FAMILY TYPE" rules.
Agreed tho its an eyesore and I'll have a look at it.
>
>
> > + defined $sd{$profile}{$hat}{netdomain}{$family}{$sock_type} ) {
>
> ... exactly as done here! ;)
>
> > + return 1;
> > + } else {
> > + return 0;
> > + }
> > +}
> > +
> > sub reload ($) {
> > my $bin = shift;
> >
>
> Thanks Dominic!
Thanks for the review seth.
-dom
> _______________________________________________
> Apparmor-dev mailing list
> Apparmor-dev at forge.novell.com
> http://forge.novell.com/mailman/listinfo/apparmor-dev
-------------- 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/b258e2fb/attachment.pgp
More information about the Apparmor-dev
mailing list