[SSSD] [PATCH] AD: LDAP component of GPO-based access control

Lukas Slebodnik lslebodn at redhat.com
Wed May 7 14:46:14 UTC 2014


On (07/05/14 08:47), Yassir Elley wrote:
>----- Original Message -----
>> On (06/05/14 12:42), Yassir Elley wrote:
>> >----- Original Message -----
>> >> On Tue, Apr 29, 2014 at 07:22:37PM -0400, Yassir Elley wrote:
>> >> > ----- Original Message -----
>> >> > > ----- Original Message -----
>> >> > > > On Fri, Apr 18, 2014 at 12:56:27PM -0400, Yassir Elley wrote:
>> >> > 
>> >> > Hi Jakub,
>> >> > 
>> >> > I have attached a revised cumulative patch that aims to address the
>> >> > review
>> >> > comments.
>> >> > 
>> >> > Additionally, I made the following changes en masse:
>> >> > * changed talloc_strdup() to talloc_steal() when applicable (i.e. when
>> >> > dealing with a talloc context)
>> >> > * changed all occurrences of strerror() to sss_strerror() in order to
>> >> > use
>> >> > sssd-specific error strings
>> >> > 
>> >> > Thanks,
>> >> > Yassir.
>> >> 
>> >> Hi,
>> >> 
>> >> I have some more comments, sorry I haven't caught them earlier. I ran the
>> >> patch through Coverity and it uncovered several warnings, see below:
>> >> 
>> >> Error: COMPILER_WARNING:
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c: scope_hint: In function
>> >> 'ad_gpo_target_dn_retrieval_done'
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:711:22: warning: assignment
>> >> discards
>> >> 'const' qualifier from pointer target type [enabled by default]
>> >> #     state->target_dn = talloc_steal(state, target_dn);
>> >> #                      ^
>> >> 
>> >> Error: COMPILER_WARNING:
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c: scope_hint: In function
>> >> 'ad_gpo_parent_dn'
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:902:17: warning: assignment
>> >> discards
>> >> 'const' qualifier from pointer target type [enabled by default]
>> >> #     *_parent_dn = talloc_steal(mem_ctx, p);
>> >> #                 ^
>> >> 
>> >> Error: DEADCODE (CWE-561):
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1053: cond_notnull: Condition "ptr
>> >> =
>> >> __coverity_strchr(ptr, 93)", taking true branch. Now the value of "ptr" is
>> >> not NULL.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1054: notnull: At condition "ptr ==
>> >> NULL", the value of "ptr" cannot be NULL.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1054: dead_error_condition: The
>> >> condition "ptr == NULL" cannot be true.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1054: dead_error_line: Execution
>> >> cannot reach this statement "break;".
>> >> 
>> >> Error: DEADCODE (CWE-561):
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1768: cond_notnull: Condition "ptr
>> >> =
>> >> __coverity_strchr(ptr, 92)", taking true branch. Now the value of "ptr" is
>> >> not NULL.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1769: notnull: At condition "ptr ==
>> >> NULL", the value of "ptr" cannot be NULL.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1769: dead_error_condition: The
>> >> condition "ptr == NULL" cannot be true.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1769: dead_error_line: Execution
>> >> cannot reach this statement "break;".
>> >> 
>> >> Error: DEADCODE (CWE-561):
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1812: cond_notnull: Condition
>> >> "raw_machine_ext_names_value == NULL", taking false branch. Now the value
>> >> of
>> >> "raw_machine_ext_names_value" is not NULL.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1824: assignment: Assigning: "ptr"
>> >> =
>> >> "raw_machine_ext_names_value".
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1825: notnull: At condition "ptr ==
>> >> NULL", the value of "ptr" cannot be NULL.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1825: dead_error_condition: The
>> >> condition "ptr == NULL" cannot be true.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1826: dead_error_begin: Execution
>> >> cannot reach this statement "ret = 12;".
>> >> 
>> >> Error: DEADCODE (CWE-561):
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1830: cond_notnull: Condition "ptr
>> >> =
>> >> __coverity_strchr(ptr, 93)", taking true branch. Now the value of "ptr" is
>> >> not NULL.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1831: notnull: At condition "ptr ==
>> >> NULL", the value of "ptr" cannot be NULL.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1831: dead_error_condition: The
>> >> condition "ptr == NULL" cannot be true.
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:1831: dead_error_line: Execution
>> >> cannot reach this statement "break;".
>> >> 
>> >> Error: COMPILER_WARNING:
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c: scope_hint: In function
>> >> 'ad_gpo_get_gpo_attrs_done'
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:2091:22: warning: assignment
>> >> discards
>> >> 'const' qualifier from pointer target type [enabled by default]
>> >> #     gp_gpo->gpo_guid = talloc_steal(gp_gpo, gpo_guid);
>> >> #                      ^
>> >> 
>> >> Error: COMPILER_WARNING:
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:2110:30: warning: assignment
>> >> discards
>> >> 'const' qualifier from pointer target type [enabled by default]
>> >> #     gp_gpo->gpo_display_name = talloc_steal(gp_gpo, gpo_display_name);
>> >> #                              ^
>> >> 
>> >> Error: COMPILER_WARNING:
>> >> sssd-1.11.90/src/providers/ad/ad_gpo.c:2131:19: warning: assignment
>> >> discards
>> >> 'const' qualifier from pointer target type [enabled by default]
>> >> #     file_sys_path = talloc_steal(gp_gpo, raw_file_sys_path);
>> >> #
>> >
>> > I have fixed the DEADCODE conditions detected by coverity and attached
>> > a revised cumulative patch. With regard to the COMPILER_WARNING, Lukas had
>> > already commented on these previously, and I had already fixed them
>> > in the most recent patch.
>> >
>> Yes warnings were fixed in previous version
>> and I could not see any problems with DEADCODE.
>> 
>> >>
>> >> Also, when I set ad_gpo_access_control=permissive and tried to log in as
>> >> Administrator, I was kicked out. I thought that permissive
>> >> setting would ignore the errors, but maybe I stumbled upon something
>> >> unexpected?
>> >
>> > This is expected behavior because I have not implemented "permissive" yet.
>> > For now, "permissive" and "enforcing" are identical. I was not planning
>> > on implementing "permissive" until after the code is able to retrieve
>> > policy
>> > over smb and make an enforcement decision. I did implement "disabled",
>> > since
>> > it allows users to simply turn off gpo checking altogether.
>> >
>> >The current code in ad_gpo_access.c simply does the following:
>> >
>> >    switch (state->ctx->gpo_access_control_mode) {
>> >    case GPO_ACCESS_CONTROL_DISABLED:
>> >        /* do not evaluate gpos; mark request done */
>> >        tevent_req_done(req);
>> >        return;
>> >    case GPO_ACCESS_CONTROL_PERMISSIVE:
>> >    case GPO_ACCESS_CONTROL_ENFORCING:
>> >        /* continue on to evaluate gpos */
>> >        break;
>> >    default:
>> >        tevent_req_error(req, EINVAL);
>> >        return;
>> >    }
>> >
>> >Regards,
>> >Yassir.
>> 
>> I saw another warnigs from static analysers.
>> 
>> Error: CLANG_WARNING: [#def1]
>> sssd-1.11.90/src/providers/ad/ad_gpo.c:777:14: warning: Function call
>> argument is an uninitialized value
>> #    subreq = ad_gpo_process_gpo_send(state,
>> #             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> This one is false possitive.
>> 
>> 
>> 
>> Error: CLANG_WARNING: [#def2]
>> sssd-1.11.90/src/providers/ad/ad_gpo.c:850:15: warning: Access to field
>> 'gpo_guid' results in a dereference of a null pointer
>> #              state->dacl_filtered_gpos[i]->gpo_guid);
>> #              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> sssd-1.11.90/src/util/util.h:127:28: note: expanded from macro 'DEBUG'
>> #                 format, ##__VA_ARGS__); \
>> #                           ^
>> This can be a probelm.
>> I am attaching html report with code flow.
>
>Good catch. I added the following to fix this:
>
>    if (state->dacl_filtered_gpos[0] == NULL) {
>        /* since no applicable gpos were found, there is nothing to enforce */
>        DEBUG(SSSDBG_TRACE_FUNC,
>              "no applicable gpos found after dacl filtering\n");
>        ret = EOK;
>        goto done;
>    }
>
>> 
>> 
>> 
>> Error: CLANG_WARNING: [#def3]
>> sssd-1.11.90/src/providers/ad/ad_gpo.c:2212:5: warning: Value stored to 'ret'
>> is never read
>> #    ret = ad_gpo_parse_sd(gp_gpo, el[0].values[0].data,
>> el[0].values[0].length,
>> #    ^
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> This can be solved with adding debug message.
>> 
>>    2212 ret = ad_gpo_parse_sd(gp_gpo, el[0].values[0].data,
>>    el[0].values[0].length,
>>    2213    &gp_gpo->gpo_sd);
>>    2214
>> 
>>    Value stored to 'ret' is never read
>>    2215 ret = ad_gpo_get_gpo_attrs_step(req);
>
>Good catch. I added the following to fix this:
>
>    if (ret != EOK) {
>        DEBUG(SSSDBG_OP_FAILURE, "ad_gpo_parse_sd() failed\n");
>        goto done;
>    }
>
>> 
>> Thank your patience.
>> 
>> LS
>> 
>
>Revised patch is attached.
>
>Regards,
>Yassir.

>From ab9b95c39d7bfc95b3073e87661ecbdabfa8a8de Mon Sep 17 00:00:00 2001
>From: Yassir Elley <yelley at redhat.com>
>Date: Mon, 20 Jan 2014 11:17:06 -0500
>Subject: [PATCH] Implemented LDAP component of GPO-based access control
>
Yassir, thank you.
I can confirm that warnings from static analysers and compilers are gone.

LS



More information about the sssd-devel mailing list