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.
>
> 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.
ACK
>
> 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)
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.
>From 991c851e8d759a9dfdafbdab9a02724df635ae05 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Thu, 2 Oct 2014 11:10:30 +0200
Subject: [PATCH] Two GPO fixes
---
src/db/sysdb_gpo.c | 14 --------------
src/providers/ad/ad_gpo.c | 2 +-
2 files changed, 1 insertion(+), 15 deletions(-)
diff --git a/src/db/sysdb_gpo.c b/src/db/sysdb_gpo.c
index
54d40e1d15b90abdc9bb551450defd8d19a10635..92559d41e341b00a8031deb6209284a9ee397a42
100644
--- a/src/db/sysdb_gpo.c
+++ b/src/db/sysdb_gpo.c
@@ -352,20 +352,6 @@ done:
return ret;
}
-static inline bool
-sysdb_gpo_guid_in_list(const char **gpo_guid_list, int num_gpos, const char
*gpo_guid)
-{
- size_t i;
-
- for (i = 0; i < num_gpos; i++) {
- if (strcasecmp(gpo_guid_list[i], gpo_guid) == 0) {
- break;
- }
- }
-
- return (i < num_gpos) ? true : false;
-}
-
/* GPO Result */
static struct ldb_dn *
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index
c5aff81fe634843a6ff96fd6639b141d2e2b08ee..cf6023a358bcc1cfb4a46fbd8e743d6d9068e6cd
100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1342,7 +1342,7 @@ parse_policy_setting_value(TALLOC_CTX *mem_ctx,
int i;
const char *value;
int sids_list_size;
- char **sids_list;
+ char **sids_list = NULL;
ret = sysdb_gpo_get_gpo_result_setting(mem_ctx, domain, key, &value);
--
1.9.3
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
