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

Yassir Elley yelley at redhat.com
Tue May 13 15:57:54 UTC 2014



----- Original Message -----
> On Wed, May 07, 2014 at 04:46:14PM +0200, Lukas Slebodnik wrote:
> > 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.
> 
> The latest version still works well in my tests. Only the dates in the
> commit header are looking a bit odd.
> 
> bye,
> Sumit

I think that the commit header shows the "magic date" of September 17, 2001 and the author date of January 20, 2014. I made my initial commit on January 20, and then made additional commits, but always squashed them into the initial commit using "git rebase -i". Since the author date does not change with rebases (although the committer date does), the author date has remained unchanged since the initial commit.

Regards,
Yassir.



More information about the sssd-devel mailing list