[PATCH] AD: conflicting gpo policy settings not being resolved correctly

[Yassir Elley]

Hi,

The attached patch fixes ticket #2437 (conflicting gpo policy settings not being resolved correctly)

https://fedorahosted.org/sssd/ticket/2437

Regards,
Yassir.

[Yassir Elley]

----- Original Message -----
> Hi,
>
> The attached patch fixes ticket #2437 (conflicting gpo policy settings not
> being resolved correctly)
>
> https://fedorahosted.org/sssd/ticket/2437
>
> Regards,
> Yassir.
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

I discovered some more issues while doing further testing on this ticket relating to "defined but empty" policy settings (see additional comments on ticket #2347). I have also made some minor changes to this patch, b/c they didn't warrant their own ticket. Namely, I have removed some attributes that we weren't using (AD_AT_USER_EXT_NAMES, etc). I have also fixed the way we handle machine_ext_names by allowing for the possibility of a "present but empty" value for AD_AT_MACHINE_EXT_NAMES.

I am attaching a revised path to address these issues.

Regards,
Yassir.

[Yassir Elley]

----- Original Message -----
> ----- Original Message -----
> > Hi,
> >
> > The attached patch fixes ticket #2437 (conflicting gpo policy settings not
> > being resolved correctly)
> >
> > https://fedorahosted.org/sssd/ticket/2437
> >
> > Regards,
> > Yassir.
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> I discovered some more issues while doing further testing on this ticket
> relating to "defined but empty" policy settings (see additional comments on
> ticket #2347). I have also made some minor changes to this patch, b/c they
> didn't warrant their own ticket. Namely, I have removed some attributes that
> we weren't using (AD_AT_USER_EXT_NAMES, etc). I have also fixed the way we
> handle machine_ext_names by allowing for the possibility of a "present but
> empty" value for AD_AT_MACHINE_EXT_NAMES.
>
> I am attaching a revised path to address these issues.
>
> Regards,
> Yassir.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

I have implemented the new design (described in ticket #2347) in which policy settings are written (and potentially over-written) to a "GPO Enforcement" object (which I ended up calling "GPO Result" object) in the sysdb cache. This allows policy settings to be resolved correctly for both the online and offline cases. It is a reasonably large patch, but I think the final implementation is cleaner and simpler. Revised patch is attached.

Regards,
Yassir.

[Stephen Gallagher]

On Thu, 2014-09-11 at 23:51 -0400, Yassir Elley wrote:
> ----- Original Message -----
> > ----- Original Message -----
> > > Hi,
> > >
> > > The attached patch fixes ticket #2437 (conflicting gpo policy settings not
> > > being resolved correctly)
> > >
> > > https://fedorahosted.org/sssd/ticket/2437
> > >
> > > Regards,
> > > Yassir.
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > I discovered some more issues while doing further testing on this ticket
> > relating to "defined but empty" policy settings (see additional comments on
> > ticket #2347). I have also made some minor changes to this patch, b/c they
> > didn't warrant their own ticket. Namely, I have removed some attributes that
> > we weren't using (AD_AT_USER_EXT_NAMES, etc). I have also fixed the way we
> > handle machine_ext_names by allowing for the possibility of a "present but
> > empty" value for AD_AT_MACHINE_EXT_NAMES.
> >
> > I am attaching a revised path to address these issues.
> >
> > Regards,
> > Yassir.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> I have implemented the new design (described in ticket #2347) in which policy settings are written (and potentially over-written) to a "GPO Enforcement" object (which I ended up calling "GPO Result" object) in the sysdb cache. This allows policy settings to be resolved correctly for both the online and offline cases. It is a reasonably large patch, but I think the final implementation is cleaner and simpler. Revised patch is attached.

Sorry it took me so long to finish this review. The code is mostly
right, but I found three issues that needed to be addressed before we
could commit it.

1) The GPO code was returning EACCES instead of ERR_ACCESS_DENIED which
was resulting in the PAM conversation ultimately returning
PAM_SYSTEM_ERR. (Pre-existing, but found while testing this patch) Fixed
in the 0001 patch attached. Can be applied separately.

2) There was an 'attrs' variable being passed in the
sysdb_gpo_get_gpo_result_setting() routine without a NULL terminator,
causing a crash when accessing LDB.

3) The code could not properly handle when a GPO had an explicitly empty
value (such as one might use to expressly disable a setting from a
lower-priority GPO).

Issues 2) and 3) are both addressed by the 0002 patch. For an easy view
of the changes I made, see
https://reviewboard-fedoraserver.rhcloud.com/r/80/diff/2-3/ (I tracked
my review there as I went through, so I wouldn't miss any corrections
and as a sort of proof-of-concept)

