Browse Source

Fix DoS bug which leads to endless loop

If the client indicates a receiveSize which is too small,
the server may run into an endless loop.

Credit to OSS-Fuzz
Stefan Profanter 7 years ago
parent
commit
6a55d79ed8
3 changed files with 29 additions and 1 deletions
  1. 20 1
      src/server/ua_server_binary.c
  2. 5 0
      src/ua_securechannel.c
  3. 4 0
      src/ua_types_encoding_binary.c

+ 20 - 1
src/server/ua_server_binary.c

@@ -243,6 +243,14 @@ processHEL(UA_Server *server, UA_Connection *connection, const UA_ByteString *ms
     connection->state = UA_CONNECTION_ESTABLISHED;
     UA_TcpHelloMessage_deleteMembers(&helloMessage);
 
+    if (connection->remoteConf.recvBufferSize == 0) {
+        UA_LOG_INFO(server->config.logger, UA_LOGCATEGORY_NETWORK,
+                    "Connection %i | Remote end indicated a receive buffer size of 0. Not able to send any messages.",
+                    connection->sockfd);
+        connection->close(connection);
+        return;
+    }
+
     /* Build acknowledge response */
     UA_TcpAcknowledgeMessage ackMessage;
     ackMessage.protocolVersion = connection->localConf.protocolVersion;
@@ -328,6 +336,17 @@ processOPN(UA_Server *server, UA_Connection *connection,
         return;
     }
 
+    // every message contains secureconversationmessageheader. Thus its size has to be
+    // the minimum buffer size
+    if (connection->localConf.sendBufferSize <= 12) {
+        UA_OpenSecureChannelResponse_deleteMembers(&p);
+        UA_AsymmetricAlgorithmSecurityHeader_deleteMembers(&asymHeader);
+        UA_LOG_INFO(server->config.logger, UA_LOGCATEGORY_NETWORK,
+                    "Connection %i | Response too large for client configuration. %s", connection->sockfd, UA_StatusCode_name(UA_STATUSCODE_BADRESPONSETOOLARGE));
+        connection->close(connection);
+        return;
+    }
+
     /* Set the starting sequence number */
     channel->receiveSequenceNumber = seqHeader.sequenceNumber;
 
@@ -358,7 +377,7 @@ processOPN(UA_Server *server, UA_Connection *connection,
     if(retval != UA_STATUSCODE_GOOD) {
         UA_LOG_INFO(server->config.logger, UA_LOGCATEGORY_NETWORK,
                     "Connection %i | Could not encode the OPN message. "
-                    "Closing the connection.", connection->sockfd);
+                    "Closing the connection. %s", connection->sockfd, UA_StatusCode_name(retval));
         connection->releaseSendBuffer(connection, &resp_msg);
         UA_OpenSecureChannelResponse_deleteMembers(&p);
         UA_AsymmetricAlgorithmSecurityHeader_deleteMembers(&asymHeader);

+ 5 - 0
src/ua_securechannel.c

@@ -199,6 +199,10 @@ UA_SecureChannel_sendBinaryMessage(UA_SecureChannel *channel, UA_UInt32 requestI
     if(!connection)
         return UA_STATUSCODE_BADINTERNALERROR;
 
+    // Minimum required size
+    if (connection->localConf.sendBufferSize <= UA_SECURE_MESSAGE_HEADER_LENGTH)
+        return UA_STATUSCODE_BADRESPONSETOOLARGE;
+
     /* Create the chunking info structure */
     UA_ChunkInfo ci;
     ci.channel = channel;
@@ -209,6 +213,7 @@ UA_SecureChannel_sendBinaryMessage(UA_SecureChannel *channel, UA_UInt32 requestI
     ci.messageType = UA_MESSAGETYPE_MSG;
     ci.errorCode = UA_STATUSCODE_GOOD;
 
+
     /* Allocate the message buffer */
     UA_StatusCode retval =
         connection->getSendBuffer(connection, connection->localConf.sendBufferSize, &ci.buffer);

+ 4 - 0
src/ua_types_encoding_binary.c

@@ -1404,6 +1404,10 @@ UA_encodeBinaryInternal(const void *src, const UA_DataType *type) {
                 pos = oldpos; /* exchange/send the buffer */
                 retval = exchangeBuffer();
                 ptr -= member->padding + memSize; /* encode the same member in the next iteration */
+                if (retval == UA_STATUSCODE_BADENCODINGLIMITSEXCEEDED || pos + memSize > end) {
+                    // the send buffer is too small to encode the member, even after exchangeBuffer
+                    return UA_STATUSCODE_BADENCODINGLIMITSEXCEEDED;
+                }
                 --i;
             }
         } else {