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

Yassir Elley yelley at redhat.com
Wed May 7 12:47:19 UTC 2014



----- 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Implemented-LDAP-component-of-GPO-based-access-contr.patch
Type: text/x-patch
Size: 107523 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140507/9af504a2/attachment.bin>


More information about the sssd-devel mailing list