[apparmor] [patch] [28/38] AARE: let match() handle plain path regexes as non-regex

[Christian Boltz]

Hello,

Am Montag, 26. September 2016, 14:45:34 CEST schrieb Steve Beattie:
> On Fri, Aug 12, 2016 at 11:03:09PM +0200, Christian Boltz wrote:
> > when matching an AARE against another AARE, most AARE objects don't
> > contain orig_regex (only AARE instances originating from a log event
> > contain orig_regex).
> > 
> > In this case, match() will use is_equal() to error out on the safe
> > side. Unfortunately this also means that there are lots of false
> > negative cases where match() returns False errornously.
> > 
> > With this patch, match() checks the given AARE regex and, if it
> > doesn't contain any special characters (wildcards, alternations or
> > variables), handles it as plain path. This avoids most of the false
> > negatives.
> > 
> > Also extend the AARE tests to check a bunch of plain path regexes
> > using AARE matching instead of only str matching.
> > 
> > [ 28-aare-plain-path.diff ]
> 
> Acked-by: Steve Beattie <steve at nxnw.org>, though I'm not crazy about
> commingling the plain checks with the regex checks in the same
> function, as I suspect it will make figuring out what's failing when
> something goes wrong more difficult (in answering "What's being
> tested and why?").

Please allow me to disagree ;-)

In most cases (except when reading from audit.log, where the input is 
guaranteed not to be a regex), we check one regex against another regex. 
Even if a profile contains a plain path like "/bin/foo", it's technically 
a regex because profiles by definition contain "path regexes", not "plain 
paths". ["regex" means AARE syntax here, as described in the apparmor.d 
manpage. Don't confuse this with the AARE class.]

Unfortunately checking one regex against another is terribly difficult [1] 
so this patch avoids this nightmare in cases we can easily avoid it.

By checking if a given regex is "boring" (as in: only contains 
characters that do not have a special meaning, so plain path == regex), 
we can cover lots of cases without too much complexity.

Yes, this is an "error out on the safe side" attemp [2] - but I still 
consider this better than relying on code I don't really understand ;-)

If someone can explain all details mentioned in [1] and/or provides a 
patch to improve the situation, this is more than welcome!


TL;DR: AARE still does only "plain" checks when matching against another 
AARE - it just became more tolerant on what to accept as "plain" ;-)


Regards,

Christian Boltz

PS: non-random sig

[1] I'd _guess_ matchliteral() in aa.py does something like that, but I 
    have to admit that I don't fully understand this function and 
    convert_regexp() in common.py which it uses, and I'm especially not 
    sure if it does what I think it does. (Needless to say: no tests.)

    Yes, there are some tests for convert_regexp(), but if you look at 
    the resulting regex, you might understand why I don't really 
    understand it ;-) (at least for the more complex cases)

    TL;DR: Welcome to regex hell...

    On the positive side, I have _the answer[tm]_ - and, what an irony, 
    this answer will be patch 42 ;-)
    After looking at the code again, I found out that matchliteral() is 
    now unused :-) (see patch 42 for details, it was an interesting 
    hunt) so the best thing I can do is to drop it.

[2] worst thing that can happen: a profile ends up with an additional, 
    superfluous rule because the tools didn't recognize that it's covered 
    by another rule

-- 
LGTM? There is a special place in hell for regular expressions.                                                                                                                                                                                                                
[ewindisch on https://github.com/docker/docker/pull/19069]                                                                                                                                                                                                                     
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160929/d3c9cf8a/attachment.pgp>