[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