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

Lukas Slebodnik lslebodn at redhat.com
Wed May 7 08:11:09 UTC 2014


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.



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);

Thank your patience.

LS
-------------- next part --------------
A non-text attachment was scrubbed...
Name: report-c38607.html.gz
Type: application/gzip
Size: 41440 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140507/9c427c95/attachment.bin>


More information about the sssd-devel mailing list