[PATCH RFC] clvmd: verify messages before processing

[PATCH RFC] clvmd: verify messages before processing

[David Teigland]

Does this look like it would be useful?  More checks can be
added over time, but are there any that would be obvious to
add now?  I'm mainly trying to catch non-message data, and
not risk rejecting any proper messages.


Check that fields in clvm_header are valid when
local or remote messages are received.  If not,
log an error, dump the message data and ignore
the message.
---
 daemons/clvmd/clvmd.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/daemons/clvmd/clvmd.c b/daemons/clvmd/clvmd.c
index a276cbc..468dc31 100644
--- a/daemons/clvmd/clvmd.c
+++ b/daemons/clvmd/clvmd.c
@@ -21,6 +21,7 @@
 
 #include <pthread.h>
 #include <getopt.h>
+#include <ctype.h>
 
 #include "clvmd-comms.h"
 #include "clvm.h"
@@ -1088,7 +1089,90 @@ static void be_daemon(int timeout)
 		log_error("Error setting current directory to /: %m");
 		exit(6);
 	}
+}
+
+static int verify_message(char *buf, int len)
+{
+	struct clvm_header *h = (struct clvm_header *)buf;
+
+	if (len < sizeof(struct clvm_header)) {
+		log_error("verify_message short len %d\n", len);
+		return -1;
+	}
+
+	switch (h->cmd) {
+	case CLVMD_CMD_REPLY:
+	case CLVMD_CMD_VERSION:
+	case CLVMD_CMD_GOAWAY:
+	case CLVMD_CMD_TEST:
+	case CLVMD_CMD_LOCK:
+	case CLVMD_CMD_UNLOCK:
+	case CLVMD_CMD_LOCK_LV:
+	case CLVMD_CMD_LOCK_VG:
+	case CLVMD_CMD_LOCK_QUERY:
+	case CLVMD_CMD_REFRESH:
+	case CLVMD_CMD_GET_CLUSTERNAME:
+	case CLVMD_CMD_SET_DEBUG:
+	case CLVMD_CMD_VG_BACKUP:
+	case CLVMD_CMD_RESTART:
+	case CLVMD_CMD_SYNC_NAMES:
+		break;
+	default:
+		log_error("verify_message bad cmd %x\n", h->cmd);
+		return -1;
+	};
+
+	/* TODO: we may be able to narrow len/flags/clientid/arglen checks based on cmd */
+
+	if (h->flags & ~(CLVMD_FLAG_LOCAL | CLVMD_FLAG_SYSTEMLV | CLVMD_FLAG_NODEERRS)) {
+		log_error("verify_message bad flags %x\n", h->flags);
+		return -1;
+	}
 
+	if (h->clientid < 0) {
+		log_error("verify_message bad clientid %x\n", h->clientid);
+		return -1;
+	}
+
+	if (h->arglen > max_cluster_message) {
+		log_error("verify_message bad arglen %x max %d\n", h->arglen, max_cluster_message);
+		return -1;
+	}
+
+	return 0;
+}
+
+static void dump_message(char *buf, int len)
+{
+	unsigned char row[8];
+	char str[9];
+	int i, j, pos;
+
+	if (len > 128)
+		len = 128;
+
+	pos = 0;
+	memset(row, 0, sizeof(row));
+
+	for (i = 0; i < len; i++) {
+		row[pos++] = buf[i];
+
+		if ((pos == 8) || (i + 1 == len)) {
+			memset(str, 0, sizeof(str));
+
+			for (j = 0; j < 8; j++) {
+				if (isprint(row[j]))
+					str[j] = row[j];
+				else
+					str[j] = ' ';
+			}
+
+			log_error("%02x %02x %02x %02x %02x %02x %02x %02x [%s]\n",
+				  row[0], row[1], row[2], row[3], row[4], row[5], row[6], row[7], str);
+			pos = 0;
+			memset(row, 0, sizeof(row));
+		}
+	}
 }
 
 /* Called when we have a read from the local socket.
@@ -1100,12 +1184,25 @@ static int read_from_local_sock(struct local_client *thisfd)
 	int missing_len;
 	char buffer[PIPE_BUF + 1];
 
+	memset(buffer, 0, sizeof(buffer));
+
 	len = read(thisfd->fd, buffer, sizeof(buffer) - 1);
 	if (len == -1 && errno == EINTR)
 		return 1;
 
 	DEBUGLOG("Read on local socket %d, len = %d\n", thisfd->fd, len);
 
+	if (len) {
+		int rv = verify_message(buffer, len);
+		if (rv < 0) {
+			log_error("read_from_local_sock from %d len %d bad verify\n",
+				  thisfd->fd, len);
+			dump_message(buffer, len);
+			/* force error handling below */
+			len = 0;
+		}
+	}
+
 	/* EOF or error on socket */
 	if (len <= 0) {
 		int *status;
@@ -2189,10 +2286,22 @@ error:
 void process_message(struct local_client *client, char *buf, int len,
 		     const char *csid)
 {
+	char nodename[max_cluster_member_name_len];
 	struct clvm_header *inheader;
+	int rv;
 
 	inheader = (struct clvm_header *) buf;
 	ntoh_clvm(inheader);	/* Byteswap fields */
+
+	rv = verify_message(buf, len);
+	if (rv < 0) {
+		memset(nodename, 0, sizeof(nodename));
+		clops->name_from_csid(csid, nodename);
+		log_error("process_message from %s len %d bad verify\n", nodename, len);
+		dump_message(buf, len);
+		return;
+	}
+
 	if (inheader->cmd == CLVMD_CMD_REPLY)
 		process_reply(inheader, len, csid);
 	else
-- 
1.8.3.1

[PATCH RFC] clvmd: verify messages before processing

[Christine Caulfield]

On 30/07/13 20:46, David Teigland wrote:
> Does this look like it would be useful?  More checks can be
> added over time, but are there any that would be obvious to
> add now?  I'm mainly trying to catch non-message data, and
> not risk rejecting any proper messages.
>

Mostly an ACK but see comment below:

> @@ -2189,10 +2286,22 @@ error:
>   void process_message(struct local_client *client, char *buf, int len,
>   		     const char *csid)
>   {
> +	char nodename[max_cluster_member_name_len];
>   	struct clvm_header *inheader;
> +	int rv;
>
>   	inheader = (struct clvm_header *) buf;
>   	ntoh_clvm(inheader);	/* Byteswap fields */
> +
> +	rv = verify_message(buf, len);
> +	if (rv < 0) {
> +		memset(nodename, 0, sizeof(nodename));
> +		clops->name_from_csid(csid, nodename);
> +		log_error("process_message from %s len %d bad verify\n", nodename, len);
> +		dump_message(buf, len);

Here you will probably have to return an error to the calling clvmd or 
you will get delays when the caller times out. If that happens then the 
'timed-out' message is less than helpful


> +		return;
> +	}
> +
>   	if (inheader->cmd == CLVMD_CMD_REPLY)
>   		process_reply(inheader, len, csid);
>   	else
>


Chrissie

[PATCH RFC] clvmd: verify messages before processing

[David Teigland]

On Wed, Jul 31, 2013 at 09:33:23AM +0100, Christine Caulfield wrote:
> >@@ -2189,10 +2286,22 @@ error:
> >  void process_message(struct local_client *client, char *buf, int len,
> >  		     const char *csid)
> >  {
> >+	char nodename[max_cluster_member_name_len];
> >  	struct clvm_header *inheader;
> >+	int rv;
> >
> >  	inheader = (struct clvm_header *) buf;
> >  	ntoh_clvm(inheader);	/* Byteswap fields */
> >+
> >+	rv = verify_message(buf, len);
> >+	if (rv < 0) {
> >+		memset(nodename, 0, sizeof(nodename));
> >+		clops->name_from_csid(csid, nodename);
> >+		log_error("process_message from %s len %d bad verify\n", nodename, len);
> >+		dump_message(buf, len);
> 
> Here you will probably have to return an error to the calling clvmd
> or you will get delays when the caller times out. If that happens
> then the 'timed-out' message is less than helpful

The thinking behind no reply was that if a command sends a bogus message,
it doesn't deserve a reply, and even if it got one, may not handle it
properly anyway.  So, if we're dealing with a bogus non-message, I think
we're beyond the point where we can be helpful.  I'm mainly trying to just
protect clvmd from faulty commands.  But, I looked at how feasible it
would be, and I don't think it's worth the trouble -- we'd have to do the
whole add_to_lvmqueue() thing, etc.

[PATCH RFC] clvmd: verify messages before processing

[Christine Caulfield]

On 31/07/13 16:31, David Teigland wrote:
> On Wed, Jul 31, 2013 at 09:33:23AM +0100, Christine Caulfield wrote:
>>> @@ -2189,10 +2286,22 @@ error:
>>>   void process_message(struct local_client *client, char *buf, int len,
>>>   		     const char *csid)
>>>   {
>>> +	char nodename[max_cluster_member_name_len];
>>>   	struct clvm_header *inheader;
>>> +	int rv;
>>>
>>>   	inheader = (struct clvm_header *) buf;
>>>   	ntoh_clvm(inheader);	/* Byteswap fields */
>>> +
>>> +	rv = verify_message(buf, len);
>>> +	if (rv < 0) {
>>> +		memset(nodename, 0, sizeof(nodename));
>>> +		clops->name_from_csid(csid, nodename);
>>> +		log_error("process_message from %s len %d bad verify\n", nodename, len);
>>> +		dump_message(buf, len);
>>
>> Here you will probably have to return an error to the calling clvmd
>> or you will get delays when the caller times out. If that happens
>> then the 'timed-out' message is less than helpful
>
> The thinking behind no reply was that if a command sends a bogus message,
> it doesn't deserve a reply, and even if it got one, may not handle it
> properly anyway.  So, if we're dealing with a bogus non-message, I think
> we're beyond the point where we can be helpful.  I'm mainly trying to just
> protect clvmd from faulty commands.  But, I looked at how feasible it
> would be, and I don't think it's worth the trouble -- we'd have to do the
> whole add_to_lvmqueue() thing, etc.
>

True it could be awkward and the worst that will happen is a minute's 
delay and an unhelpful message at the other end.

Chrissie