I (believe) I have done a very thorough review of this code, so I'd ask
that someone double-check my updates and then we should be good to go
with committing this. It's a pretty important set of fixes.

[Stephen Gallagher]

On Wed, 2014-10-01 at 22:50 -0400, Stephen Gallagher wrote:
> On Thu, 2014-09-11 at 23:51 -0400, Yassir Elley wrote:
> > ----- Original Message -----
> > > ----- Original Message -----
> > > > Hi,
> > > >
> > > > The attached patch fixes ticket #2437 (conflicting gpo policy settings not
> > > > being resolved correctly)
> > > >
> > > > https://fedorahosted.org/sssd/ticket/2437
> > > >
> > > > Regards,
> > > > Yassir.
> > > >
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > I discovered some more issues while doing further testing on this ticket
> > > relating to "defined but empty" policy settings (see additional comments on
> > > ticket #2347). I have also made some minor changes to this patch, b/c they
> > > didn't warrant their own ticket. Namely, I have removed some attributes that
> > > we weren't using (AD_AT_USER_EXT_NAMES, etc). I have also fixed the way we
> > > handle machine_ext_names by allowing for the possibility of a "present but
> > > empty" value for AD_AT_MACHINE_EXT_NAMES.
> > >
> > > I am attaching a revised path to address these issues.
> > >
> > > Regards,
> > > Yassir.
> > > _______________________________________________
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > I have implemented the new design (described in ticket #2347) in which policy settings are written (and potentially over-written) to a "GPO Enforcement" object (which I ended up calling "GPO Result" object) in the sysdb cache. This allows policy settings to be resolved correctly for both the online and offline cases. It is a reasonably large patch, but I think the final implementation is cleaner and simpler. Revised patch is attached.
>
> Sorry it took me so long to finish this review. The code is mostly
> right, but I found three issues that needed to be addressed before we
> could commit it.
> The GPO code was returning EACCES instead of ERR_ACCESS_DENIED which
>
> was resulting in the PAM conversation ultimately returning
> PAM_SYSTEM_ERR. (Pre-existing, but found while testing this patch) Fixed
> in the 0001 patch attached. Can be applied separately.
> There was an 'attrs' variable being passed in the
>
> sysdb_gpo_get_gpo_result_setting() routine without a NULL terminator,
> causing a crash when accessing LDB.
> The code could not properly handle when a GPO had an explicitly empty
>
> value (such as one might use to expressly disable a setting from a
> lower-priority GPO).
>
> Issues 2) and 3) are both addressed by the 0002 patch. For an easy view
> of the changes I made, see
> https://reviewboard-fedoraserver.rhcloud.com/r/80/diff/2-3/ (I tracked
> my review there as I went through, so I wouldn't miss any corrections
> and as a sort of proof-of-concept)
>
> I (believe) I have done a very thorough review of this code, so I'd ask
> that someone double-check my updates and then we should be good to go
> with committing this. It's a pretty important set of fixes.

One additional note. This incorrect resolution order does *NOT* cause a
security issue. I've carefully tested that the way processing was
happening would only cause unintentional denials. (Specifically, we were
too zealous about returning a denial as soon as we saw one, even though
AD's GPOs permit the deny list to be overruled by a higher-priority
GPO.) However, because of the way we were doing the lookups, the reverse
was not true. A grant of permission overridden by an exclusion of that
user/group from the permitted list would still not grant permission,
because we were re-validating access permission for each GPO, so it had
to be positively accepted at all levels to succeed.

[Jakub Hrozek]

On Wed, Oct 01, 2014 at 10:50:26PM -0400, Stephen Gallagher wrote:
> Sorry it took me so long to finish this review. The code is mostly
> right, but I found three issues that needed to be addressed before we
> could commit it.
> The GPO code was returning EACCES instead of ERR_ACCESS_DENIED which
>
> was resulting in the PAM conversation ultimately returning
> PAM_SYSTEM_ERR. (Pre-existing, but found while testing this patch) Fixed
> in the 0001 patch attached. Can be applied separately.

ACK
> There was an 'attrs' variable being passed in the
>
> sysdb_gpo_get_gpo_result_setting() routine without a NULL terminator,
> causing a crash when accessing LDB.
> The code could not properly handle when a GPO had an explicitly empty
>
> value (such as one might use to expressly disable a setting from a
> lower-priority GPO).
>
> Issues 2) and 3) are both addressed by the 0002 patch. For an easy view
> of the changes I made, see
> https://reviewboard-fedoraserver.rhcloud.com/r/80/diff/2-3/ (I tracked
> my review there as I went through, so I wouldn't miss any corrections
> and as a sort of proof-of-concept)

