Merge pull request #211 from acplt/hardening_client

Make the client robust against message fragmentation (#166 remains an issue for the server)

CMakeLists.txt

@@ -51,9 +51,9 @@ set(exported_headers ${PROJECT_BINARY_DIR}/src_generated/ua_config.h
 set(internal_headers ${PROJECT_SOURCE_DIR}/src/ua_util.h
                      ${PROJECT_SOURCE_DIR}/deps/queue.h
                      ${PROJECT_BINARY_DIR}/src_generated/ua_transport_generated.h
+                     ${PROJECT_SOURCE_DIR}/src/ua_types_encoding_binary.h
                      ${PROJECT_SOURCE_DIR}/src/ua_securechannel.h
                      ${PROJECT_SOURCE_DIR}/src/ua_session.h
-                     ${PROJECT_SOURCE_DIR}/src/ua_types_encoding_binary.h
                      ${PROJECT_SOURCE_DIR}/src/server/ua_nodes.h
                      ${PROJECT_SOURCE_DIR}/src/server/ua_nodestore.h
                      ${PROJECT_SOURCE_DIR}/src/server/ua_session_manager.h

examples/networklayer_tcp.c

@@ -341,7 +341,7 @@ static UA_Int32 ServerNetworkLayerTCP_getWork(ServerNetworkLayerTCP *layer, UA_W
 		struct sockaddr_in cli_addr;
 		socklen_t cli_len = sizeof(cli_addr);
 		int newsockfd = accept(layer->serversockfd, (struct sockaddr *) &cli_addr, &cli_len);
-		int i = 1;
+		int i = 1;
 		setsockopt(newsockfd, IPPROTO_TCP, TCP_NODELAY, (void *)&i, sizeof(i));
 		if (newsockfd >= 0)
 			ServerNetworkLayerTCP_add(layer, newsockfd);
@@ -568,26 +568,16 @@ static UA_StatusCode ClientNetworkLayerTCP_send(ClientNetworkLayerTCP *handle, U
     return UA_STATUSCODE_GOOD;
 }
 
-static UA_StatusCode ClientNetworkLayerTCP_awaitResponse(ClientNetworkLayerTCP *handle, UA_ByteString *response,
-                                                         UA_UInt32 timeout) {
-    //FD_ZERO(&handle->read_fds);
-    //FD_SET(handle->sockfd, &handle->read_fds);//tcp socket
+static UA_StatusCode
+ClientNetworkLayerTCP_awaitResponse(ClientNetworkLayerTCP *handle, UA_ByteString *response,
+                                    UA_UInt32 timeout) {
     struct timeval tmptv = {0, timeout};
-    /*int ret = select(handle->sockfd+1, &handle->read_fds, NULL, NULL, &tmptv);
-    if(ret <= -1)
-        return UA_STATUSCODE_BADINTERNALERROR;
-    if(ret == 0)
-        return UA_STATUSCODE_BADTIMEOUT;*/
-
     setsockopt(handle->sockfd, SOL_SOCKET, SO_RCVTIMEO, (char *)&tmptv,sizeof(struct timeval));
-
     int ret = recv(handle->sockfd, (char*)response->data, response->length, 0);
-
     if(ret <= -1)
         return UA_STATUSCODE_BADINTERNALERROR;
     if(ret == 0)
         return UA_STATUSCODE_BADSERVERNOTCONNECTED;
-
     response->length = ret;
     return UA_STATUSCODE_GOOD;
 }

include/ua_connection.h

@@ -62,11 +62,16 @@ typedef struct UA_Connection {
     UA_SecureChannel   *channel;
     void (*write)(void *connection, UA_ByteStringArray buf);
     void (*close)(void *connection);
+    UA_ByteString incompleteMessage;
 } UA_Connection;
 
 void UA_EXPORT UA_Connection_detachSecureChannel(UA_Connection *connection);
 // void UA_Connection_attachSecureChannel(UA_Connection *connection);
 
+/** Returns a string of complete message (the length entry is decoded for that).
+    If the received message is incomplete, it is retained in the connection. */
+UA_ByteString UA_EXPORT UA_Connection_completeMessages(UA_Connection *connection, UA_ByteString received);
+
 /** @} */
 
 #ifdef __cplusplus

src/client/ua_client.c

@@ -5,6 +5,7 @@
 #include "ua_transport_generated.h"
 
 struct UA_Client {
+    /* Connection */
     UA_ClientNetworkLayer networkLayer;
     UA_String endpointUrl;
     UA_Connection connection;
@@ -30,6 +31,7 @@ UA_Client * UA_Client_new(void) {
         return UA_NULL;
     UA_String_init(&client->endpointUrl);
     client->connection.state = UA_CONNECTION_OPENING;
+    UA_ByteString_init(&client->connection.incompleteMessage);
 
     client->sequenceNumber = 0;
     client->requestId = 0;
@@ -70,8 +72,8 @@ static UA_StatusCode HelAckHandshake(UA_Client *c) {
 	hello.receiveBufferSize = conn->localConf.recvBufferSize;
 	hello.sendBufferSize = conn->localConf.sendBufferSize;
 
-	messageHeader.messageSize = UA_TcpHelloMessage_calcSizeBinary((UA_TcpHelloMessage const*) &hello) +
-                                UA_TcpMessageHeader_calcSizeBinary((UA_TcpMessageHeader const*) &messageHeader);
+	messageHeader.messageSize = UA_TcpHelloMessage_calcSizeBinary((UA_TcpHelloMessage const*)&hello) +
+                                UA_TcpMessageHeader_calcSizeBinary((UA_TcpMessageHeader const*)&messageHeader);
 	UA_ByteString message;
     message.data = UA_alloca(messageHeader.messageSize);
     message.length = messageHeader.messageSize;
@@ -171,7 +173,15 @@ static UA_StatusCode SecureChannelHandshake(UA_Client *client) {
     // parse the response
     UA_ByteString reply;
     UA_ByteString_newMembers(&reply, client->connection.localConf.recvBufferSize);
-    retval = client->networkLayer.awaitResponse(client->networkLayer.nlHandle, &reply, 1000);
+    do {
+        retval = client->networkLayer.awaitResponse(client->networkLayer.nlHandle, &reply, 1000);
+        if(retval != UA_STATUSCODE_GOOD) {
+            UA_ByteString_deleteMembers(&reply);
+            return retval;
+        }
+        reply = UA_Connection_completeMessages(&client->connection, reply);
+    } while(reply.length < 0);
+
     if(retval) {
         UA_ByteString_deleteMembers(&reply);
         return retval;
@@ -214,6 +224,9 @@ static void sendReceiveRequest(UA_RequestHeader *request, const UA_DataType *req
                                void *response, const UA_DataType *responseType, UA_Client *client,
                                UA_Boolean sendOnly)
 {
+	if(response)
+		UA_init(response, responseType);
+
     UA_NodeId_copy(&client->authenticationToken, &request->authenticationToken);
 
     UA_SecureConversationMessageHeader msgHeader;
@@ -270,13 +283,16 @@ static void sendReceiveRequest(UA_RequestHeader *request, const UA_DataType *req
 
     /* Response */
     UA_ByteString reply;
-    UA_ByteString_newMembers(&reply, client->connection.localConf.recvBufferSize);
-    retval = client->networkLayer.awaitResponse(client->networkLayer.nlHandle, &reply, 1000);
-    if(retval != UA_STATUSCODE_GOOD) {
-        UA_ByteString_deleteMembers(&reply);
-        respHeader->serviceResult = retval;
-        return;
-    }
+    do {
+        UA_ByteString_newMembers(&reply, client->connection.localConf.recvBufferSize);
+        retval = client->networkLayer.awaitResponse(client->networkLayer.nlHandle, &reply, 1000);
+        if(retval != UA_STATUSCODE_GOOD) {
+            UA_ByteString_deleteMembers(&reply);
+            respHeader->serviceResult = retval;
+            return;
+        }
+        reply = UA_Connection_completeMessages(&client->connection, reply);
+    } while(reply.length < 0);
 
 	offset = 0;
 	retval |= UA_SecureConversationMessageHeader_decodeBinary(&reply, &offset, &msgHeader);
@@ -342,6 +358,7 @@ static UA_StatusCode SessionHandshake(UA_Client *client) {
 	/* UA_String_copycstring("abcd", &rq.clientCertificate); */
 
     UA_CreateSessionResponse response;
+    UA_CreateSessionResponse_init(&response);
     synchronousRequest(&request, &UA_TYPES[UA_TYPES_CREATESESSIONREQUEST],
                        &response, &UA_TYPES[UA_TYPES_CREATESESSIONRESPONSE],
                        client);

src/ua_securechannel.c

@@ -1,12 +1,82 @@
 #include "ua_util.h"
 #include "ua_securechannel.h"
 #include "ua_statuscodes.h"
+#include "ua_types_encoding_binary.h"
 
 // max message size is 64k
 const UA_ConnectionConfig UA_ConnectionConfig_standard =
     {.protocolVersion = 0, .sendBufferSize = 65536, .recvBufferSize  = 65536,
      .maxMessageSize = 65536, .maxChunkCount   = 1};
 
+UA_ByteString UA_Connection_completeMessages(UA_Connection *connection, UA_ByteString received)
+{
+    if(received.length == -1)
+        return received;
+
+    /* concat received to the incomplete message we have */
+    UA_ByteString current;
+    if(connection->incompleteMessage.length < 0)
+        current = received;
+    else {
+        current.data = UA_realloc(connection->incompleteMessage.data,
+                                  connection->incompleteMessage.length + received.length);
+        if(!current.data) {
+            /* not enough memory */
+            UA_ByteString_deleteMembers(&connection->incompleteMessage);
+            connection->incompleteMessage.length = -1;
+            UA_ByteString_deleteMembers(&received);
+            received.length = -1;
+            return received;
+        }
+        UA_memcpy(current.data + connection->incompleteMessage.length, received.data, received.length);
+        current.length = connection->incompleteMessage.length + received.length;
+        UA_ByteString_deleteMembers(&received);
+        UA_ByteString_init(&connection->incompleteMessage);
+    }
+
+    /* find the first non-complete message */
+    size_t end_pos = 0; // the end of the last complete message
+    while(current.length - end_pos >= 32) {
+        if(!(current.data[0] == 'M' && current.data[1] == 'S' && current.data[2] == 'G') &&
+           !(current.data[0] == 'O' && current.data[1] == 'P' && current.data[2] == 'N') &&
+           !(current.data[0] == 'H' && current.data[1] == 'E' && current.data[2] == 'L') &&
+           !(current.data[0] == 'A' && current.data[1] == 'C' && current.data[2] == 'K') &&
+           !(current.data[0] == 'C' && current.data[1] == 'L' && current.data[2] == 'O')) {
+            current.length = end_pos; // throw the remaining bytestring away
+            break;
+        }
+        UA_Int32 length = 0;
+        size_t pos = end_pos + 4;
+        UA_StatusCode retval = UA_Int32_decodeBinary(&current, &pos, &length);
+        if(retval != UA_STATUSCODE_GOOD || length < 32 ||
+           length > (UA_Int32)connection->localConf.maxMessageSize) {
+            current.length = end_pos; // throw the remaining bytestring away
+            break;
+        }
+        if(length + (UA_Int32)end_pos > current.length)
+            break; // the message is incomplete
+        end_pos += length;
+    }
+
+    if(current.length == 0) { /* throw everything away */
+        UA_String_deleteMembers(&current);
+        current.length = -1;
+        return current;
+    }
+
+    /* retain the incomplete message at the end */
+    if(current.length - end_pos > 0) {
+        connection->incompleteMessage.data = UA_malloc(current.length - end_pos);
+        if(connection->incompleteMessage.data) {
+            UA_memcpy(connection->incompleteMessage.data, &current.data[end_pos], current.length - end_pos);
+            connection->incompleteMessage.length = current.length - end_pos;
+        }
+    }
+
+    current.length = end_pos;
+    return current;
+}
+
 void UA_SecureChannel_init(UA_SecureChannel *channel) {
     UA_MessageSecurityMode_init(&channel->securityMode);
     UA_ChannelSecurityToken_init(&channel->securityToken);

src/ua_types.c

@@ -89,10 +89,8 @@ void UA_String_init(UA_String *p) {
 }
 
 void UA_String_deleteMembers(UA_String *p) {
-	if(p->data) {
-		UA_free(p->data);
-        p->data = UA_NULL;
-    }
+    UA_free(p->data);
+    p->data = UA_NULL;
 }
 
 UA_StatusCode UA_String_copy(UA_String const *src, UA_String *dst) {
@@ -108,7 +106,7 @@ UA_StatusCode UA_String_copy(UA_String const *src, UA_String *dst) {
 
 UA_String UA_String_fromChars(char const *src) {
     UA_String str;
-    size_t length = sizeof(src)-1;
+    size_t length = strlen(src);
     if(length == 0) {
         str.length = 0;
         str.data = UA_NULL;