[Freeipa-devel] [PATCH 130] extdom: add support for new version
Jakub Hrozek
jhrozek at redhat.com
Wed Sep 24 13:23:54 UTC 2014
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)
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".
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 '||'.
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,...".
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.
>
> #define IPA_EXTDOM_PLUGIN_NAME "ipa-extdom-extop"
> #define IPA_EXTDOM_FEATURE_DESC "IPA trusted domain ID mapper"
> @@ -71,6 +72,11 @@
>
> #define IPA_PLUGIN_NAME IPA_EXTDOM_PLUGIN_NAME
>
> +enum extdom_version {
> + EXTDOM_V1 = 1,
> + EXTDOM_V2
> +};
> +
> enum input_types {
> INP_SID = 1,
> INP_NAME,
> @@ -80,14 +86,17 @@ enum input_types {
>
> enum request_types {
> REQ_SIMPLE = 1,
> - REQ_FULL
> + REQ_FULL,
> + REQ_FULL_WITH_GROUPS
> };
>
> enum response_types {
> RESP_SID = 1,
> RESP_NAME,
> RESP_USER,
> - RESP_GROUP
> + RESP_GROUP,
> + RESP_USER_GROUPLIST,
> + RESP_GROUP_MEMBERS
> };
>
> struct extdom_req {
> @@ -123,11 +132,18 @@ struct extdom_res {
> char *user_name;
> uid_t uid;
> gid_t gid;
> + char *gecos;
> + char *home;
> + char *shell;
> + size_t ngroups;
> + char **groups;
> } user;
> struct {
> char *domain_name;
> char *group_name;
> gid_t gid;
> + size_t nmembers;
> + char **members;
> } group;
> } data;
> };
> @@ -150,15 +166,14 @@ struct pwd_grp {
> struct passwd pwd;
> struct group grp;
> } data;
> + int ngroups;
> + gid_t *groups;
> };
>
> int parse_request_data(struct berval *req_val, struct extdom_req **_req);
> void free_req_data(struct extdom_req *req);
> +int check_request(struct extdom_req *req, enum extdom_version version);
> int handle_request(struct ipa_extdom_ctx *ctx, struct extdom_req *req,
> - struct extdom_res **res);
> -int create_response(struct extdom_req *req, struct pwd_grp *pg_data,
> - const char *sid_str, enum sss_id_type id_type,
> - const char *domain_name, struct extdom_res **_res);
> -void free_resp_data(struct extdom_res *res);
> + struct berval **berval);
> int pack_response(struct extdom_res *res, struct berval **ret_val);
> #endif /* _IPA_EXTDOM_H_ */
> diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
> index 025d37dc5eda05c8db43d4e8176fd7898ed32fe7..5c1ae79c818676c3660d5cd5b8ca5515a4f0f18d 100644
> --- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
> +++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
> @@ -70,6 +70,7 @@ int parse_request_data(struct berval *req_val, struct extdom_req **_req)
> * requestType ENUMERATED {
> * simple (1),
> * full (2)
> + * full_with_groups (3)
> * },
> * data InputData
> * }
> @@ -179,23 +180,23 @@ void free_req_data(struct extdom_req *req)
> free(req);
> }
>
> -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?
> +
> + 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?
> + 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?
> + }
> +
> + 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?
The rest of the file looks to me, just the same "issue" with guessing the
request type is repeated.
More information about the Freeipa-devel
mailing list