[apparmor] [patch] parser - add support for variable expansion in dbus rules v2
Tyler Hicks
tyhicks at canonical.com
Thu Aug 29 18:40:08 UTC 2013
On 2013-08-29 10:50:35, Steve Beattie wrote:
>
> Subject: parser - add support for variable expansion in dbus rules v2
> Bug: https://bugs.launchpad.net/bugs/1218099
>
> This patch adds support for expanding variables with dbus rules.
> Specifically, they can expanded within the bus, name, path, member,
> interface, and peer label fields.
>
> Parser test cases and regression test cases are added as well.
>
> Patch history:
> v1: initial version of patch
> v2: add equality.sh tests to verify that the results of using
> variable expansion is the same as what should be equivalent rules
>
> Signed-off-by: Steve Beattie <sbeattie at ubuntu.com>
> ---
The patch looks really nice. Thanks for fixing this!
I have one minor nitpick below about memory allocation failures that I
think should be addressed. After that, add my ack.
Acked-by: Tyler Hicks <tyhicks at canonical.com>
> parser/dbus.c | 24 ++++++++++++
> parser/dbus.h | 1
> parser/parser_variable.c | 54 ++++++++++++++++++++++++++++
> parser/tst/equality.sh | 29 +++++++++++++++
> parser/tst/simple_tests/vars/vars_dbus_1.sd | 11 +++++
> parser/tst/simple_tests/vars/vars_dbus_2.sd | 11 +++++
> parser/tst/simple_tests/vars/vars_dbus_3.sd | 10 +++++
> parser/tst/simple_tests/vars/vars_dbus_4.sd | 13 ++++++
> parser/tst/simple_tests/vars/vars_dbus_5.sd | 12 ++++++
> parser/tst/simple_tests/vars/vars_dbus_6.sd | 11 +++++
> parser/tst/simple_tests/vars/vars_dbus_7.sd | 11 +++++
> tests/regression/apparmor/dbus.inc | 11 +++++
> tests/regression/apparmor/dbus_message.sh | 28 ++++++++++++++
> 13 files changed, 226 insertions(+)
>
> Index: b/parser/tst/simple_tests/vars/vars_dbus_1.sd
> ===================================================================
> --- /dev/null
> +++ b/parser/tst/simple_tests/vars/vars_dbus_1.sd
> @@ -0,0 +1,11 @@
> +#=DESCRIPTION reference variables in dbus rules
> +#=EXRESULT PASS
> +
> +@{FOO}=bar baz
> +@{BAR}=@{FOO} blort
> +
> +/does/not/exist {
> + dbus (send)
> + bus=session
> + path="/com/canonical/hud/applications/@{BAR}",
> +}
> Index: b/parser/parser_variable.c
> ===================================================================
> --- a/parser/parser_variable.c
> +++ b/parser/parser_variable.c
> @@ -29,6 +29,7 @@
>
> #include "parser.h"
> #include "mount.h"
> +#include "dbus.h"
>
> static inline char *get_var_end(char *var)
> {
> @@ -213,6 +214,19 @@ int clone_and_chain_mnt(void *v)
> return 1;
> }
>
> +int clone_and_chain_dbus(void *v)
> +{
> + struct dbus_entry *entry = v;
> +
> + struct dbus_entry *dup = dup_dbus_entry(entry);
> + if (!dup)
> + return 0;
> +
> + entry->next = dup;
> +
> + return 1;
> +}
> +
> static int process_variables_in_entries(struct cod_entry *entry_list)
> {
> int ret = TRUE, rc;
> @@ -253,6 +267,42 @@ static int process_variables_in_mnt_entr
> return ret;
> }
>
> +static int process_dbus_variables(struct dbus_entry *entry_list)
> +{
> + int ret = TRUE, rc;
> + struct dbus_entry *entry;
> +
> + list_for_each(entry_list, entry) {
> + rc = expand_entry_variables(&entry->bus, entry,
> + clone_and_chain_dbus);
> + if (!rc)
> + return FALSE;
> + rc = expand_entry_variables(&entry->name, entry,
> + clone_and_chain_dbus);
> + if (!rc)
> + return FALSE;
> + rc = expand_entry_variables(&entry->peer_label, entry,
> + clone_and_chain_dbus);
> + if (!rc)
> + return FALSE;
> + rc = expand_entry_variables(&entry->path, entry,
> + clone_and_chain_dbus);
> + if (!rc)
> + return FALSE;
> + rc = expand_entry_variables(&entry->interface, entry,
> + clone_and_chain_dbus);
> + if (!rc)
> + return FALSE;
> + rc = expand_entry_variables(&entry->member, entry,
> + clone_and_chain_dbus);
> + if (!rc)
> + return FALSE;
> +
> + }
> +
> + return ret;
> +}
> +
> int process_variables(struct codomain *cod)
> {
> int error = 0;
> @@ -265,6 +315,10 @@ int process_variables(struct codomain *c
> error = -1;
> }
>
> + if (!process_dbus_variables(cod->dbus_ents)) {
> + error = -1;
> + }
> +
> if (process_hat_variables(cod) != 0) {
> error = -1;
> }
> Index: b/parser/dbus.c
> ===================================================================
> --- a/parser/dbus.c
> +++ b/parser/dbus.c
> @@ -141,6 +141,30 @@ out:
> return ent;
> }
>
> +#define DUP_STRING(orig, new, field) \
> + (new)->field = (orig)->field ? strdup((orig)->field) : NULL
> +
> +struct dbus_entry *dup_dbus_entry(struct dbus_entry *orig)
> +{
> + struct dbus_entry *ent = NULL;
> + ent = (struct dbus_entry *) calloc(1, sizeof(struct dbus_entry));
> + if (!ent)
> + return NULL;
> +
> + DUP_STRING(orig, ent, bus);
> + DUP_STRING(orig, ent, name);
> + DUP_STRING(orig, ent, peer_label);
> + DUP_STRING(orig, ent, path);
> + DUP_STRING(orig, ent, interface);
> + DUP_STRING(orig, ent, member);
There should be error checking on these strdup()'s. Otherwise, ent could
be returned with NULL pointers in fields where orig didn't have any.
> + ent->mode = orig->mode;
> + ent->audit = orig->audit;
> + ent->deny = orig->deny;
> +
> + ent->next = orig->next;
> +
> + return ent;
> +}
>
> void print_dbus_entry(struct dbus_entry *ent)
> {
> Index: b/parser/dbus.h
> ===================================================================
> --- a/parser/dbus.h
> +++ b/parser/dbus.h
> @@ -43,6 +43,7 @@ struct dbus_entry {
> void free_dbus_entry(struct dbus_entry *ent);
> struct dbus_entry *new_dbus_entry(int mode, struct cond_entry *conds,
> struct cond_entry *peer_conds);
> +struct dbus_entry *dup_dbus_entry(struct dbus_entry *ent);
> void print_dbus_entry(struct dbus_entry *ent);
>
> #endif /* __AA_DBUS_H */
> Index: b/parser/tst/simple_tests/vars/vars_dbus_2.sd
> ===================================================================
> --- /dev/null
> +++ b/parser/tst/simple_tests/vars/vars_dbus_2.sd
> @@ -0,0 +1,11 @@
> +#=DESCRIPTION reference variables in dbus rules, interfaces
> +#=EXRESULT PASS
> +
> +@{ORGS}=freedesktop ubuntu gnome kde
> +
> +/does/not/exist {
> + dbus (receive)
> + bus=accessibility
> + interface=org.@{ORGS}.DBus.Properties
> + path="/com/canonical/hud/applications/bar",
> +}
> Index: b/parser/tst/simple_tests/vars/vars_dbus_3.sd
> ===================================================================
> --- /dev/null
> +++ b/parser/tst/simple_tests/vars/vars_dbus_3.sd
> @@ -0,0 +1,10 @@
> +#=DESCRIPTION reference variables in dbus rules, bus fields
> +#=EXRESULT PASS
> +
> +@{BUSES}=session system accessability choochoo
> +
> +/does/not/exist {
> + dbus (send)
> + bus=@{BUSES}
> + path="/com/canonical/hud/applications/baz",
> +}
> Index: b/parser/tst/simple_tests/vars/vars_dbus_4.sd
> ===================================================================
> --- /dev/null
> +++ b/parser/tst/simple_tests/vars/vars_dbus_4.sd
> @@ -0,0 +1,13 @@
> +#=DESCRIPTION reference variables in dbus rules, members
> +#=EXRESULT PASS
> +
> +@{MEMBERS}=blurt blirt @{BAR}
> +@{BAR}=@{FOO} blort
> +@{FOO}=bink bank bonk blurry*
> +
> +/does/not/exist {
> + dbus (send)
> + bus=session
> + member=@{MEMBERS}
> + path="/com/canonical/hud/applications/biff",
> +}
> Index: b/parser/tst/simple_tests/vars/vars_dbus_5.sd
> ===================================================================
> --- /dev/null
> +++ b/parser/tst/simple_tests/vars/vars_dbus_5.sd
> @@ -0,0 +1,12 @@
> +#=DESCRIPTION reference variables in dbus rules, with peers
> +#=EXRESULT PASS
> +
> +@{FOO}=bar baz
> +@{BAR}=@{FOO} blort
> +
> +/does/not/exist {
> + dbus (send, receive)
> + bus=session
> + path="/foo/bar" member="bar"
> + peer=(name="com.@{FOO}" label="/usr/bin/app.@{FOO}"),
> +}
> Index: b/parser/tst/simple_tests/vars/vars_dbus_6.sd
> ===================================================================
> --- /dev/null
> +++ b/parser/tst/simple_tests/vars/vars_dbus_6.sd
> @@ -0,0 +1,11 @@
> +#=DESCRIPTION reference variables in dbus rules, with name
> +#=EXRESULT PASS
> +
> +@{FOO}=bar baz
> +@{BAR}=@{FOO} blort
> +
> +/does/not/exist {
> + dbus (bind)
> + bus=session
> + name="com.@{BAR}.@{FOO}",
> +}
> Index: b/parser/tst/simple_tests/vars/vars_dbus_7.sd
> ===================================================================
> --- /dev/null
> +++ b/parser/tst/simple_tests/vars/vars_dbus_7.sd
> @@ -0,0 +1,11 @@
> +#=DESCRIPTION reference variables in dbus rules, with duplicates
> +#=EXRESULT PASS
> +
> +@{FOO}=bar baz bar
> +@{BAR}=@{FOO} blort
> +
> +/does/not/exist {
> + dbus (bind)
> + bus=session
> + name="com.@{BAR}.@{FOO}",
> +}
> Index: b/tests/regression/apparmor/dbus.inc
> ===================================================================
> --- a/tests/regression/apparmor/dbus.inc
> +++ b/tests/regression/apparmor/dbus.inc
> @@ -10,11 +10,22 @@
> gendbusprofile()
> {
> genprofile --stdin <<EOF
> +${__dbus_var_decl}
> $test {
> @{gen $test}
> $@
> }
> EOF
> + unset __dbus_var_decl
> +}
> +
> +# the arguments passed are emitted in the profile's prologue, for
> +# setting profile variables, e.g.
> +# set_dbus_var "@{MY_DBUS_VAR}=stuff"
> +# the saved variable declaration gets unset after each test run
> +set_dbus_var()
> +{
> + __dbus_var_decl=$@
> }
>
> start_bus()
> Index: b/tests/regression/apparmor/dbus_message.sh
> ===================================================================
> --- a/tests/regression/apparmor/dbus_message.sh
> +++ b/tests/regression/apparmor/dbus_message.sh
> @@ -102,6 +102,34 @@ message_gendbusprofile "dbus send bus=se
> runtestfg "message (send allowed w/ bus, dest, path, interface, method)" pass $confined_args
> checktestfg "compare_logs $unconfined_log eq $confined_log"
>
> +# Make sure send is allowed when confined with appropriate permissions along
> +# with conditionals and variables (same tests as above, with vars)
> +
> +set_dbus_var "@{BUSES}=session system"
> +message_gendbusprofile "dbus send bus=@{BUSES},"
> +runtestfg "message (send allowed w/ bus)" pass $confined_args
> +checktestfg "compare_logs $unconfined_log eq $confined_log"
> +
> +set_dbus_var "@{PEERNAMES}=com.ubuntu.what net.apparmor.wiki org.freedesktop.DBus"
> +message_gendbusprofile "dbus send bus=session peer=(name=@{PEERNAMES}),"
> +runtestfg "message (send allowed w/ bus, dest)" pass $confined_args
> +checktestfg "compare_logs $unconfined_log eq $confined_log"
> +
> +set_dbus_var "@{PATHNAMES}=DBus spork spoon spork"
> +message_gendbusprofile "dbus send bus=session path=/org/freedesktop/@{PATHNAMES} peer=(name=org.freedesktop.DBus),"
> +runchecktest "message (send allowed w/ bus, dest, path)" pass $confined_args
> +checktestfg "compare_logs $unconfined_log eq $confined_log"
> +
> +set_dbus_var "@{INTERFACE_NAMES}=DBus spork spoon spork"
> +message_gendbusprofile "dbus send bus=session path=/org/freedesktop/DBus interface=org.freedesktop.@{INTERFACE_NAMES} peer=(name=org.freedesktop.DBus),"
> +runtestfg "message (send allowed w/ bus, dest, path, interface)" pass $confined_args
> +checktestfg "compare_logs $unconfined_log eq $confined_log"
> +
> +set_dbus_var "@{MEMBERS}=Hello ListNames Spork Spoon"
> +message_gendbusprofile "dbus send bus=session path=/org/freedesktop/DBus interface=org.freedesktop.DBus member=@{MEMBERS} peer=(name=org.freedesktop.DBus),"
> +runtestfg "message (send allowed w/ bus, dest, path, interface, method)" pass $confined_args
> +checktestfg "compare_logs $unconfined_log eq $confined_log"
> +
> # Make sure send is denied when confined with appropriate permissions along
> # with incorrect conditionals
>
> Index: b/parser/tst/equality.sh
> ===================================================================
> --- a/parser/tst/equality.sh
> +++ b/parser/tst/equality.sh
> @@ -148,6 +148,35 @@ verify_binary_equality "dbus access pars
> "/t { dbus (send,receive,,,,,,,,,,,,,,,,bind), }" \
> "/t { dbus (send,send,send,send send receive,bind), }" \
>
> +verify_binary_equality "dbus variable expansion" \
> + "/t { dbus (send, receive) path=/com/foo member=spork interface=org.foo peer=(name=com.foo label=/com/foo), }" \
> + "@{FOO}=foo
> + /t { dbus (send, receive) path=/com/@{FOO} member=spork interface=org.@{FOO} peer=(name=com.@{FOO} label=/com/@{FOO}), }" \
> + "@{FOO}=foo
> + @{SPORK}=spork
> + /t { dbus (send, receive) path=/com/@{FOO} member=@{SPORK} interface=org.@{FOO} peer=(name=com.@{FOO} label=/com/@{FOO}), }" \
> + "@{FOO}=/com/foo
> + /t { dbus (send, receive) path=@{FOO} member=spork interface=org.foo peer=(name=com.foo label=@{FOO}), }" \
> + "@{FOO}=com
> + /t { dbus (send, receive) path=/@{FOO}/foo member=spork interface=org.foo peer=(name=@{FOO}.foo label=/@{FOO}/foo), }"
> +
> +verify_binary_equality "dbus variable expansion, multiple values/rules" \
> + "/t { dbus (send, receive) path=/com/foo, dbus (send, receive) path=/com/bar, }" \
> + "@{FOO}=foo
> + /t { dbus (send, receive) path=/com/@{FOO}, dbus (send, receive) path=/com/bar, }" \
> + "@{FOO}=foo bar
> + /t { dbus (send, receive) path=/com/@{FOO}, }" \
> + "@{FOO}=bar foo
> + /t { dbus (send, receive) path=/com/@{FOO}, }"
> +
> +verify_binary_equality "dbus variable expansion, ensure rule de-duping occurs" \
> + "/t { dbus (send, receive) path=/com/foo, dbus (send, receive) path=/com/bar, }" \
> + "/t { dbus (send, receive) path=/com/foo, dbus (send, receive) path=/com/bar, dbus (send, receive) path=/com/bar, }" \
> + "@{FOO}=bar foo bar foo
> + /t { dbus (send, receive) path=/com/@{FOO}, }" \
> + "@{FOO}=bar foo bar foo
> + /t { dbus (send, receive) path=/com/@{FOO}, dbus (send, receive) path=/com/@{FOO}, }"
> +
This looks fine, but it shows the future need for a variant of
verify_binary_equality() that takes a path to a profile instead of a
string containing a profile.
Tyler
> if [ $fails -ne 0 -o $errors -ne 0 ]
> then
> printf "ERRORS: %d\nFAILS: %d\n" $errors $fails 2>&1
>
> --
> Steve Beattie
> <sbeattie at ubuntu.com>
> http://NxNW.org/~steve/
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130829/d7c41420/attachment.pgp>
More information about the AppArmor
mailing list