checkpolicy: memory leak in declare_type()

[Nicolas Iooss <nicolas.iooss@m4x.org>]

Hello,

When building checkpolicy with gcc Address Sanitizer and using the
result to compile refpolicy, some leaks are reported. I wrote a minimal
policy which exposes one of the issues I am experiencing:

$ cat test.mod
module test 1.0.0;
require { role system_r; }
optional {
	require { type sysadm_sudo_t; }
}

optional {
	require { attribute sudodomain; }
	type sysadm_sudo_t, sudodomain;
}

$ checkmodule -m test.mod -o test.mod
checkmodule:  loading policy configuration from test.mod
checkmodule:  policy configuration loaded
checkmodule:  writing binary representation (version 17) to test.mod

=================================================================
==27309==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 40 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c7214ee60 in __interceptor_malloc
/build/gcc-multilib/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:62
    #1 0x40cb8c in declare_type
/usr/src/selinux/checkpolicy/module_compiler.c:327

Direct leak of 14 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c7214ee60 in __interceptor_malloc
/build/gcc-multilib/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:62
    #1 0x413220 in insert_id
/usr/src/selinux/checkpolicy/policy_define.c:120

SUMMARY: AddressSanitizer: 54 byte(s) leaked in 2 allocation(s).


In the code, when checkpolicy parses "type sysadm_sudo_t, sudodomain;"
it calls declare_type() to declare this new type. This function calls
declare_symbol() [1], which returns 1 because the type has been
previously required (require_symbol() added it to the policy hash tables).

However declare_symbol() comment states "For roles and users, it is
legal to have multiple declarations; as such return 1 to indicate that
caller must free() the datum because it was not added". This comment
does not consider the case of a type which is declared after being required.

The reported leak is a consequence of this corner case. In
declare_type(), variables id and typdatum are not freed when
declare_symbol() returns 1 [3]. Moreover typdatum, which is the datum of
the type which has NOT been inserted in the policy hashtables, is
returned to declare_type's caller.

This seems buggy and it seems reasonable to copy what declare_role() [4]
and declare_user() [5] do: create new type object in the current
declaration scope. As I am not quite confident with this approach, is it
something which would make sense?

Cheers,
Nicolas

[1]
https://github.com/SELinuxProject/selinux/blob/checkpolicy-2.6/checkpolicy/module_compiler.c#L337
[2]
https://github.com/SELinuxProject/selinux/blob/checkpolicy-2.6/checkpolicy/module_compiler.c#L121
[3]
https://github.com/SELinuxProject/selinux/blob/checkpolicy-2.6/checkpolicy/module_compiler.c#L338
[4]
https://github.com/SELinuxProject/selinux/blob/checkpolicy-2.6/checkpolicy/module_compiler.c#L238
[5]
https://github.com/SELinuxProject/selinux/blob/checkpolicy-2.6/checkpolicy/module_compiler.c#L443