[apparmor] Memory leaks in parser/parser_regex.c (at least)

Mon Aug 24 15:43:46 UTC 2015

On Sun, Aug 23, 2015 at 02:57:21PM +0200, intrigeri wrote:
> Steve Beattie wrote (22 Aug 2015 21:06:21 GMT) :
> > In fact, the act of cleaning these up and running valgrind on the unit
> > tests picked up another issue, a use-before-initialization bug in the
> > regex conversion code -- see the second attached patch. I'm curious if
> > ASAN would notice it.
> 
> With both your first patch applied, and with both applied, I now see
> this failure (I don't if that's before the 2nd bug you've fixed, or
> after — in case ASAN didn't catch it):

It's after.

> *** running tst_variable
> *** running tst_lib
> make[3]: Entering directory '/tmp/buildd/apparmor-2.10/parser/tst'
> Cache read/write disabled: interface file missing. (Kernel needs AppArmor 2.4 compatibility patch.)
> =================================================================
> ==16428==ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new [] vs operator delete) on 0x611000005b80
>     #0 0x7f9ed7ce162a in operator delete(void*) (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x9562a)
>     #1 0x5abbd7 in hashedNodeVec::~hashedNodeVec() /tmp/buildd/apparmor-2.10/parser/libapparmor_re/expr-tree.h:675
>     #2 0x5abbd7 in NodeVecCache::insert(std::set<ImportantNode*, std::less<ImportantNode*>, std::allocator<ImportantNode*> >*) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/expr-tree.h:777
>     #3 0x5abbd7 in DFA::add_new_state(std::set<ImportantNode*, std::less<ImportantNode*>, std::allocator<ImportantNode*> >*, std::set<ImportantNode*, std::less<ImportantNode*>, std::allocator<ImportantNode*> >*, State*) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/hfa.cc:284
>     #4 0x5af832 in DFA::add_new_state(std::set<ImportantNode*, std::less<ImportantNode*>, std::allocator<ImportantNode*> >*, State*) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/hfa.cc:309
>     #5 0x5d244b in DFA::update_state_transitions(State*) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/hfa.cc:346
>     #6 0x5d4a07 in DFA::process_work_queue(char const*, int) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/hfa.cc:390
>     #7 0x5db3d2 in DFA::DFA(Node*, int) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/hfa.cc:430
>     #8 0x52a615 in aare_rules::create_dfa(unsigned long*, int) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/aare_rules.cc:184
>     #9 0x4b364f in process_profile_regex(Profile*) /tmp/buildd/apparmor-2.10/parser/parser_regex.c:636
>     #10 0x4c3914 in process_profile_rules(Profile*) /tmp/buildd/apparmor-2.10/parser/parser_policy.c:336
>     #11 0x4c4677 in post_process_profile(Profile*, int) /tmp/buildd/apparmor-2.10/parser/parser_policy.c:387
>     #12 0x4c3e0d in post_process_policy_list(ProfileList&, int) /tmp/buildd/apparmor-2.10/parser/parser_policy.c:400
>     #13 0x48d097 in process_profile(int, aa_kernel_interface*, char const*, char const*) /tmp/buildd/apparmor-2.10/parser/parser_main.c:771
>     #14 0x46008f in main /tmp/buildd/apparmor-2.10/parser/parser_main.c:978
>     #15 0x7f9ed6697b44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b44)
>     #16 0x462494  (/tmp/buildd/apparmor-2.10/parser/apparmor_parser+0x462494)
> 
> 0x611000005b80 is located 0 bytes inside of 200-byte region [0x611000005b80,0x611000005c48)
> allocated by thread T0 here:
>     #0 0x7f9ed7ce123a in operator new[](unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x9523a)
>     #1 0x5ab8db in hashedNodeVec::hashedNodeVec(std::set<ImportantNode*, std::less<ImportantNode*>, std::allocator<ImportantNode*> >*) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/expr-tree.h:655
>     #2 0x5ab8db in NodeVecCache::insert(std::set<ImportantNode*, std::less<ImportantNode*>, std::allocator<ImportantNode*> >*) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/expr-tree.h:774
>     #3 0x5ab8db in DFA::add_new_state(std::set<ImportantNode*, std::less<ImportantNode*>, std::allocator<ImportantNode*> >*, std::set<ImportantNode*, std::less<ImportantNode*>, std::allocator<ImportantNode*> >*, State*) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/hfa.cc:284
>     #4 0x5af832 in DFA::add_new_state(std::set<ImportantNode*, std::less<ImportantNode*>, std::allocator<ImportantNode*> >*, State*) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/hfa.cc:309
>     #5 0x5d244b in DFA::update_state_transitions(State*) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/hfa.cc:346
>     #6 0x5d4a07 in DFA::process_work_queue(char const*, int) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/hfa.cc:390
>     #7 0x5db3d2 in DFA::DFA(Node*, int) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/hfa.cc:430
>     #8 0x52a615 in aare_rules::create_dfa(unsigned long*, int) /tmp/buildd/apparmor-2.10/parser/libapparmor_re/aare_rules.cc:184
>     #9 0x4b364f in process_profile_regex(Profile*) /tmp/buildd/apparmor-2.10/parser/parser_regex.c:636
>     #10 0x4c3914 in process_profile_rules(Profile*) /tmp/buildd/apparmor-2.10/parser/parser_policy.c:336
>     #11 0x4c4677 in post_process_profile(Profile*, int) /tmp/buildd/apparmor-2.10/parser/parser_policy.c:387
>     #12 0x4c3e0d in post_process_policy_list(ProfileList&, int) /tmp/buildd/apparmor-2.10/parser/parser_policy.c:400
>     #13 0x48d097 in process_profile(int, aa_kernel_interface*, char const*, char const*) /tmp/buildd/apparmor-2.10/parser/parser_main.c:771
>     #14 0x46008f in main /tmp/buildd/apparmor-2.10/parser/parser_main.c:978
>     #15 0x7f9ed6697b44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b44)
> 
> SUMMARY: AddressSanitizer: alloc-dealloc-mismatch ??:0 operator delete(void*)
> ==16428==HINT: if you don't care about these warnings you may set ASAN_OPTIONS=alloc_dealloc_mismatch=0
> ==16428==ABORTING
> Makefile:33: recipe for target 'error_output' failed
> make[3]: *** [error_output] Error 1
> make[3]: Leaving directory '/tmp/buildd/apparmor-2.10/parser/tst'
> Makefile:298: recipe for target 'tests' failed

