Re: Another libsepol USE_AFTER_FREE defects detected

[Petr Lautrbach <plautrba@redhat.com>]

James Carter <jwcart2@gmail.com> writes:

> On Thu, Sep 9, 2021 at 3:10 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>>
>> Hello,
>>
>> our internal scanner reports the following defects:
>>
>> Error: USE_AFTER_FREE (CWE-416): [#def1]
>> libsepol/cil/src/cil_build_ast.c:473: freed_arg: "cil_gen_node" frees "class".
>> libsepol/cil/src/cil_build_ast.c:480: deref_after_free: Dereferencing freed pointer "class".
>> #  478|         if (parse_current->next->next != NULL) {
>> #  479|                 perms = parse_current->next->next->cl_head;
>> #  480|->               rc = cil_gen_perm_nodes(db, perms, ast_node, CIL_PERM, &class->num_perms);
>> #  481|                 if (rc != SEPOL_OK) {
>> #  482|                         goto exit;
>>
>> Error: USE_AFTER_FREE (CWE-416): [#def2]
>> libsepol/cil/src/cil_build_ast.c:942: freed_arg: "cil_gen_node" frees "map".
>> libsepol/cil/src/cil_build_ast.c:947: deref_after_free: Dereferencing freed pointer "map".
>> #  945|         }
>> #  946|
>> #  947|->       rc = cil_gen_perm_nodes(db, parse_current->next->next->cl_head, ast_node, CIL_MAP_PERM, &map->num_perms);
>> #  948|         if (rc != SEPOL_OK) {
>> #  949|                 goto exit;
>>
>> Error: USE_AFTER_FREE (CWE-416): [#def3]
>> libsepol/cil/src/cil_build_ast.c:1042: freed_arg: "cil_gen_node" frees "common".
>> libsepol/cil/src/cil_build_ast.c:1047: deref_after_free: Dereferencing freed pointer "common".
>> # 1045|         }
>> # 1046|
>> # 1047|->       rc = cil_gen_perm_nodes(db, parse_current->next->next->cl_head, ast_node, CIL_PERM, &common->num_perms);
>> # 1048|         if (rc != SEPOL_OK) {
>> # 1049|                 goto exit;
>>
>>
>> They might be related to commit 0d4e568afe5a28edc5fcdcff8e925d4ec1d0d3d0
>> ("libsepol/cil: Create function cil_add_decl_to_symtab() and refactor")
>>
>> Please take a look.
>>
>
> These are false positives. The only time the datum passed to
> cil_gen_node() gets freed is when the declaration is a duplicate and
> duplicate declarations are allowed and the datum is a type, type
> attribute, or an optional.
>
> It would probably be better, however, to pass the SEPOL_EEXIST back to
> the calling function and to free the datum there.
>
> Jim
>

Thanks!

Petr