Thank you, the diff looks good to me.
> I (believe) I have done a very thorough review of this code, so I'd ask
> that someone double-check my updates and then we should be good to go
> with committing this. It's a pretty important set of fixes.

I will push the second patch if you agree to squash in two additional fixes
in the attached diff. One removes an unused function (I have -Werror set
for these types of warnings), the other fixes a potential uninitialized
pointer access.

[Stephen Gallagher]

On Thu, 2014-10-02 at 11:45 +0200, Jakub Hrozek wrote:
> On Wed, Oct 01, 2014 at 10:50:26PM -0400, Stephen Gallagher wrote:
> > Sorry it took me so long to finish this review. The code is mostly
> > right, but I found three issues that needed to be addressed before we
> > could commit it.
> > The GPO code was returning EACCES instead of ERR_ACCESS_DENIED which
> >
> > was resulting in the PAM conversation ultimately returning
> > PAM_SYSTEM_ERR. (Pre-existing, but found while testing this patch) Fixed
> > in the 0001 patch attached. Can be applied separately.
>
> ACK
> > There was an 'attrs' variable being passed in the
> >
> > sysdb_gpo_get_gpo_result_setting() routine without a NULL terminator,
> > causing a crash when accessing LDB.
> > The code could not properly handle when a GPO had an explicitly empty
> >
> > value (such as one might use to expressly disable a setting from a
> > lower-priority GPO).
> >
> > Issues 2) and 3) are both addressed by the 0002 patch. For an easy view
> > of the changes I made, see
> > https://reviewboard-fedoraserver.rhcloud.com/r/80/diff/2-3/ (I tracked
> > my review there as I went through, so I wouldn't miss any corrections
> > and as a sort of proof-of-concept)
>
> Thank you, the diff looks good to me.
> > I (believe) I have done a very thorough review of this code, so I'd ask
> > that someone double-check my updates and then we should be good to go
> > with committing this. It's a pretty important set of fixes.
>
> I will push the second patch if you agree to squash in two additional fixes
> in the attached diff. One removes an unused function (I have -Werror set
> for these types of warnings), the other fixes a potential uninitialized
> pointer access.

Thanks, good catches. I squashed your patch into the attached ones.

[Jakub Hrozek]

On Thu, Oct 02, 2014 at 08:02:00AM -0400, Stephen Gallagher wrote:
> On Thu, 2014-10-02 at 11:45 +0200, Jakub Hrozek wrote:
> > On Wed, Oct 01, 2014 at 10:50:26PM -0400, Stephen Gallagher wrote:
> > > Sorry it took me so long to finish this review. The code is mostly
> > > right, but I found three issues that needed to be addressed before we
> > > could commit it.
> > > The GPO code was returning EACCES instead of ERR_ACCESS_DENIED which
> > >
> > > was resulting in the PAM conversation ultimately returning
> > > PAM_SYSTEM_ERR. (Pre-existing, but found while testing this patch) Fixed
> > > in the 0001 patch attached. Can be applied separately.
> >
> > ACK
> > > There was an 'attrs' variable being passed in the
> > >
> > > sysdb_gpo_get_gpo_result_setting() routine without a NULL terminator,
> > > causing a crash when accessing LDB.
> > > The code could not properly handle when a GPO had an explicitly empty
> > >
> > > value (such as one might use to expressly disable a setting from a
> > > lower-priority GPO).
> > >
> > > Issues 2) and 3) are both addressed by the 0002 patch. For an easy view
> > > of the changes I made, see
> > > https://reviewboard-fedoraserver.rhcloud.com/r/80/diff/2-3/ (I tracked
> > > my review there as I went through, so I wouldn't miss any corrections
> > > and as a sort of proof-of-concept)
> >
> > Thank you, the diff looks good to me.
> > > I (believe) I have done a very thorough review of this code, so I'd ask
> > > that someone double-check my updates and then we should be good to go
> > > with committing this. It's a pretty important set of fixes.
> >
> > I will push the second patch if you agree to squash in two additional fixes
> > in the attached diff. One removes an unused function (I have -Werror set
> > for these types of warnings), the other fixes a potential uninitialized
> > pointer access.
>
> Thanks, good catches. I squashed your patch into the attached ones.

ACK to both!

[Jakub Hrozek]

On Thu, Oct 02, 2014 at 02:06:59PM +0200, Jakub Hrozek wrote:
> > Thanks, good catches. I squashed your patch into the attached ones.
>
> ACK to both!

master:
 * 7f4270ee333c1128a87fce6e6a3de64d263733e1
 * 4611802d41d8954a3040f39403590adb920ca521