[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