On 2015-03-12 04:17:56, John Johansen wrote:
> On 03/06/2015 01:48 PM, Tyler Hicks wrote:
> > Defines a function that can be called to test features support. It is
> > string based which allows the support tests to work with new kernel
> > features without any changes.
> >
> > The use of global variables in the parser to store and check features
> > support is still preserved. The parser should probably move over to
> > passing the aa_features object around but that's left for later.
> >
> > Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> > ---
> > parser/features.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > parser/features.h | 1 +
> > parser/parser_main.c | 52 ++++++++++-----------
> > 3 files changed, 155 insertions(+), 26 deletions(-)
> >
> > diff --git a/parser/features.c b/parser/features.c
> > index 07d6e6c..50e0024 100644
> > --- a/parser/features.c
> > +++ b/parser/features.c
> > @@ -17,6 +17,7 @@
> > */
> >
> > #include <errno.h>
> > +#include <ctype.h>
> > #include <fcntl.h>
> > #include <stdio.h>
> > #include <string.h>
> > @@ -163,6 +164,86 @@ static int load_features_file(const char *name, char *buffer, size_t size)
> > return 0;
> > }
> >
> > +static bool walk_one(const char **str, const char *component, bool is_top_level)
> > +{
> > + const char *cur = *str;
> > + uint32_t bracket_count = 0;
> > + int i = 0;
> > +
> > + /* Empty strings are not accepted */
> > + if (!*cur || !component[0])
> > + return false;
> > +
> > + /**
> > + * If @component is not top-level, the first character in the string to
> > + * search MUST be '{'
> > + */
> > + if (!is_top_level) {
> > + if (*cur != '{')
> > + return false;
> > +
> > + cur++;
> > + }
> > +
> > + /**
> > + * This loop tries to find the @component in *@str. When this loops
> > + * completes, cur will either point one character past the end of the
> > + * matched @component or to the NUL terminator of *@str.
> > + */
> > + while(*cur && component[i]) {
> > + if (!isascii(*cur)) {
> > + /* Only ASCII is expected */
> > + return false;
> > + } else if (*cur == '{') {
> > + /* There's a limit to the number of opening brackets */
> > + if (bracket_count == UINT32_MAX)
> > + return false;
> > +
> > + bracket_count++;
> > + } else if (*cur == '}') {
> > + /* Check for unexpected closing brackets */
> > + if (bracket_count == 0)
> > + return false;
> > +
> > + bracket_count--;
> > + }
> > +
> > + /**
> > + * Move to the next character in @component if we have a match
> > + * and either @component is not top-level or, if @component is
> > + * top-level, we're not inside of brackets
> > + */
> > + if (*cur == component[i] &&
> > + (!is_top_level || bracket_count == 0))
> > + i++;
> > + else
> > + i = 0;
> > +
> This isn't correct. IF we are !is_top_level then we can have a partial leading
> match, hit a bracket, increase bracket_count and continue matching, so that
> the component
> frontback
> will match either of
> front{back
> front}back
Hmm... I don't think that's possible since there's no 'continue' when we
hit a bracket. The (*cur == component[i]) test will fail and i will be
reset back to 0.
Either way, this is way too complicated of a function for me to be
sending out without tests. I apologize for that. I'll add tests,
including the case you mentioned above, and then fix issues and resend
this patch out.
> > + cur++;
> > + }
> > +
> > + /* A full match was not found if component[i] is non-NUL */
> > + if (component[i])
> > + return false;
> > +
> > + /**
> > + * This loop eats up valid (ASCII) characters until a non-bracket or
> > + * non-space character is found so that *@str is properly set to call
> > + * back into this function, if necessary
> > + */
> > + while (*cur) {
> > + if (!isascii(*cur))
> > + return false;
> > + else if (*cur == '{' || *cur == '}' || !isspace(*cur))
> > + break;
> > +
> > + cur++;
> > + }
> > +
> > + *str = cur;
> > + return true;
> > +}
> > +
> > /**
> > * aa_features_new - create a new features based on a path
> > * @features: will point to the address of an allocated and initialized
> > @@ -299,3 +380,50 @@ bool aa_features_is_equal(aa_features *features1, aa_features *features2)
> > return features1 && features2 &&
> > strcmp(features1->string, features2->string) == 0;
> > }
> > +
> > +/**
> > + * aa_features_supports - provides features support status
> > + * @features: the features
> > + * @str: the string representation of a feature to check
> > + *
> > + * Example @str values are "dbus/mask/send", "caps/mask/audit_read", and
> > + * "policy/versions/v7".
> > + *
> > + * Returns: a bool specifying the support status of @str feature
> > + */
> > +bool aa_features_supports(aa_features *features, char *str)
> > +{
> > + const char *features_string = features->string;
> > + char *components[32];
> > + char *saveptr = NULL;
> > + size_t i;
> > +
> > + /* Empty strings are not accepted. Neither are leading '/' chars. */
> > + if (!str || str[0] == '/')
> > + return false;
> > +
> > + /**
> > + * Break @str into an array of components. For example,
> > + * "mount/mask/mount" would turn into "mount" as the first component,
> > + * "mask" as the second, and "mount" as the third
> > + */
> > + for (i = 0; i < sizeof(components); i++) {
> > + components[i] = strtok_r(str, "/", &saveptr);
> > + if (!components[i])
> > + break;
> > +
> > + str = NULL;
> > + }
> I am not a fan of the use of strtok in this fn. It means const strings can
> not be passed, nor could the same query string be used multiple times (not
> that you generally should but ...).
> I would rather see an api that can use constant strings, but at the very
> least @str needs to document that it will be modified.
Yeah, I completely agree. I'll see if I can come up with something that
works off of string lengths instead of NUL terminators.
Tyler
> > +
> > + /* At least one valid token is required */
> > + if (!components[0])
> > + return false;
> > +
> > + /* Ensure that all components are valid and found */
> > + for (i = 0; i < sizeof(components) && components[i]; i++) {
> > + if (!walk_one(&features_string, components[i], i == 0))
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > diff --git a/parser/features.h b/parser/features.h
> > index a0c177f..96d0ee9 100644
> > --- a/parser/features.h
> > +++ b/parser/features.h
> > @@ -29,5 +29,6 @@ aa_features *aa_features_ref(aa_features *features);
> > void aa_features_unref(aa_features *features);
> > const char *aa_features_get_string(aa_features *features);
> > bool aa_features_is_equal(aa_features *features1, aa_features *features2);
> > +bool aa_features_supports(aa_features *features, char *str);
> >
> > #endif /* __AA_FEATURES_H */
> > diff --git a/parser/parser_main.c b/parser/parser_main.c
> > index 3f8ca6a..2d220a7 100644
> > --- a/parser/parser_main.c
> > +++ b/parser/parser_main.c
> > @@ -574,7 +574,17 @@ no_match:
> >
> > static void set_supported_features(void)
> > {
> > - const char *features_string;
> > + char file[] = "file";
> > + char network[] = "network";
> > + char af_unix[] = "network/af_unix";
> > + char mount[] = "mount";
> > + char dbus[] = "dbus";
> > + char signal[] = "signal";
> > + char ptrace[] = "ptrace";
> > + char set_load[] = "policy/set_load";
> > + char diff_encode[] = "policy/diff_encode";
> > + char v7[] = "policy/versions/v7";
> > + char v6[] = "policy/versions/v6";
> >
> > /* has process_args() already assigned a match string? */
> > if (!features && aa_features_new_from_kernel(&features) == -1) {
> > @@ -582,33 +592,23 @@ static void set_supported_features(void)
> > return;
> > }
> >
> > - features_string = aa_features_get_string(features);
> > perms_create = 1;
> > -
> > - /* TODO: make this real parsing and config setting */
> > - if (strstr(features_string, "file {")) /* pre policydb is file= */
> > - kernel_supports_policydb = 1;
> > - if (strstr(features_string, "v6"))
> > - kernel_abi_version = 6;
> > - if (strstr(features_string, "v7"))
> > + kernel_supports_policydb = aa_features_supports(features, file);
> > + kernel_supports_network = aa_features_supports(features, network);
> > + kernel_supports_unix = aa_features_supports(features, af_unix);
> > + kernel_supports_mount = aa_features_supports(features, mount);
> > + kernel_supports_dbus = aa_features_supports(features, dbus);
> > + kernel_supports_signal = aa_features_supports(features, signal);
> > + kernel_supports_ptrace = aa_features_supports(features, ptrace);
> > + kernel_supports_setload = aa_features_supports(features, set_load);
> > + kernel_supports_diff_encode = aa_features_supports(features, diff_encode);
> > +
> > + if (aa_features_supports(features, v7))
> > kernel_abi_version = 7;
> > - if (strstr(features_string, "set_load"))
> > - kernel_supports_setload = 1;
> > - if (strstr(features_string, "network"))
> > - kernel_supports_network = 1;
> > - if (strstr(features_string, "af_unix"))
> > - kernel_supports_unix = 1;
> > - if (strstr(features_string, "mount"))
> > - kernel_supports_mount = 1;
> > - if (strstr(features_string, "dbus"))
> > - kernel_supports_dbus = 1;
> > - if (strstr(features_string, "signal"))
> > - kernel_supports_signal = 1;
> > - if (strstr(features_string, "ptrace {"))
> > - kernel_supports_ptrace = 1;
> > - if (strstr(features_string, "diff_encode"))
> > - kernel_supports_diff_encode = 1;
> > - else if (dfaflags & DFA_CONTROL_DIFF_ENCODE)
> > + else if (aa_features_supports(features, v6))
> > + kernel_abi_version = 6;
> > +
> > + if (!kernel_supports_diff_encode)
> > /* clear diff_encode because it is not supported */
> > dfaflags &= ~DFA_CONTROL_DIFF_ENCODE;
> > }
> >
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150312/5f4de7a2/attachment.pgp>