clvmd: verify messages before processing
RFC PATCH 1/1 clvmd: verify messages before processing
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(+)
daemons/clvmd/clvmd.c +109 -0
@@ -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
Signature · 1 line
1.8.3.1
RFC PATCH 1/1 clvmd: verify messages before processing
Quoted 5 lines · 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.
Mostly an ACK but see comment below:
Quoted 7 lines
daemons/clvmd/clvmd.c +2
-0@@ -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;
daemons/clvmd/clvmd.c error: · line 2301
Quoted 10 lines
daemons/clvmd/clvmd.c +7
-0inheader = (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
daemons/clvmd/clvmd.c error: · line 2307
Quoted 7 lines
daemons/clvmd/clvmd.c +3
-0+return ; + } +if (inheader->cmd ==CLVMD_CMD_REPLY )process_reply (inheader, len, csid);else
Chrissie
RFC PATCH 1/1 clvmd: verify messages before processing
Quoted 8 lines · Christine Caulfield
Quoted 8 lines
daemons/clvmd/clvmd.c +2
-0@@ -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;
Quoted 13 lines
daemons/clvmd/clvmd.c error: · line 2301
Quoted 9 lines
daemons/clvmd/clvmd.c +7
-0inheader = (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.
RFC PATCH 1/1 clvmd: verify messages before processing
Quoted 23 lines · David Teigland
Quoted 8 lines · Christine Caulfield
Quoted 8 lines
daemons/clvmd/clvmd.c +2
-0@@ -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;Quoted 13 lines
daemons/clvmd/clvmd.c error: · line 2301
Quoted 9 lines
daemons/clvmd/clvmd.c +7
-0inheader = (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
Quoted 8 lines
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