[PATCH] ipcc: Check that ipc socket is correctly connected

[Jan Friesse <jfriesse@xxxxxxxxxx>]

If client application closes fd returned by corosync ipc, various
strange things can happen. Easiest way how to prevent such situation is
to properly check if fd is valid.

Signed-off-by: Jan Friesse <jfriesse@xxxxxxxxxx>
---
 lib/coroipcc.c |   90 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 82 insertions(+), 8 deletions(-)

diff --git a/lib/coroipcc.c b/lib/coroipcc.c
index 3ad4921..36d4fb0 100644
--- a/lib/coroipcc.c
+++ b/lib/coroipcc.c
@@ -84,6 +84,8 @@ struct ipc_instance {
 	size_t dispatch_size;
 	uid_t euid;
 	pthread_mutex_t mutex;
+	struct sockaddr_un address;
+	size_t address_path_len;
 };
 
 struct ipc_path_data {
@@ -96,6 +98,7 @@ struct ipc_path_data {
 };
 
 void ipc_hdb_destructor (void *context);
+static int is_valid_ipc_fd(struct ipc_instance *ipc_instance);
 
 DECLARE_HDB_DATABASE(ipc_hdb,ipc_hdb_destructor);
 
@@ -268,6 +271,11 @@ priv_change_send (struct ipc_instance *ipc_instance)
 	if (ipc_instance->euid == req_priv_change.euid) {
 		return (0);
 	}
+
+	if (!is_valid_ipc_fd(ipc_instance->fd)) {
+		return (CS_ERR_BAD_HANDLE);
+	}
+
 	req_priv_change.egid = getegid();
 
 	buf_req = MESSAGE_REQ_CHANGE_EUID;
@@ -526,6 +534,10 @@ reply_receive (
 	coroipc_response_header_t *response_header;
 	cs_error_t res;
 
+	if (!is_valid_ipc_fd(ipc_instance)) {
+		return (CS_ERR_BAD_HANDLE);
+	}
+
 retry_ipc_sem_wait:
 	res = ipc_sem_wait (ipc_instance->control_buffer, SEMAPHORE_RESPONSE, ipc_instance->fd);
 	if (res != CS_OK) {
@@ -553,6 +565,10 @@ reply_receive_in_buf (
 {
 	cs_error_t res;
 
+	if (!is_valid_ipc_fd(ipc_instance)) {
+		return (CS_ERR_BAD_HANDLE);
+	}
+
 retry_ipc_sem_wait:
 	res = ipc_sem_wait (ipc_instance->control_buffer, SEMAPHORE_RESPONSE, ipc_instance->fd);
 	if (res != CS_OK) {
@@ -569,6 +585,43 @@ retry_ipc_sem_wait:
 }
 
 /*
+ * Check that passed fd is valid socket and it's peer is really corosync socket
+ */
+static int
+is_valid_ipc_fd(
+	struct ipc_instance *ipc_instance)
+{
+	struct sockaddr_un peer_addr;
+	socklen_t peer_len;
+	int res;
+
+	memset(&peer_addr, 0, sizeof(peer_addr));
+	peer_len = sizeof(peer_addr);
+	res = getpeername(ipc_instance->fd, &peer_addr, &peer_len);
+	if (res != 0) {
+		/*
+		 * It's not valid socket
+		 */
+		return (0);
+	}
+	if (peer_addr.sun_family != AF_UNIX) {
+		/*
+		 * It's socket but not unix one
+		 */
+		return (0);
+	}
+
+	if (memcmp(ipc_instance->address.sun_path, peer_addr.sun_path, ipc_instance->address_path_len) != 0) {
+		/*
+		 * Path is different then expected
+		 */
+		return (0);
+	}
+
+	return (1);
+}
+
+/*
  * External API
  */
 cs_error_t
@@ -582,7 +635,6 @@ coroipcc_service_connect (
 
 {
 	int request_fd;
-	struct sockaddr_un address;
 	cs_error_t res;
 	struct ipc_instance *ipc_instance;
 #if _POSIX_THREAD_PROCESS_SHARED < 1
@@ -623,19 +675,21 @@ coroipcc_service_connect (
 
 	path_data->res_setup.error = CS_ERR_LIBRARY;
 
-	memset (&address, 0, sizeof (struct sockaddr_un));
-	address.sun_family = AF_UNIX;
+	memset (&ipc_instance->address, 0, sizeof (struct sockaddr_un));
+	ipc_instance->address.sun_family = AF_UNIX;
 #if defined(COROSYNC_BSD) || defined(COROSYNC_DARWIN)
-	address.sun_len = SUN_LEN(&address);
+	ipc_instance->address.sun_len = SUN_LEN(&ipc_instance->address);
 #endif
+	ipc_instance->address_path_len = 0;
 
 #if defined(COROSYNC_LINUX)
-	sprintf (address.sun_path + 1, "%s", socket_name);
+	ipc_instance->address_path_len = sprintf (ipc_instance->address.sun_path + 1, "%s", socket_name);
+	ipc_instance->address_path_len++;
 #else
-	sprintf (address.sun_path, "%s/%s", SOCKETDIR, socket_name);
+	ipc_instance->address_path_len = sprintf (ipc_instance->address.sun_path, "%s/%s", SOCKETDIR, socket_name);
 #endif
-	sys_res = connect (request_fd, (struct sockaddr *)&address,
-		COROSYNC_SUN_LEN(&address));
+	sys_res = connect (request_fd, (struct sockaddr *)&ipc_instance->address,
+		COROSYNC_SUN_LEN(&ipc_instance->address));
 	if (sys_res == -1) {
 		res = CS_ERR_TRY_AGAIN;
 		goto error_connect;
@@ -808,6 +862,10 @@ coroipcc_service_disconnect (
 		return (res);
 	}
 
+	if (!is_valid_ipc_fd(ipc_instance)) {
+		return (CS_ERR_BAD_HANDLE);
+	}
+
 	shutdown (ipc_instance->fd, SHUT_RDWR);
 	close (ipc_instance->fd);
 	hdb_handle_destroy (&ipc_hdb, handle);
@@ -847,6 +905,10 @@ coroipcc_fd_get (
 		return (res);
 	}
 
+	if (!is_valid_ipc_fd(ipc_instance)) {
+		return (CS_ERR_BAD_HANDLE);
+	}
+
 	*fd = ipc_instance->fd;
 
 	hdb_handle_put (&ipc_hdb, handle);
@@ -885,6 +947,10 @@ coroipcc_dispatch_get (
 
 	*data = NULL;
 
+	if (!is_valid_ipc_fd(ipc_instance)) {
+		return (CS_ERR_BAD_HANDLE);
+	}
+
 	ufds.fd = ipc_instance->fd;
 	ufds.events = POLLIN;
 	ufds.revents = 0;
@@ -907,6 +973,10 @@ coroipcc_dispatch_get (
 		goto error_put;
 	}
 
+	if (!is_valid_ipc_fd(ipc_instance)) {
+		return (CS_ERR_BAD_HANDLE);
+	}
+
 	error = socket_recv (ipc_instance->fd, &buf, 1);
 	if (error != CS_OK) {
 		goto error_put;
@@ -952,6 +1022,10 @@ coroipcc_dispatch_put (hdb_handle_t handle)
 		return (res);
 	}
 
+	if (!is_valid_ipc_fd(ipc_instance)) {
+		return (CS_ERR_BAD_HANDLE);
+	}
+
 retry_ipc_sem_wait:
 	res = ipc_sem_wait (ipc_instance->control_buffer, SEMAPHORE_DISPATCH, ipc_instance->fd);
 	if (res != CS_OK) {
-- 
1.7.1

_______________________________________________
discuss mailing list
discuss@xxxxxxxxxxxx
http://lists.corosync.org/mailman/listinfo/discuss