On (19/01/16 15:38), Simo Sorce wrote:
>On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
>> On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
>> [...]
>> > >+#endif /* __SSSD_UTIL_SELINUX_H__ */
>> > BTW will we need this header file if we make
>> > struct cli_creds opaque?
>>
>> Replying to both your mails here:
>> Making cli_creds opaque requires using a pointer and dealing with
>> allocating it, I guess I can do that, the headers file would still be
>> needed in order to avoid huge ifdefs around the functions that implement
>> handling SELinux stuff. It makes the code a lot more readable and
>> searchable.
>>
>> Simo.
>>
>
>Attached find patch that changes the code so that cli_creds is now
>opaque.
>This *should* work w/o the patch that changes the headers but I haven't
>tested it w/o that patch yet.
>
I had an issue to build it correctly with ifp responder.
src/responder/ifp/ifpsrv_util.c: In function ‘ifp_req_create’:
src/responder/ifp/ifpsrv_util.c:64:30: error: passing argument 1 of
‘check_allowed_uids’ makes pointer from integer without a cast
[-Werror=int-conversion]
ret = check_allowed_uids(dbus_req->client,
^
In file included from ../sssd/src/responder/ifp/ifp_private.h:27:0,
from ../sssd/src/responder/ifp/ifpsrv_util.c:27:
src/responder/common/responder.h:334:9: note: expected ‘struct cli_creds *’ but
argument is of type ‘int64_t {aka long int}’
errno_t check_allowed_uids(struct cli_creds *creds,
^
ifp responder uses different way to obtain uid. I changed back the prototype of
check_allowed_uids.
Here is a diff on to of your patch.
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 419a863..3b70b69 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -331,8 +331,7 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const
char *csv_string,
size_t *_uid_count, uid_t **_uids);
uid_t client_euid(struct cli_creds *creds);
-errno_t check_allowed_uids(struct cli_creds *creds,
- size_t allowed_uids_count,
+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
uid_t *allowed_uids);
struct tevent_req *
diff --git a/src/responder/common/responder_common.c
b/src/responder/common/responder_common.c
index 27193ce..6ac1ea2 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -138,8 +138,7 @@ uid_t client_euid(struct cli_creds *creds)
return cli_creds_get_uid(creds);
}
-errno_t check_allowed_uids(struct cli_creds *creds,
- size_t allowed_uids_count,
+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
uid_t *allowed_uids)
{
size_t c;
@@ -149,7 +148,7 @@ errno_t check_allowed_uids(struct cli_creds *creds,
}
for (c = 0; c < allowed_uids_count; c++) {
- if (client_euid(creds) == allowed_uids[c]) {
+ if (uid == allowed_uids[c]) {
return EOK;
}
}
@@ -449,12 +448,13 @@ static void accept_fd_handler(struct tevent_context *ev,
return;
}
- ret = check_allowed_uids(cctx->creds, rctx->allowed_uids_count,
+ ret = check_allowed_uids(client_euid(cctx->creds),
rctx->allowed_uids_count,
rctx->allowed_uids);
if (ret != EOK) {
if (ret == EACCES) {
- DEBUG(SSSDBG_CRIT_FAILURE, "Access denied for uid [%d].\n",
- (int)client_euid(cctx->creds));
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Access denied for uid [%"SPRIuid"].\n",
+ client_euid(cctx->creds));
} else {
DEBUG(SSSDBG_OP_FAILURE, "check_allowed_uids failed.\n");
}
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 5fe3e6b..bfc534f 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -1001,7 +1001,7 @@ static bool is_uid_trusted(struct cli_creds *creds,
return true;
}
- ret = check_allowed_uids(creds, trusted_uids_count, trusted_uids);
+ ret = check_allowed_uids(client_euid(creds), trusted_uids_count,
trusted_uids);
if (ret == EOK) return true;
return false;
@@ -1091,13 +1091,13 @@ static int pam_forwarder(struct cli_ctx *cctx, int
pam_cmd)
}
pd = preq->pd;
- preq->is_uid_trusted = is_uid_trusted(&cctx->creds,
+ preq->is_uid_trusted = is_uid_trusted(cctx->creds,
pctx->trusted_uids_count,
pctx->trusted_uids);
if (!preq->is_uid_trusted) {
- DEBUG(SSSDBG_MINOR_FAILURE, "uid %d is not trusted.\n",
- (int)client_euid(&cctx->creds));
+ DEBUG(SSSDBG_MINOR_FAILURE, "uid %"SPRIuid" is not trusted.\n",
+ client_euid(cctx->creds));
}
@@ -1776,8 +1776,8 @@ static void pam_dom_forwarder(struct pam_auth_req *preq)
!is_domain_public(preq->pd->domain, pctx->public_domains,
pctx->public_domains_count)) {
DEBUG(SSSDBG_MINOR_FAILURE,
- "Untrusted user %d cannot access non-public domain %s.\n",
- (int)client_euid(&preq->cctx->creds), preq->pd->domain);
+ "Untrusted user %"SPRIuid" cannot access non-public domain
%s.\n",
+ client_euid(preq->cctx->creds), preq->pd->domain);
preq->pd->pam_status = PAM_PERM_DENIED;
pam_reply(preq);
return;
I know you are busy. I fixed few conflics + updated your patch and
added my singoff. I hope you don mind.
If you are find with attached patch then we need to find 3rd person
for review (and ACK)
LS
>From 69b9fb82242b91f6f51245b633861fe01db92839 Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Wed, 20 Jan 2016 09:12:47 +0100
Subject: [PATCH] Util: Improve code to get connection credentials
Adds support to get SELINUX context and make code more abstract so
that struct ucred (if availale) can be used w/o redefining uid,gid,pid to
int32. Also gives a layer of indirection that may come handy if we want
to improve the code further in the future.
Signed-off-by: Lukas Slebodnik <lsleb...@redhat.com>
---
Makefile.am | 2 +
src/responder/common/responder.h | 8 ++--
src/responder/common/responder_common.c | 57 +++++++++++++++--------
src/responder/pam/pamsrv_cmd.c | 23 ++++-----
src/util/util_creds.h | 82 +++++++++++++++++++++++++++++++++
5 files changed, 138 insertions(+), 34 deletions(-)
create mode 100644 src/util/util_creds.h
diff --git a/Makefile.am b/Makefile.am
index
6008b06ddf45fa8c6558876db37d9d07788ca9da..588d892cf42ac06bed219d9084c46dcdf3a53eba
100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -497,6 +497,7 @@ SSSD_LIBS = \
$(COLLECTION_LIBS) \
$(DHASH_LIBS) \
$(OPENLDAP_LIBS) \
+ $(SELINUX_LIBS) \
$(TDB_LIBS)
PYTHON_BINDINGS_LIBS = \
@@ -557,6 +558,7 @@ dist_noinst_HEADERS = \
src/util/authtok-utils.h \
src/util/util_safealign.h \
src/util/util_sss_idmap.h \
+ src/util/util_creds.h \
src/monitor/monitor.h \
src/monitor/monitor_interfaces.h \
src/responder/common/responder.h \
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index
f363c2074ab519c4266565aab937921a2eb44a46..3b70b69e4718539a139c7b8889addce2e58f626b
100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -118,6 +118,8 @@ struct resp_ctx {
bool shutting_down;
};
+struct cli_creds;
+
struct cli_ctx {
struct tevent_context *ev;
struct resp_ctx *rctx;
@@ -127,9 +129,8 @@ struct cli_ctx {
struct cli_request *creq;
struct cli_protocol_version *cli_protocol_version;
int priv;
- int32_t client_euid;
- int32_t client_egid;
- int32_t client_pid;
+
+ struct cli_creds *creds;
int pwent_dom_idx;
int pwent_cur;
@@ -329,6 +330,7 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const
char *csv_string,
bool allow_sss_loop,
size_t *_uid_count, uid_t **_uids);
+uid_t client_euid(struct cli_creds *creds);
errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
uid_t *allowed_uids);
diff --git a/src/responder/common/responder_common.c
b/src/responder/common/responder_common.c
index
a7e198cc55428f5d16046cd1732716453999c108..6ac1ea2222984442792a21b53869a2911b431255
100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -42,6 +42,7 @@
#include "providers/data_provider.h"
#include "monitor/monitor_interfaces.h"
#include "sbus/sbus_client.h"
+#include "util/util_creds.h"
static errno_t set_close_on_exec(int fd)
{
@@ -84,16 +85,20 @@ static int client_destructor(struct cli_ctx *ctx)
static errno_t get_client_cred(struct cli_ctx *cctx)
{
- cctx->client_euid = -1;
- cctx->client_egid = -1;
- cctx->client_pid = -1;
+ SEC_CTX secctx;
+ int ret;
+
+ cctx->creds = talloc(cctx, struct cli_creds);
+ if (!cctx->creds) return ENOMEM;
#ifdef HAVE_UCRED
- int ret;
- struct ucred client_cred;
- socklen_t client_cred_len = sizeof(client_cred);
+ socklen_t client_cred_len = sizeof(struct ucred);
- ret = getsockopt(cctx->cfd, SOL_SOCKET, SO_PEERCRED, &client_cred,
+ cctx->creds->ucred.uid = -1;
+ cctx->creds->ucred.gid = -1;
+ cctx->creds->ucred.pid = -1;
+
+ ret = getsockopt(cctx->cfd, SOL_SOCKET, SO_PEERCRED, &cctx->creds->ucred,
&client_cred_len);
if (ret != EOK) {
ret = errno;
@@ -107,15 +112,30 @@ static errno_t get_client_cred(struct cli_ctx *cctx)
return ENOMSG;
}
- cctx->client_euid = client_cred.uid;
- cctx->client_egid = client_cred.gid;
- cctx->client_pid = client_cred.pid;
-
- DEBUG(SSSDBG_TRACE_ALL, "Client creds: euid[%d] egid[%d] pid[%d].\n",
- cctx->client_euid, cctx->client_egid, cctx->client_pid);
+ DEBUG(SSSDBG_TRACE_ALL,
+ "Client creds: euid[%d] egid[%d] pid[%d].\n",
+ cctx->creds->ucred.uid, cctx->creds->ucred.gid,
+ cctx->creds->ucred.pid);
#endif
- return EOK;
+ ret = SELINUX_getpeercon(cctx->cfd, &secctx);
+ if (ret != 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "SELINUX_getpeercon failed [%d][%s].\n", ret, strerror(ret));
+ /* This is not fatal, as SELinux may simply be disabled */
+ ret = EOK;
+ } else {
+ cctx->creds->selinux_ctx = SELINUX_context_new(secctx);
+ SELINUX_freecon(secctx);
+ }
+
+ return ret;
+}
+
+uid_t client_euid(struct cli_creds *creds)
+{
+ if (!creds) return -1;
+ return cli_creds_get_uid(creds);
}
errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
@@ -418,7 +438,7 @@ static void accept_fd_handler(struct tevent_context *ev,
}
if (rctx->allowed_uids_count != 0) {
- if (cctx->client_euid == -1) {
+ if (client_euid(cctx->creds) == -1) {
DEBUG(SSSDBG_CRIT_FAILURE, "allowed_uids configured, " \
"but platform does not support " \
"reading peer credential from the " \
@@ -428,12 +448,13 @@ static void accept_fd_handler(struct tevent_context *ev,
return;
}
- ret = check_allowed_uids(cctx->client_euid, rctx->allowed_uids_count,
+ ret = check_allowed_uids(client_euid(cctx->creds),
rctx->allowed_uids_count,
rctx->allowed_uids);
if (ret != EOK) {
if (ret == EACCES) {
- DEBUG(SSSDBG_CRIT_FAILURE, "Access denied for uid [%d].\n",
- cctx->client_euid);
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Access denied for uid [%"SPRIuid"].\n",
+ client_euid(cctx->creds));
} else {
DEBUG(SSSDBG_OP_FAILURE, "check_allowed_uids failed.\n");
}
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index
b9fd353259834910e85b05f8e5aa20c9c5674501..bfc534f577cf80d5acfab81db30550ab5b5bdd2b
100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -985,14 +985,14 @@ static int pam_auth_req_destructor(struct pam_auth_req
*preq)
return 0;
}
-static bool is_uid_trusted(uint32_t uid,
+static bool is_uid_trusted(struct cli_creds *creds,
size_t trusted_uids_count,
uid_t *trusted_uids)
{
- size_t i;
+ errno_t ret;
/* root is always trusted */
- if (uid == 0) {
+ if (client_euid(creds) == 0) {
return true;
}
@@ -1001,11 +1001,8 @@ static bool is_uid_trusted(uint32_t uid,
return true;
}
- for(i = 0; i < trusted_uids_count; i++) {
- if (trusted_uids[i] == uid) {
- return true;
- }
- }
+ ret = check_allowed_uids(client_euid(creds), trusted_uids_count,
trusted_uids);
+ if (ret == EOK) return true;
return false;
}
@@ -1094,13 +1091,13 @@ static int pam_forwarder(struct cli_ctx *cctx, int
pam_cmd)
}
pd = preq->pd;
- preq->is_uid_trusted = is_uid_trusted(cctx->client_euid,
+ preq->is_uid_trusted = is_uid_trusted(cctx->creds,
pctx->trusted_uids_count,
pctx->trusted_uids);
if (!preq->is_uid_trusted) {
- DEBUG(SSSDBG_MINOR_FAILURE, "uid %"PRIu32" is not trusted.\n",
- cctx->client_euid);
+ DEBUG(SSSDBG_MINOR_FAILURE, "uid %"SPRIuid" is not trusted.\n",
+ client_euid(cctx->creds));
}
@@ -1779,8 +1776,8 @@ static void pam_dom_forwarder(struct pam_auth_req *preq)
!is_domain_public(preq->pd->domain, pctx->public_domains,
pctx->public_domains_count)) {
DEBUG(SSSDBG_MINOR_FAILURE,
- "Untrusted user %"PRIu32" cannot access non-public domain
%s.\n",
- preq->cctx->client_euid, preq->pd->domain);
+ "Untrusted user %"SPRIuid" cannot access non-public domain
%s.\n",
+ client_euid(preq->cctx->creds), preq->pd->domain);
preq->pd->pam_status = PAM_PERM_DENIED;
pam_reply(preq);
return;
diff --git a/src/util/util_creds.h b/src/util/util_creds.h
new file mode 100644
index
0000000000000000000000000000000000000000..65468fa12b8c6921859574c40f5759c936a10e86
--- /dev/null
+++ b/src/util/util_creds.h
@@ -0,0 +1,82 @@
+/*
+ Authors:
+ Simo Sorce <s...@redhat.com>
+
+ Copyright (C) 2016 Red Hat
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#ifndef __SSSD_UTIL_CREDS_H__
+#define __SSSD_UTIL_CREDS_H__
+
+/* following code comes from gss-proxy's gp_selinux.h file */
+#ifdef HAVE_SELINUX
+
+#include <selinux/context.h>
+#define SELINUX_CTX context_t
+#include <selinux/selinux.h>
+#define SEC_CTX security_context_t
+
+#define SELINUX_context_new context_new
+#define SELINUX_context_free context_free
+#define SELINUX_context_str context_str
+#define SELINUX_context_type_get context_type_get
+#define SELINUX_context_user_get context_user_get
+#define SELINUX_context_role_get context_role_get
+#define SELINUX_context_range_get context_range_get
+#define SELINUX_getpeercon getpeercon
+#define SELINUX_freecon freecon
+
+#else /* not HAVE_SELINUX */
+
+#define SELINUX_CTX void *
+#define SEC_CTX void *
+
+#define SELINUX_context_new(x) NULL
+#define SELINUX_context_free(x) (x) = NULL
+#define SELINUX_context_dummy_get(x) "<SELinux not compiled in>"
+#define SELINUX_context_str SELINUX_context_dummy_get
+#define SELINUX_context_type_get SELINUX_context_dummy_get
+#define SELINUX_context_user_get SELINUX_context_dummy_get
+#define SELINUX_context_role_get SELINUX_context_dummy_get
+#define SELINUX_context_range_get SELINUX_context_dummy_get
+
+#include <errno.h>
+#define SELINUX_getpeercon(x, y) -1; do { \
+ *(y) = NULL; \
+ errno = ENOTSUP; \
+} while(0)
+
+#define SELINUX_freecon(x) (x) = NULL
+
+#endif /* done HAVE_SELINUX */
+
+#ifdef HAVE_UCRED
+#include <sys/socket.h>
+struct cli_creds {
+ struct ucred ucred;
+ SELINUX_CTX selinux_ctx;
+};
+
+#define cli_creds_get_uid(x) x->ucred.uid
+
+#else /* not HAVE_UCRED */
+struct cli_creds {
+ SELINUX_CTX selinux_ctx;
+};
+#define cli_creds_get_uid(x) -1
+#endif /* done HAVE_UCRED */
+
+#endif /* __SSSD_UTIL_CREDS_H__ */
--
2.5.0
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
