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

Jakub Hrozek jhrozek at redhat.com
Tue May 13 16:10:06 UTC 2014


On Tue, May 13, 2014 at 11:57:54AM -0400, Yassir Elley wrote:
> 
> 
> ----- 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.

I can fix that before pushing..



More information about the sssd-devel mailing list