[Freeipa-devel] [PATCH 130] extdom: add support for new version
Sumit Bose
sbose at redhat.com
Mon Sep 29 16:15:21 UTC 2014
On Thu, Sep 25, 2014 at 01:46:00PM +0200, Sumit Bose wrote:
> On Wed, Sep 24, 2014 at 03:23:54PM +0200, Jakub Hrozek wrote:
> > On Tue, Sep 23, 2014 at 05:11:01PM +0200, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patch should fix https://fedorahosted.org/freeipa/ticket/4031 and
> > > with the corresponding SSSD part it would be possible to get the full
> > > list of group memberships with the id command even for user who didn't
> > > log in before.
> > >
> > > bye,
> > > Sumit
> >
> > So far I only read the patch, no testing was done yet (I'm installing a
> > separate VM where I'll keep this new plugin for easy comparison and
> > backwards-compatibility testing)
>
> Thank you for the review, please see comments below.
>
> >
> > First, there are some Coverity warnings:
> >
> > Error: USE_AFTER_FREE (CWE-825):
> > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:242: alias: Assigning: "groups" = "new_groups". Now both point to the same storage.
> > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:246: freed_arg: "free(void *)" frees "groups".
> > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:252: use_after_free: Using freed pointer "groups".
>
> fixed
>
> >
> > Error: CONSTANT_EXPRESSION_RESULT (CWE-398):
> > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:596: missing_parentheses: "!id_type != SSS_ID_TYPE_GID" is always true regardless of the values of its operands. Did you intend to either negate the entire comparison expression, in which case parentheses would be required around the entire comparison expression to force that interpretation, or negate the sense of the comparison (that is, use '==' rather than '!=')? This occurs as the logical second operand of '||'.
>
> fixed
>
> >
> > Error: DEADCODE (CWE-561):
> > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:594: cond_cannot_single: Condition "request_type == 1U", taking false branch. Now the value of "request_type" cannot be equal to 1.
> > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:594: cond_cannot_set: Condition "request_type == 3U", taking false branch. Now the value of "request_type" cannot be equal to any of {1, 3}.
> > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:606: cannot_set: At condition "request_type == 1U", the value of "request_type" cannot be equal to any of {1, 3}.
> > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:606: dead_error_condition: The condition "request_type == 1U" cannot be true.
> > freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:607: dead_error_line: Execution cannot reach this statement "ret = pack_ber_sid(sid_str,...".
>
> I thik this is a result of the CONSTANT_EXPRESSION_RESULT issue, since I
> fixed it this warning should be gone as well.
>
> >
> > See some comments inline.
> >
> > > From 23ff38cdea85995b211e73f474bcb4b0d7fb8039 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose at redhat.com>
> > > Date: Tue, 23 Sep 2014 15:55:43 +0200
> > > Subject: [PATCH] extdom: add support for new version
> > >
> > > Currently the extdom plugin is basically used to translate SIDs of AD
> > > users and groups to names and POSIX IDs.
> > >
> > > With this patch a new version is added which will return the full member
> > > list for groups and the full list of group memberships for a user.
> > > Additionally the gecos field, the home directory and the login shell of a
> > > user are returned and an optional list of key-value pairs which
> > > currently will contain the SID of the requested object if available.
> > >
> > > https://fedorahosted.org/freeipa/ticket/4031
> > > ---
> > > .../ipa-extdom-extop/ipa_extdom.h | 29 +-
> > > .../ipa-extdom-extop/ipa_extdom_common.c | 850 +++++++++++++++------
> > > .../ipa-extdom-extop/ipa_extdom_extop.c | 28 +-
> > > 3 files changed, 640 insertions(+), 267 deletions(-)
> > >
> > > diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
> > > index 5f834a047a579104cd2589ce417c580c1c5388d3..548ee74f561c474854c049726c4c3e71da5cbbea 100644
> > > --- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
> > > +++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
> > > @@ -64,6 +64,7 @@
> > > #include <sss_nss_idmap.h>
> > >
> > > #define EXOP_EXTDOM_OID "2.16.840.1.113730.3.8.10.4"
> > > +#define EXOP_EXTDOM_V2_OID "2.16.840.1.113730.3.8.10.4.1"
> >
> > It's a bit odd that this control is called V1 in the SSSD tree and V2 in
> > the IPA tree. It's not wrong, just strange maybe.
>
> you are right, I renamed the versions here.
>
> >
> > >
> > > -int handle_request(struct ipa_extdom_ctx *ctx, struct extdom_req *req,
> > > - struct extdom_res **res)
> > > +int check_request(struct extdom_req *req, enum extdom_version version)
> > > +{
> > > + if (version == EXTDOM_V1) {
> > > + if (req->request_type == REQ_FULL_WITH_GROUPS) {
> > > + return LDAP_PROTOCOL_ERROR;
> > > + }
> > > + }
> >
> > Any particular reason why these conditions are nested and not and-ed ?
> > Did you expect more under the EXTDOM_V1 condition?
>
> I'm not expecting them, but who knows :-) I think this way it is more
> clear that we are testing features of a specific version here.
>
> >
> > > +
> > > + return LDAP_SUCCESS;
> > > +}
> > > +
> > > +static int get_buffer(size_t *_buf_len, char **_buf)
> > > {
> > > - int ret;
> > > - char *domain_name = NULL;
> > > - char *sid_str = NULL;
> > > - size_t buf_len;
> > > - char *buf = NULL;
> > > long pw_max;
> > > long gr_max;
> > > - struct pwd_grp pg_data;
> > > - struct passwd *pwd_result = NULL;
> > > - struct group *grp_result = NULL;
> > > - enum sss_id_type id_type;
> > > - char *fq_name = NULL;
> > > - char *sep;
> > > -
> > > + size_t buf_len;
> > > + char *buf;
> > >
> > > pw_max = sysconf(_SC_GETPW_R_SIZE_MAX);
> > > gr_max = sysconf(_SC_GETGR_R_SIZE_MAX);
> > > @@ -211,302 +212,655 @@ int handle_request(struct ipa_extdom_ctx *ctx, struct extdom_req *req,
> > > return LDAP_OPERATIONS_ERROR;
> > > }
> > >
> > > - switch (req->input_type) {
> > > - case INP_POSIX_UID:
> > > - if (req->request_type == REQ_SIMPLE) {
> > > - ret = sss_nss_getsidbyid(req->data.posix_uid.uid, &sid_str,
> > > - &id_type);
> > > + *_buf_len = buf_len;
> > > + *_buf = buf;
> > > +
> > > + return LDAP_SUCCESS;
> > > +}
> > > +
> > > +static int get_user_grouplist(const char *name, gid_t gid,
> > > + size_t *_ngroups, gid_t **_groups )
> > > +{
> > > + int ret;
> > > + int ngroups;
> > > + gid_t *groups;
> > > + gid_t *new_groups;
> > > +
> > > + ngroups = 128;
> >
> > I was wondering whether to use _SC_NGROUPS_MAX or NGROUPS_MAX here, but
> > I guess you're right it's very unlikely that a user will be a member of
> > more than 128 groups so we'd just clinge to more memory than needed..
> >
> > > + groups = malloc(ngroups * sizeof(gid_t));
> > > + if (groups == NULL) {
> > > + return LDAP_OPERATIONS_ERROR;
> > > + }
> > > +
> > > + ret = getgrouplist(name, gid, groups, &ngroups);
> > > + if (ret == -1) {
> > > + new_groups = realloc(groups, ngroups);
> > > + if (new_groups == NULL) {
> > > + free(groups);
> > > + return LDAP_OPERATIONS_ERROR;
> > > + }
> > > + groups = new_groups;
> > > +
> > > + ret = getgrouplist(name, gid, groups, &ngroups);
> > > + if (ret == -1) {
> > > + free(groups);
> > > + ret = LDAP_OPERATIONS_ERROR;
> > > + }
> > > + }
> > > +
> > > + *_ngroups = ngroups;
> > > + *_groups = groups;
> > > +
> > > + return LDAP_SUCCESS;
> > > +}
> > > +
> > > +static int pack_ber_sid(const char *sid, struct berval **berval)
> > > +{
> > > + BerElement *ber = NULL;
> > > + int ret;
> > > +
> > > + ber = ber_alloc_t( LBER_USE_DER );
> > > + if (ber == NULL) {
> > > + return LDAP_OPERATIONS_ERROR;
> > > + }
> > > +
> > > + ret = ber_printf(ber,"{es}", RESP_SID, sid);
> > > + if (ret == -1) {
> > > + ber_free(ber, 1);
> > > + return LDAP_OPERATIONS_ERROR;
> > > + }
> > > +
> > > + ret = ber_flatten(ber, berval);
> > > + ber_free(ber, 1);
> > > + if (ret == -1) {
> > > + return LDAP_OPERATIONS_ERROR;
> > > + }
> > > +
> > > + return LDAP_SUCCESS;
> > > +}
> > > +
> > > +#define SSSD_SYSDB_SID_STR "objectSIDString"
> > > +
> > > +static int pack_ber_user(const char *domain_name, const char *user_name,
> > > + uid_t uid, gid_t gid,
> > > + const char *gecos, const char *homedir,
> > > + const char *shell, const char *sid_str,
> > > + struct berval **berval)
> > > +{
> > > + BerElement *ber = NULL;
> > > + int ret;
> > > + enum response_types response_type;
> > > + size_t ngroups;
> > > + gid_t *groups = NULL;
> > > + size_t buf_len;
> > > + char *buf = NULL;
> > > + struct group grp;
> > > + struct group *grp_result;
> > > + size_t c;
> > > + char *locat;
> > > + char *short_user_name = NULL;
> > > + const char *single_value_string_array[] = {NULL, NULL};
> > > +
> > > + if (gecos == NULL && homedir == NULL && shell == NULL) {
> > > + response_type = RESP_USER;
> > > + } else {
> > > + response_type = RESP_USER_GROUPLIST;
> > > + }
> > > +
> > > + short_user_name = strdup(user_name);
> > > + if ((locat = strchr(short_user_name, SSSD_DOMAIN_SEPARATOR)) != NULL) {
> >
> > Some functions in the code use strchr to fund the at-sign, some use
> > strrch. Could we standardize on one or the other? Do you expect some
> > usernames with an at-sign in them?
>
> I think the 'rr' version was just a typo, I changed it to 'r'.
>
> >
> > > + if (strcasecmp(locat+1, domain_name) == 0 ) {
> > > + locat[0] = '\0';
> > > } else {
> > > - id_type = SSS_ID_TYPE_UID;
> > > - ret = getpwuid_r(req->data.posix_uid.uid, &pg_data.data.pwd, buf,
> > > - buf_len, &pwd_result);
> > > + ret = LDAP_NO_SUCH_OBJECT;
> > > + goto done;
> > > }
> > > + }
> > >
> > > - domain_name = strdup(req->data.posix_uid.domain_name);
> > > - break;
> > > - case INP_POSIX_GID:
> > > - if (req->request_type == REQ_SIMPLE) {
> > > - ret = sss_nss_getsidbyid(req->data.posix_uid.uid, &sid_str,
> > > - &id_type);
> > > + ber = ber_alloc_t( LBER_USE_DER );
> > > + if (ber == NULL) {
> > > + return LDAP_OPERATIONS_ERROR;
> > > + }
> > > +
> > > + ret = ber_printf(ber,"{e{ssii", response_type, domain_name, short_user_name,
> > > + uid, gid);
> > > + if (ret == -1) {
> > > + ret = LDAP_OPERATIONS_ERROR;
> > > + goto done;
> > > + }
> > > +
> > > + if (response_type == RESP_USER_GROUPLIST) {
> > > + ret = get_user_grouplist(user_name, gid, &ngroups, &groups);
> > > + if (ret != LDAP_SUCCESS) {
> > > + goto done;
> > > + }
> > > +
> > > + ret = get_buffer(&buf_len, &buf);
> > > + if (ret != LDAP_SUCCESS) {
> > > + goto done;
> > > + }
> > > +
> > > + ret = ber_printf(ber,"sss", gecos, homedir, shell);
> > > + if (ret == -1) {
> > > + ret = LDAP_OPERATIONS_ERROR;
> > > + goto done;
> > > + }
> > > +
> > > + ret = ber_printf(ber,"{");
> > > + if (ret == -1) {
> > > + ret = LDAP_OPERATIONS_ERROR;
> > > + goto done;
> > > + }
> > > +
> > > + for (c = 0; c < ngroups; c++) {
> > > + ret = getgrgid_r(groups[c], &grp, buf, buf_len, &grp_result);
> > > + if (ret != 0) {
> > > + ret = LDAP_OPERATIONS_ERROR;
> > > + goto done;
> > > + }
> > > + if (grp_result == NULL) {
> > > + ret = LDAP_NO_SUCH_OBJECT;
> > > + goto done;
> >
> > I wanted to check if you think it's better to continue or fail here. Did
> > you opt for failing because you were afraid of missing some deny access
> > checks in case we couldn't resolv a group?
>
> I think there is a disconnect if getgrouplist() returns a GID that
> cannot be resolved so I prefer an error in this case.
>
> >
> > > + }
> > > +
> > > + ret = ber_printf(ber, "s", grp.gr_name);
> > > + if (ret == -1) {
> > > + ret = LDAP_OPERATIONS_ERROR;
> > > + goto done;
> > > + }
> > > + }
> > > +
> > > + ret = ber_printf(ber,"}");
> > > + if (ret == -1) {
> > > + ret = LDAP_OPERATIONS_ERROR;
> > > + goto done;
> > > + }
> > > +
> > > + single_value_string_array[0] = sid_str;
> > > + ret = ber_printf(ber,"{{s{v}}}", SSSD_SYSDB_SID_STR,
> > > + single_value_string_array);
> > > + if (ret == -1) {
> > > + ret = LDAP_OPERATIONS_ERROR;
> > > + goto done;
> > > + }
> > > + }
> > > +
> > > + ret = ber_printf(ber,"}}");
> > > + if (ret == -1) {
> > > + ret = LDAP_OPERATIONS_ERROR;
> > > + goto done;
> > > + }
> > > +
> > > + ret = ber_flatten(ber, berval);
> > > + if (ret == -1) {
> > > + ret = LDAP_OPERATIONS_ERROR;
> > > + goto done;
> > > + }
> > > +
> > > + ret = LDAP_SUCCESS;
> > > +done:
> > > + free(short_user_name);
> > > + free(groups);
> > > + free(buf);
> > > + ber_free(ber, 1);
> > > + return ret;
> > > +}
> > > +
> > > +static int pack_ber_group(const char *domain_name, const char *group_name,
> > > + gid_t gid, char **members, const char *sid_str,
> > > + struct berval **berval)
> > > +{
> > > + BerElement *ber = NULL;
> > > + int ret;
> > > + size_t c;
> > > + char *locat;
> > > + char *short_group_name = NULL;
> > > + const char *single_value_string_array[] = {NULL, NULL};
> > > +
> > > + short_group_name = strdup(group_name);
> > > + if ((locat = strchr(short_group_name, SSSD_DOMAIN_SEPARATOR)) != NULL) {
> > > + if (strcasecmp(locat+1, domain_name) == 0 ) {
> > > + locat[0] = '\0';
> > > } else {
> > > - id_type = SSS_ID_TYPE_GID;
> > > - ret = getgrgid_r(req->data.posix_gid.gid, &pg_data.data.grp, buf,
> > > - buf_len, &grp_result);
> > > + ret = LDAP_NO_SUCH_OBJECT;
> > > + goto done;
> > > }
> > > + }
> > >
> > > - domain_name = strdup(req->data.posix_gid.domain_name);
> > > - break;
> > > - case INP_SID:
> > > - ret = sss_nss_getnamebysid(req->data.sid, &fq_name, &id_type);
> > > - if (ret != 0) {
> > > + ber = ber_alloc_t( LBER_USE_DER );
> > > + if (ber == NULL) {
> > > + return LDAP_OPERATIONS_ERROR;
> > > + }
> > > +
> > > + ret = ber_printf(ber,"{e{ssi", members == NULL ? RESP_GROUP
> > > + : RESP_GROUP_MEMBERS,
> >
> > Each pack_ber_group is called like this:
> > 718 if (request_type == REQ_FULL) {
> > 719 ret = pack_ber_group(domain_name, grp.gr_name, grp.gr_gid,
> > 720 NULL, NULL, berval);
> > 721 } else {
> > 722 ret = pack_ber_group(domain_name, grp.gr_name, grp.gr_gid,
> > 723 grp.gr_mem, sid, berval);
> > 724 }
> >
> > And then you guess the request_type again based on the parameter
> > values. Isn't it safer to add the request type parameter avoid the if-else
> > switch in the callers? Or were you trying to be on the safe side to avoid
> > checking the validity members array in the pack_ber_group function and have
> > the array set to NULL by the caller?
>
> You are right, the if-block is odd. Instead of the request_type I added
> the response_type to the argument list of pack_ber_user() and
> pack_ber_group() which I think is more natural because it is the
> response that is packed.
>
> >
> > The rest of the file looks to me, just the same "issue" with guessing the
> > request type is repeated.
> >
>
> New version attached.
>
> bye,
> Sumit
Hi,
Jakub found another issue which is fixed with this new version.
bye,
Sumit
More information about the Freeipa-devel
mailing list