[apparmor] [PATCH] Add compressed dfa matching routines to library, and a base test program

[Christian Boltz]

Hello,

Am Montag, 11. Januar 2016 schrieb John Johansen:
> On 01/10/2016 07:22 AM, Christian Boltz wrote:
> > Am Freitag, 8. Januar 2016 schrieb John Johansen:
> >> diff --git a/devtools/Makefile b/devtools/Makefile
> >> new file mode 100644
> >> index 0000000..b0cd26e
> >> --- /dev/null
> >> +++ b/devtools/Makefile

> >> +print_hfa: print_hfa.o
> >> +	$(CC) ${CFLAGS} -o $@ $^  -L 
../libraries/libapparmor/src/.libs/
> >> -static -lapparmor
> > 
> > Should this -L depend on USE_SYSTEM?
> > (I'm not sure which files are needed, so maybe I'm wrong ;-)
> 
> well, not yet. Eventually we will get there but this being the first
> pass I actually wanted to restrict it to the in tree builds atm

I'd still use a variable and set it to "-L ../libraries/..." in both 
branches of the "if USE_SYSTEM" to make it obvious that we build in-tree 
only.

> >> diff --git a/devtools/expr.txt b/devtools/expr.txt
> >> new file mode 100644
> >> index 0000000..4b7d12b
> >> --- /dev/null
> >> +++ b/devtools/expr.txt
> >> @@ -0,0 +1,772 @@
> >> +1 /
> >> +1 /([^\0000])*\.[Bb][Mm][Pp]
> >> +1 /([^\0000])*\.[Bb][Zz]2
> >> +1 /([^\0000])*\.[Cc][Bb][7RZrz]
> >> +1 /([^\0000])*\.[Dd][Jj][Vv][Uu]
> >> +1 /([^\0000])*\.[Dd][Vv][Ii]
> > 
> > What's the reason for using that syntax (looks like "real" regexes
> > or
> > PCRE) instead of AARE?
> 
> because it is a PCRE syntax which is more powerful than what AARE can
> express and what is actually used internally on the backend.
> 
> > I'd prefer to feed the devtools with AARE, since this is what we use
> > in the profiles. A requirement "variables must be expanded" would
> > be ok.
> Nope, and nope.
> 
> This is targeting testing the backend dev work. It allows more and
> will also start allowing "kernel" vars etc which won't be expanded.

Indeed, kernel vars are a good argument to not expect _all_ variables 
expanded ;-)

It might still be ok to expect non-kernel vars expanded. (At least for 
now - a separate parameter containing variable values might be even 
better, but one step after the other ;-)

> Eventually we may open up some of the PCRE syntax to the front end
> profiles, certainly if not the syntax some of the capabilities.
> 
> We do have some unit tests for converting between the two, but more
> tests are always good

There are some tests (including expected matches) in the python tests 
(test-aare.py TestConvert_regexpAndAAREMatch) that test the python AARE 
implementation.
	
If you provide a way to convert them (or even to run everything from 
python), we can run the same set of tests against the libapparmor/future 
kernel AARE checking.

Yes, I know that won't happen in the next days. Just keep it in mind as 
something to do in the future.

> >> diff --git a/devtools/print_hfa.c b/devtools/print_hfa.c
> >> new file mode 100644
> >> index 0000000..c5a5b65
> >> --- /dev/null
> >> +++ b/devtools/print_hfa.c
> >> 
> >> +const char *short_options = "";
> >> +struct option long_options[] = {
> >> +	{NULL,		0, 0, 0},
> >> +};
> >> +
> >> +static int process_args(int argc, char *argv[])

> > Also, how can the error condition (case 0) happen? From reading
> > getopt_long(3), I'd guess it happens if a long option is matched -
> > but it's very unlikely that NULLL will ever match ;-)
> > (Also, why do we need that NULL in long_options at all?)
> 
> the null is a terminator, without it you can get strange option
> processing based on random junk in memory, and yes we had a bug
> related to this once

Sounds interesting[tm] ;-)  (It also sounds like a bug in getopt_long, 
but if the workaround is that easy, we should of course keep it as 
safety net. Nevertheless, I hope this is or gets fixed in getopt_long.)

> > Reading a file into a buffer doesn't look that easy in C ;-)
> > 
> > Is there really no function available somewhere that shortens all
> > this to something like
> > 
> >     buffer = read_file(filename)
> > 
> > + checking errno?
> 
> meh, there are alternatives, like using fopen and fread, mmap, ...
> 
> I'm not sure why I through in a more low level approach here. It can
> be changed

That would be a good idea - I already found two double-close(), and 
someone who really understands C might find even more interesting things.


> >> +char *load_file(const char *path)
> > 
> > That looks like the same load_file() as in print_hfa.c, therefore
> > - please avoid copy&paste programming and move it to a separate file
> > 
> >   that can be used by both binaries
> 
> no, this really does not warrant sharing, especially not at this time

copy&paste programming is never a good idea IMHO, but well, _I_ will not 
suffer the pain from fixing it ;-)

Hint: you already have to copy over the double-close() fix...


> >> diff --git a/libraries/libapparmor/src/hfa.c
> >> b/libraries/libapparmor/src/hfa.c new file mode 100644
> >> index 0000000..ec31a15
> >> --- /dev/null
> >> +++ b/libraries/libapparmor/src/hfa.c

> > I don't say that I completely understand this, but a short and
> > simplified summary seems to be that this code does matching like
> > the kernel does when deciding if an action is allowed/denied.
> > Right?
> 
> yes
> 
> > Does this also mean this code is "stolen" from the kernel code?
> 
> Largely it is a fresh rewrite, and the kernel will be picking up this
> code once its ready. There are a few bits and pieces from the kernel
> code, largely around the defines in the .h files

Ah, good to know.

Will this enter the kernel before or after ptrace, signal etc. handling? 
*SCNR*

> >> diff --git a/libraries/libapparmor/swig/SWIG/libapparmor.i
> >> b/libraries/libapparmor/swig/SWIG/libapparmor.i index
> >> 69b4cc2..47213a1 100644
> >> --- a/libraries/libapparmor/swig/SWIG/libapparmor.i
> >> +++ b/libraries/libapparmor/swig/SWIG/libapparmor.i

> > It's nice to see more library functions exported towards the swig
> > bindings.
> 
> ah crud I missed ripping those out :)
> 
> we aren't ready to be exporting these functions yet.
> 
> > Now to the practical question - are some of those functions useful
> > to, let's say, handle the AARE matching in the tools?
> 
> Not yet, but that is the goal

:-)


Also, thanks for explaining the less obvious parts of the code ;-)


Regards,

Christian Boltz
-- 
Was habt Ihr denn?  emacs ist doch ein tolles Betriebssystem!
Das einzige was ihm fehlt, ist ein vernünftiger Editor (vim?)
[Jan Trippler in suse-linux]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160111/d5ed525b/attachment.pgp>