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

Sun Aug 23 12:57:21 UTC 2015

Hi,

Steve Beattie wrote (22 Aug 2015 21:06:21 GMT) :
> -ENO_DEBDIFF,

Oops, sorry. debdiff(1) is part of the devscripts package (that has
*tons* of other useful tools by the way). It's often used in Debian to
represent changes between two source packages, e.g. when requesting
freeze exceptions to the release team; to do that, one passes two .dsc
files to debdiff.

>> I see test suite failures such as:
>> 
>> ==22749==ERROR: LeakSanitizer: detected memory leaks [...]

> These are memory leaks in the unit tests, which we've not fretted over
> too much in the past (i.e. they're known; I don't recall which tools
> have flagged them before). However, it's not a terrible idea to clean
> them up; try the attached parser-fix_memory_leaks_in_unit_tests.patch
> patch.

This fixes the issue I've reported initially, thanks!

> 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):

*** 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

> 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.

> 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 :)

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

Cheers,
-- 
intrigeri