Looks like a legitimate type mismatch error when the objects are
deleted.

> > However, once you get past the unit tests, you will still hit a
> > couple of memory leaks running the regular parser sanity checks,
> > in the backend conversion bits[0].
> 
> Not sure if I've passed the unit tests yet.

Not yet, no.

> > IIRC, I tracked what I could
> > down to error conditions occurring while walking the various trees
> > that caused things to jump back out and then attempt to cleanup;
> > unfortunately, I think things get left in an incosistent state that
> > causes the cleanup walkthrough to miss a few objects.
> 
> OK, thanks!
> 
> I would be totally awesome if the whole thing could build with ASAN.
> I have two main incentives:
> 
> 1. I guess that so far, minor memory leaks were not so bad, because oh
> well, the apparmor_parser process is not long-lived. But now that
> systemd is linking against libapparmor, and will soon be using its
> functionality, perhaps they will need to be handled with higher
> priority. (I'm no expert in this field, and my assesment may be
> totally wrong, so of course I'll let you judge :)

Actually, we try to take memory leaks seriously, as policy compilation
can use a lot of memory depending on the policy, and memory leaks make
memory pressure even worse. And yes, moving more functionality into
libapparmor means we need to be ever more vigilant about them.

Where we didn't particularly prioritize fixing them is in unit tests,
where the leak isn't actual code that's incorporated in the parser.
But if it helps with building ASAN versions, then I'm happy to fix those
as well.

> 2. apparmor-notify, aa-genprof and aa-logprof (or rather, the
> libraries behind them) sometimes handle untrusted input.

Yep.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- 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/20150824/3102ce99/attachment.pgp>