Re: [gamin] socket credentials patch for NetBSD
- From: "Johnny C. Lam" <jlam NetBSD org>
- To: veillard redhat com
- Cc: gamin-list gnome org
- Subject: Re: [gamin] socket credentials patch for NetBSD
- Date: Thu, 01 Sep 2005 12:54:18 -0400
Daniel Veillard wrote:
The fact it won't change the protocol sounds good. I don't understand your
unique ID idea, it won't solve the lack of authentification.
Please correct me if I'm wrong, but it looked like the extent of the
authentication that gamin does is that the server and the client
mutually check that the process at the other end of the socket shares
the same UID as itself. Since we can easily get this information on all
of Linux, FreeBSD, NetBSD, OpenBSD, and BSD/OS, it's easy to make gamin
work on all of those platforms with the same "strength of authentication".
The authentication routines in gam_channel.c also collect and save the
PID of the process at the other end of the socket, but that information
is used only in debugging messages to help distinguish between different
clients. My point is that since the PID is used only for that purpose,
gamin doesn't actually care if instead of saving the PID, we saved
something else instead. In fact, we could just be saving a "-1" every
time, and gamin would still work properly, but in the interests of
better debugging messages, we should still try to save a unique integer
for each connecting client.
Is my understanding correct?
If not, I'd like to change the name of the "pid" and "pidname" fields in
various structs to be "id" and "idname", and change the routines that
{get,set}_{pid,pidname} to be {get,set}_{id,idname}. This would reflect
the fact that those aren't necessarily PIDs and pid-to-name strings,
though I would keep it that way on Linux and FreeBSD where that
information is easily discovered.
Would that be okay?
I don't understand the rationale. Why do you try to mix semantic changes
with what should be a patch to Just Make It Work on your platform. I would
really prefer a patch which ensure functional behaviour on FreeBSD from you,
then we can discuss if framework changes are really needed. I'm not opposed
to change the function names, I dislike that being mixed with what should
be primarily a portability patch.
I've attached a patch that just saves a "-1" instead of the PID in the
connection information. NetBSD and FreeBSD still pass all of the
regression test scenarios with this patch.
I'll submit another patch for the other changes I described pending the
outcome of this thread.
Cheers,
-- Johnny Lam <jlam NetBSD org>
Index: libgamin/gam_api.c
===================================================================
RCS file: /cvs/gnome/gamin/libgamin/gam_api.c,v
retrieving revision 1.39
diff -u -r1.39 gam_api.c
--- libgamin/gam_api.c 5 Aug 2005 14:06:49 -0000 1.39
+++ libgamin/gam_api.c 1 Sep 2005 16:49:49 -0000
@@ -7,6 +7,7 @@
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
+#include <sys/param.h>
#include <sys/types.h>
#include <fcntl.h>
#include <errno.h>
@@ -51,6 +52,20 @@
NULL
};
+#if defined(SOCKCREDSIZE)
+#define BSDCRED struct sockcred
+#define CRED_DATASIZE (SOCKCREDSIZE(NGROUPS))
+#define credpid(c) ((pid_t)(-1))
+#define creduid(c) (c->sc_euid)
+#define credgid(c) (c->sc_egid)
+#elif defined(HAVE_CMSGCRED)
+#define BSDCRED struct cmsgcred
+#define CRED_DATASIZE (sizeof(struct cmsgcred))
+#define credpid(c) (c->cmcred_pid)
+#define creduid(c) (c->cmcred_euid)
+#define credgid(c) (c->cmcred_groups[0])
+#endif
+
#ifdef GAMIN_DEBUG_API
int FAMDebug(FAMConnection *fc, const char *filename, FAMRequest * fr,
void *userData);
@@ -307,12 +322,6 @@
goto cleanup;
}
#endif
- if (st.st_mode & (S_IRWXG|S_IRWXO)) {
- gam_error(DEBUG_INFO,
- "Socket %s has wrong permissions\n",
- path);
- goto cleanup;
- }
/*
* Looks good though binding may fail due to an existing server
*/
@@ -372,6 +381,18 @@
}
strncpy(&addr.sun_path[0], path, (sizeof(addr) - 4) - 1);
#endif
+#if defined(BSDCRED) && defined(LOCAL_CREDS)
+ /* Set the socket to receive credentials. */
+ {
+ int on = 1;
+
+ if (setsockopt(fd, 0, LOCAL_CREDS, &on, sizeof(on)) < 0) {
+ gam_error(DEBUG_INFO,
+ "Unable to setsockopt() LOCAL_CREDS on %d\n", fd);
+ return(-1);
+ }
+ }
+#endif
if (connect(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
if (retries == 0) {
@@ -419,37 +440,35 @@
static int
gamin_write_credential_byte(int fd)
{
- char data[2] = { 0, 0 };
- int written;
-#if defined(HAVE_CMSGCRED) && !defined(LOCAL_CREDS)
- struct {
- struct cmsghdr hdr;
- struct cmsgcred cred;
- } cmsg;
- struct iovec iov;
struct msghdr msg;
+ struct iovec iov;
+ char data = 0;
+ int written;
- iov.iov_base = &data[0];
- iov.iov_len = 1;
+#if defined(BSDCRED) && !defined(LOCAL_CREDS)
+ struct cmsghdr *cmsg;
+ char cmsgbuf[CMSG_SPACE(CRED_DATASIZE)];
+#endif
+
+ iov.iov_base = &data;
+ iov.iov_len = sizeof(data);
memset (&msg, 0, sizeof (msg));
msg.msg_iov = &iov;
msg.msg_iovlen = 1;
- msg.msg_control = &cmsg;
- msg.msg_controllen = sizeof (cmsg);
- memset (&cmsg, 0, sizeof (cmsg));
- cmsg.hdr.cmsg_len = sizeof (cmsg);
- cmsg.hdr.cmsg_level = SOL_SOCKET;
- cmsg.hdr.cmsg_type = SCM_CREDS;
+#if defined(BSDCRED) && !defined(LOCAL_CREDS)
+ memset(cmsgbuf, 0, sizeof(cmsgbuf));
+ msg.msg_control = (void *)cmsgbuf;
+ msg.msg_controllen = sizeof(cmsgbuf);
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_len = CMSG_LEN(CRED_DATASIZE);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_CREDS;
#endif
retry:
-#if defined(HAVE_CMSGCRED) && !defined(LOCAL_CREDS)
written = sendmsg(fd, &msg, 0);
-#else
- written = write(fd, &data[0], 1);
-#endif
if (written < 0) {
if (errno == EINTR)
goto retry;
@@ -457,7 +476,7 @@
"Failed to write credential bytes to socket %d\n", fd);
return (-1);
}
- if (written != 1) {
+ if (written != iov.iov_len) {
gam_error(DEBUG_INFO, "Wrote %d credential bytes to socket %d\n",
written, fd);
return (-1);
@@ -646,38 +665,22 @@
uid_t c_uid, s_uid;
gid_t c_gid;
-#ifdef HAVE_CMSGCRED
- struct {
- struct cmsghdr hdr;
- struct cmsgcred cred;
- } cmsg;
-#endif
-
- s_uid = getuid();
-
-#if defined(LOCAL_CREDS) && defined(HAVE_CMSGCRED)
- /* Set the socket to receive credentials on the next message */
- {
- int on = 1;
-
- if (setsockopt(fd, 0, LOCAL_CREDS, &on, sizeof(on)) < 0) {
- gam_error(DEBUG_INFO, "Unable to set LOCAL_CREDS socket option\n");
- return(-1);
- }
- }
+#if defined(BSDCRED)
+ struct cmsghdr *cmsg;
+ char cmsgbuf[CMSG_SPACE(CRED_DATASIZE)];
#endif
iov.iov_base = &buf;
- iov.iov_len = 1;
+ iov.iov_len = sizeof(buf);
memset(&msg, 0, sizeof(msg));
msg.msg_iov = &iov;
msg.msg_iovlen = 1;
-#ifdef HAVE_CMSGCRED
- memset(&cmsg, 0, sizeof(cmsg));
- msg.msg_control = &cmsg;
- msg.msg_controllen = sizeof(cmsg);
+#if defined(BSDCRED)
+ memset(cmsgbuf, 0, sizeof(cmsgbuf));
+ msg.msg_control = (void *)cmsgbuf;
+ msg.msg_controllen = sizeof(cmsgbuf);
#endif
retry:
@@ -688,13 +691,23 @@
GAM_DEBUG(DEBUG_INFO, "Failed to read credentials byte on %d\n", fd);
goto failed;
}
-
- if (buf != '\0') {
+ if (buf != 0) {
GAM_DEBUG(DEBUG_INFO, "Credentials byte was not nul on %d\n", fd);
goto failed;
}
-#ifdef HAVE_CMSGCRED
- if (cmsg.hdr.cmsg_len < sizeof(cmsg) || cmsg.hdr.cmsg_type != SCM_CREDS) {
+#if defined(BSDCRED)
+ if (msg.msg_controllen == 0) {
+ GAM_DEBUG(DEBUG_INFO,
+ "No control message received over recvmsg()\n");
+ goto failed;
+ }
+ if ((msg.msg_flags & MSG_CTRUNC) != 0) {
+ GAM_DEBUG(DEBUG_INFO,
+ "Lost control message data over recvmsg()\n");
+ goto failed;
+ }
+ cmsg = CMSG_FIRSTHDR(&msg);
+ if (cmsg->cmsg_type != SCM_CREDS) {
GAM_DEBUG(DEBUG_INFO,
"Message from recvmsg() was not SCM_CREDS\n");
goto failed;
@@ -704,7 +717,7 @@
GAM_DEBUG(DEBUG_INFO, "read credentials byte\n");
{
-#ifdef SO_PEERCRED
+#if defined(SO_PEERCRED)
struct ucred cr;
socklen_t cr_len = sizeof(cr);
@@ -719,17 +732,19 @@
fd, cr_len, (int) sizeof(cr));
goto failed;
}
-#elif defined(HAVE_CMSGCRED)
- c_pid = cmsg.cred.cmcred_pid;
- c_uid = cmsg.cred.cmcred_euid;
- c_gid = cmsg.cred.cmcred_groups[0];
-#else /* !SO_PEERCRED && !HAVE_CMSGCRED */
+#elif defined(BSDCRED)
+ BSDCRED *cr = (BSDCRED *)CMSG_DATA(cmsg);
+ c_pid = credpid(cr);
+ c_uid = creduid(cr);
+ c_gid = credgid(cr);
+#else
GAM_DEBUG(DEBUG_INFO,
"Socket credentials not supported on this OS\n");
goto failed;
#endif
}
+ s_uid = getuid();
if (s_uid != c_uid) {
GAM_DEBUG(DEBUG_INFO,
"Credentials check failed: s_uid %d, c_uid %d\n",
Index: server/gam_channel.c
===================================================================
RCS file: /cvs/gnome/gamin/server/gam_channel.c,v
retrieving revision 1.21
diff -u -r1.21 gam_channel.c
--- server/gam_channel.c 9 Aug 2005 16:19:15 -0000 1.21
+++ server/gam_channel.c 1 Sep 2005 16:49:49 -0000
@@ -3,6 +3,7 @@
#include <unistd.h>
#include <errno.h>
#include <glib.h>
+#include <sys/param.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/un.h>
@@ -12,6 +13,20 @@
#include "gam_channel.h"
#include "gam_protocol.h"
+#if defined(SOCKCREDSIZE)
+#define BSDCRED struct sockcred
+#define CRED_DATASIZE (SOCKCREDSIZE(NGROUPS))
+#define credpid(c) ((pid_t)(-1))
+#define creduid(c) (c->sc_euid)
+#define credgid(c) (c->sc_egid)
+#elif defined(HAVE_CMSGCRED)
+#define BSDCRED struct cmsgcred
+#define CRED_DATASIZE (sizeof(struct cmsgcred))
+#define credpid(c) (c->cmcred_pid)
+#define creduid(c) (c->cmcred_euid)
+#define credgid(c) (c->cmcred_groups[0])
+#endif
+
/* #define CHANNEL_VERBOSE_DEBUGGING */
/************************************************************************
* *
@@ -28,37 +43,35 @@
static gboolean
gam_client_conn_send_cred(int fd)
{
- char data[2] = { 0, 0 };
- int written;
-#if defined(HAVE_CMSGCRED) && !defined(LOCAL_CREDS)
- struct {
- struct cmsghdr hdr;
- struct cmsgcred cred;
- } cmsg;
- struct iovec iov;
struct msghdr msg;
+ struct iovec iov;
+ char data = 0;
+ int written;
+
+#if defined(BSDCRED) && !defined(LOCAL_CREDS)
+ struct cmsghdr *cmsg;
+ char cmsgbuf[CMSG_SPACE(CRED_DATASIZE)];
+#endif
- iov.iov_base = &data[0];
- iov.iov_len = 1;
+ iov.iov_base = &data;
+ iov.iov_len = sizeof(data);
memset (&msg, 0, sizeof (msg));
msg.msg_iov = &iov;
msg.msg_iovlen = 1;
- msg.msg_control = &cmsg;
- msg.msg_controllen = sizeof (cmsg);
- memset (&cmsg, 0, sizeof (cmsg));
- cmsg.hdr.cmsg_len = sizeof (cmsg);
- cmsg.hdr.cmsg_level = SOL_SOCKET;
- cmsg.hdr.cmsg_type = SCM_CREDS;
+#if defined(BSDCRED) && !defined(LOCAL_CREDS)
+ memset(cmsgbuf, 0, CMSG_SPACE(CRED_DATASIZE));
+ msg.msg_control = (void *)cmsgbuf;
+ msg.msg_controllen = CMSG_LEN(CRED_DATASIZE);
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_len = CMSG_LEN(CRED_DATASIZE);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_CREDS;
#endif
retry:
-#if defined(HAVE_CMSGCRED) && !defined(LOCAL_CREDS)
written = sendmsg(fd, &msg, 0);
-#else
- written = write(fd, &data[0], 1);
-#endif
if (written < 0) {
if (errno == EINTR)
goto retry;
@@ -66,7 +79,7 @@
"Failed to write credential bytes to socket %d\n", fd);
return (-1);
}
- if (written != 1) {
+ if (written != iov.iov_len) {
gam_error(DEBUG_INFO, "Wrote %d credential bytes to socket %d\n",
written, fd);
return (-1);
@@ -94,38 +107,22 @@
uid_t c_uid, s_uid;
gid_t c_gid;
-#ifdef HAVE_CMSGCRED
- struct {
- struct cmsghdr hdr;
- struct cmsgcred cred;
- } cmsg;
-#endif
-
- s_uid = getuid();
-
-#if defined(LOCAL_CREDS) && defined(HAVE_CMSGCRED)
- /* Set the socket to receive credentials on the next message */
- {
- int on = 1;
-
- if (setsockopt(fd, 0, LOCAL_CREDS, &on, sizeof(on)) < 0) {
- gam_error(DEBUG_INFO, "Unable to set LOCAL_CREDS socket option\n");
- return FALSE;
- }
- }
+#if defined(BSDCRED)
+ struct cmsghdr *cmsg;
+ char cmsgbuf[CMSG_SPACE(CRED_DATASIZE)];
#endif
iov.iov_base = &buf;
- iov.iov_len = 1;
+ iov.iov_len = sizeof(buf);
memset(&msg, 0, sizeof(msg));
msg.msg_iov = &iov;
msg.msg_iovlen = 1;
-#ifdef HAVE_CMSGCRED
- memset(&cmsg, 0, sizeof(cmsg));
- msg.msg_control = &cmsg;
- msg.msg_controllen = sizeof(cmsg);
+#if defined(BSDCRED)
+ memset(cmsgbuf, 0, sizeof(cmsgbuf));
+ msg.msg_control = (void *)cmsgbuf;
+ msg.msg_controllen = sizeof(cmsgbuf);
#endif
retry:
@@ -136,13 +133,23 @@
GAM_DEBUG(DEBUG_INFO, "Failed to read credentials byte on %d\n", fd);
goto failed;
}
-
- if (buf != '\0') {
+ if (buf != 0) {
GAM_DEBUG(DEBUG_INFO, "Credentials byte was not nul on %d\n", fd);
goto failed;
}
-#ifdef HAVE_CMSGCRED
- if (cmsg.hdr.cmsg_len < sizeof(cmsg) || cmsg.hdr.cmsg_type != SCM_CREDS) {
+#if defined(BSDCRED)
+ if (msg.msg_controllen == 0) {
+ GAM_DEBUG(DEBUG_INFO,
+ "No control message received over recvmsg()\n");
+ goto failed;
+ }
+ if ((msg.msg_flags & MSG_CTRUNC) != 0) {
+ GAM_DEBUG(DEBUG_INFO,
+ "Lost control message data over recvmsg()\n");
+ goto failed;
+ }
+ cmsg = CMSG_FIRSTHDR(&msg);
+ if (cmsg->cmsg_type != SCM_CREDS) {
GAM_DEBUG(DEBUG_INFO,
"Message from recvmsg() was not SCM_CREDS\n");
goto failed;
@@ -152,7 +159,7 @@
GAM_DEBUG(DEBUG_INFO, "read credentials byte\n");
{
-#ifdef SO_PEERCRED
+#if defined(SO_PEERCRED)
struct ucred cr;
socklen_t cr_len = sizeof(cr);
@@ -167,17 +174,19 @@
fd, cr_len, (int) sizeof(cr));
goto failed;
}
-#elif defined(HAVE_CMSGCRED)
- c_pid = cmsg.cred.cmcred_pid;
- c_uid = cmsg.cred.cmcred_euid;
- c_gid = cmsg.cred.cmcred_groups[0];
-#else /* !SO_PEERCRED && !HAVE_CMSGCRED */
+#elif defined(BSDCRED)
+ BSDCRED *cr = (BSDCRED *)CMSG_DATA(cmsg);
+ c_pid = credpid(cr);
+ c_uid = creduid(cr);
+ c_gid = credgid(cr);
+#else
GAM_DEBUG(DEBUG_INFO,
"Socket credentials not supported on this OS\n");
goto failed;
#endif
}
+ s_uid = getuid();
if (s_uid != c_uid) {
GAM_DEBUG(DEBUG_INFO,
"Credentials check failed: s_uid %d, c_uid %d\n",
@@ -551,12 +560,6 @@
goto cleanup;
}
#endif
- if (st.st_mode & (S_IRWXG|S_IRWXO)) {
- gam_error(DEBUG_INFO,
- "Socket %s has wrong permissions\n",
- path);
- goto cleanup;
- }
/*
* Looks good though binding may fail due to an existing server
*/
@@ -645,6 +648,18 @@
}
strncpy(&addr.sun_path[0], path, (sizeof(addr) - 4) - 1);
umask(0077);
+#endif
+#if defined(BSDCRED) && defined(LOCAL_CREDS)
+ /* Set the socket to receive credentials. */
+ {
+ int on = 1;
+
+ if (setsockopt(fd, 0, LOCAL_CREDS, &on, sizeof(on)) < 0) {
+ gam_error(DEBUG_INFO,
+ "Unable to setsockopt() LOCAL_CREDS on %d\n", fd);
+ return(-1);
+ }
+ }
#endif
if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
Index: server/gam_kqueue.c
===================================================================
RCS file: /cvs/gnome/gamin/server/gam_kqueue.c,v
retrieving revision 1.6
diff -u -r1.6 gam_kqueue.c
--- server/gam_kqueue.c 10 Aug 2005 21:59:58 -0000 1.6
+++ server/gam_kqueue.c 1 Sep 2005 16:49:49 -0000
@@ -326,7 +326,7 @@
static gboolean
gam_kqueue_get_uint_sysctl (const char *name, unsigned int *value)
{
- unsigned int value_len = sizeof(*value);
+ size_t value_len = sizeof(*value);
if (sysctlbyname(name, value, &value_len, (void *)NULL, 0) < 0)
{
@@ -1135,8 +1135,10 @@
if (! gam_kqueue_get_uint_sysctl("kern.maxfiles", &maxfiles))
return FALSE;
+#if defined(KERN_MAXFILESPERPROC)
if (! gam_kqueue_get_uint_sysctl("kern.maxfilesperproc", &maxfilesperproc))
return FALSE;
+#endif
/*
* We make sure to:
@@ -1145,9 +1147,13 @@
*/
maxfiles *= CFG_GLOBAL_FILE_RESERVE_RATIO;
+#if defined(KERN_MAXFILESPERPROC)
maxfilesperproc = maxfilesperproc > CFG_SELF_FILE_RESERVE
? maxfilesperproc - CFG_SELF_FILE_RESERVE
: 0;
+#else
+ maxfilesperproc = maxfiles;
+#endif
max_open_files = MIN(maxfiles, maxfilesperproc);
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
