Fix #1930

Dirty fix for incorrect processing sequence

Swap keys correctly on token switchover

Timeout is now calculated per token. Removed testing code

Fix?? bug in worker

src/client/ua_client.c

@@ -260,9 +260,12 @@ static UA_StatusCode
 sendSymmetricServiceRequest(UA_Client *client, const void *request,
                             const UA_DataType *requestType, UA_UInt32 *requestId) {
     /* Make sure we have a valid session */
-    UA_StatusCode retval = openSecureChannel(client, true);
+    UA_StatusCode retval = UA_STATUSCODE_GOOD;
+    /* FIXME: this is just a dirty workaround. We need to rework some of the sync and async processing
+     * FIXME: in the client. Currently a lot of stuff is semi broken and in dire need of cleaning up.*/
+    /*UA_StatusCode retval = openSecureChannel(client, true);
     if(retval != UA_STATUSCODE_GOOD)
-        return retval;
+        return retval;*/
 
     /* Adjusting the request header. The const attribute is violated, but we
      * only touch the following members: */
@@ -438,10 +441,9 @@ finish:
 static UA_StatusCode
 client_processChunk(void *application, UA_Connection *connection, UA_ByteString *chunk) {
     SyncResponseDescription *rd = (SyncResponseDescription*)application;
-    UA_StatusCode retval = UA_SecureChannel_decryptAddChunk(&rd->client->channel, chunk);
+    UA_StatusCode retval = UA_SecureChannel_decryptAddChunk(&rd->client->channel, chunk, UA_TRUE);
     if(retval != UA_STATUSCODE_GOOD)
         return retval;
-    UA_SecureChannel_processCompleteMessages(&rd->client->channel, rd, processServiceResponse);
     return UA_SecureChannel_persistIncompleteMessages(&rd->client->channel);
 }
 
@@ -471,6 +473,7 @@ receiveServiceResponse(UA_Client *client, void *response, const UA_DataType *res
         UA_UInt32 timeout = (UA_UInt32)(((maxDate - now) + (UA_DATETIME_MSEC - 1)) / UA_DATETIME_MSEC);
 
         retval = UA_Connection_receiveChunksBlocking(&client->connection, &rd, client_processChunk, timeout);
+        UA_SecureChannel_processCompleteMessages(&client->channel, &rd, processServiceResponse);
 
         if(retval != UA_STATUSCODE_GOOD && retval != UA_STATUSCODE_GOODNONCRITICALTIMEOUT) {
             if(retval == UA_STATUSCODE_BADCONNECTIONCLOSED)
@@ -521,6 +524,7 @@ receiveServiceResponseAsync(UA_Client *client, void *response,
 
     UA_StatusCode retval = UA_Connection_receiveChunksNonBlocking(
             &client->connection, &rd, client_processChunk);
+    UA_SecureChannel_processCompleteMessages(&client->channel, &rd, processServiceResponse);
     /*let client run when non critical timeout*/
     if(retval != UA_STATUSCODE_GOOD
             && retval != UA_STATUSCODE_GOODNONCRITICALTIMEOUT) {

src/client/ua_client_connect_async.c

@@ -202,7 +202,7 @@ static UA_StatusCode
 processOPNResponse(void *application, UA_Connection *connection,
                    UA_ByteString *chunk) {
     UA_Client *client = (UA_Client*) application;
-    UA_StatusCode retval = UA_SecureChannel_decryptAddChunk(&client->channel, chunk);
+    UA_StatusCode retval = UA_SecureChannel_decryptAddChunk(&client->channel, chunk, UA_TRUE);
     client->connectStatus = retval;
     if(retval != UA_STATUSCODE_GOOD)
         goto error;

src/client/ua_client_worker.c

@@ -158,14 +158,22 @@ UA_Client_backgroundConnectivity(UA_Client *client) {
 
 UA_StatusCode UA_Client_run_iterate(UA_Client *client, UA_UInt16 timeout) {
 // TODO connectivity check & timeout features for the async implementation (timeout == 0)
-    UA_StatusCode retval;
+    UA_StatusCode retval = UA_STATUSCODE_GOOD;
 #ifdef UA_ENABLE_SUBSCRIPTIONS
     UA_StatusCode retvalPublish = UA_Client_Subscriptions_backgroundPublish(client);
     if(client->state >= UA_CLIENTSTATE_SESSION && retvalPublish != UA_STATUSCODE_GOOD)
         return retvalPublish;
 #endif
-    if(timeout) {
+    /* Make sure we have an open channel */
+
+    /************************************************************/
+    /* FIXME: This is a dirty workaround */
+    if(client->state >= UA_CLIENTSTATE_SECURECHANNEL)
         retval = openSecureChannel(client, true);
+    /* FIXME: Will most likely break somewhere in the future */
+    /************************************************************/
+
+    if(timeout) {
         if(retval != UA_STATUSCODE_GOOD)
             return retval;
 
@@ -196,7 +204,6 @@ UA_StatusCode UA_Client_run_iterate(UA_Client *client, UA_UInt16 timeout) {
         } else {
             retval = receivePacketAsync(client);
         }
-
     }
 #ifdef UA_ENABLE_SUBSCRIPTIONS
         /* The inactivity check must be done after receiveServiceResponse*/

src/server/ua_server_binary.c

@@ -718,7 +718,7 @@ processCompleteChunkWithoutChannel(UA_Server *server, UA_Connection *connection,
         if(retval != UA_STATUSCODE_GOOD)
             break;
 
-        retval = UA_SecureChannel_decryptAddChunk(connection->channel, message);
+        retval = UA_SecureChannel_decryptAddChunk(connection->channel, message, UA_FALSE);
         if(retval != UA_STATUSCODE_GOOD)
             break;
 
@@ -745,7 +745,7 @@ processCompleteChunk(void *const application, UA_Connection *connection,
 #endif
     if(!connection->channel)
         return processCompleteChunkWithoutChannel(server, connection, chunk);
-    return UA_SecureChannel_decryptAddChunk(connection->channel, chunk);
+    return UA_SecureChannel_decryptAddChunk(connection->channel, chunk, UA_FALSE);
 }
 
 void

src/ua_securechannel.c

@@ -266,11 +266,18 @@ UA_SecureChannel_revolveTokens(UA_SecureChannel *channel) {
     if(channel->nextSecurityToken.tokenId == 0) // no security token issued
         return UA_STATUSCODE_BADSECURECHANNELTOKENUNKNOWN;
 
-    //FIXME: not thread-safe
-    memcpy(&channel->securityToken, &channel->nextSecurityToken,
-           sizeof(UA_ChannelSecurityToken));
+    //FIXME: not thread-safe ???? Why is this not thread safe?
+    UA_ChannelSecurityToken_deleteMembers(&channel->previousSecurityToken);
+    UA_ChannelSecurityToken_copy(&channel->securityToken, &channel->previousSecurityToken);
+
+    UA_ChannelSecurityToken_deleteMembers(&channel->securityToken);
+    UA_ChannelSecurityToken_copy(&channel->nextSecurityToken, &channel->securityToken);
+
+    UA_ChannelSecurityToken_deleteMembers(&channel->nextSecurityToken);
     UA_ChannelSecurityToken_init(&channel->nextSecurityToken);
-    return UA_SecureChannel_generateNewKeys(channel);
+
+    /* remote keys are generated later on */
+    return UA_SecureChannel_generateLocalKeys(channel, channel->securityPolicy);
 }
 /***************************/
 /* Send Asymmetric Message */
@@ -1133,15 +1140,53 @@ checkAsymHeader(UA_SecureChannel *const channel,
     return UA_STATUSCODE_GOOD;
 }
 
+static UA_StatusCode
+checkPreviousToken(UA_SecureChannel *const channel, const UA_UInt32 tokenId) {
+    if(tokenId != channel->previousSecurityToken.tokenId)
+        return UA_STATUSCODE_BADSECURECHANNELTOKENUNKNOWN;
+
+    UA_DateTime timeout = channel->previousSecurityToken.createdAt +
+                          (UA_DateTime)((UA_Double)channel->previousSecurityToken.revisedLifetime *
+                                        (UA_Double)UA_DATETIME_MSEC *
+                                        1.25);
+
+    if(timeout < UA_DateTime_nowMonotonic()) {
+        return UA_STATUSCODE_BADSECURECHANNELTOKENUNKNOWN;
+    }
+
+    return UA_STATUSCODE_GOOD;
+}
+
 static UA_StatusCode
 checkSymHeader(UA_SecureChannel *const channel,
-               const UA_UInt32 tokenId) {
+               const UA_UInt32 tokenId, UA_Boolean allowPreviousToken) {
+
+    if(tokenId == channel->securityToken.tokenId) {
+        if(channel->state == UA_SECURECHANNELSTATE_OPEN &&
+           (channel->securityToken.createdAt +
+            (channel->securityToken.revisedLifetime * UA_DATETIME_MSEC))
+           < UA_DateTime_nowMonotonic()) {
+            UA_SecureChannel_deleteMembersCleanup(channel);
+            return UA_STATUSCODE_BADSECURECHANNELCLOSED;
+        }
+    }
+
     if(tokenId != channel->securityToken.tokenId) {
-        if(tokenId != channel->nextSecurityToken.tokenId)
-            return UA_STATUSCODE_BADSECURECHANNELTOKENUNKNOWN;
+        if(tokenId != channel->nextSecurityToken.tokenId) {
+            if(allowPreviousToken)
+                return checkPreviousToken(channel, tokenId);
+            else
+                return UA_STATUSCODE_BADSECURECHANNELTOKENUNKNOWN;
+        }
         return UA_SecureChannel_revolveTokens(channel);
     }
 
+    if(channel->previousSecurityToken.tokenId != 0) {
+        UA_StatusCode retval = UA_SecureChannel_generateRemoteKeys(channel, channel->securityPolicy);
+        UA_ChannelSecurityToken_deleteMembers(&channel->previousSecurityToken);
+        return retval;
+    }
+
     return UA_STATUSCODE_GOOD;
 }
 
@@ -1153,15 +1198,17 @@ putPayload(UA_SecureChannel *const channel, UA_UInt32 const requestId, UA_Messag
     case UA_CHUNKTYPE_FINAL:
         return addChunkPayload(channel, requestId, messageType,
                                chunkPayload, chunkType == UA_CHUNKTYPE_FINAL);
-    case UA_CHUNKTYPE_ABORT:deleteLatestMessage(channel, requestId);
+    case UA_CHUNKTYPE_ABORT:
+        deleteLatestMessage(channel, requestId);
         return UA_STATUSCODE_GOOD;
-    default:return UA_STATUSCODE_BADTCPMESSAGETYPEINVALID;
+    default:
+        return UA_STATUSCODE_BADTCPMESSAGETYPEINVALID;
     }
 }
 
 /* The chunk body begins after the SecureConversationMessageHeader */
 static UA_StatusCode
-decryptAddChunk(UA_SecureChannel *channel, const UA_ByteString *chunk) {
+decryptAddChunk(UA_SecureChannel *channel, const UA_ByteString *chunk, UA_Boolean allowPreviousToken) {
     /* Decode the MessageHeader */
     size_t offset = 0;
     UA_SecureConversationMessageHeader messageHeader;
@@ -1212,7 +1259,7 @@ decryptAddChunk(UA_SecureChannel *channel, const UA_ByteString *chunk) {
         symmetricSecurityHeader.tokenId = channel->securityToken.tokenId;
 #endif
 
-        retval = checkSymHeader(channel, symmetricSecurityHeader.tokenId);
+        retval = checkSymHeader(channel, symmetricSecurityHeader.tokenId, allowPreviousToken);
         if(retval != UA_STATUSCODE_GOOD)
             return retval;
 
@@ -1257,33 +1304,29 @@ decryptAddChunk(UA_SecureChannel *channel, const UA_ByteString *chunk) {
     if(retval != UA_STATUSCODE_GOOD)
         return retval;
 
-#if !defined(FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION)
     /* Check the sequence number. Skip sequence number checking for fuzzer to
      * improve coverage */
     if(sequenceNumberCallback == NULL)
         return UA_STATUSCODE_BADINTERNALERROR;
+#if defined(FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION)
+    retval = UA_STATUSCODE_GOOD;
+#else
     retval = sequenceNumberCallback(channel, sequenceNumber);
+#endif
     if(retval != UA_STATUSCODE_GOOD)
         return retval;
-#endif
 
     return putPayload(channel, requestId, messageType, chunkType, &chunkPayload);
 }
 
 UA_StatusCode
-UA_SecureChannel_decryptAddChunk(UA_SecureChannel *channel, const UA_ByteString *chunk) {
+UA_SecureChannel_decryptAddChunk(UA_SecureChannel *channel, const UA_ByteString *chunk,
+                                 UA_Boolean allowPreviousToken) {
     /* Has the SecureChannel timed out? */
     if(channel->state == UA_SECURECHANNELSTATE_CLOSED)
         return UA_STATUSCODE_BADSECURECHANNELCLOSED;
-    if(channel->state == UA_SECURECHANNELSTATE_OPEN &&
-       (channel->securityToken.createdAt +
-        (channel->securityToken.revisedLifetime * UA_DATETIME_MSEC))
-       < UA_DateTime_nowMonotonic()) {
-        UA_SecureChannel_deleteMembersCleanup(channel);
-        return UA_STATUSCODE_BADSECURECHANNELCLOSED;
-    }
 
-    UA_StatusCode retval = decryptAddChunk(channel, chunk);
+    UA_StatusCode retval = decryptAddChunk(channel, chunk, allowPreviousToken);
     if(retval != UA_STATUSCODE_GOOD)
         UA_SecureChannel_deleteMembersCleanup(channel);
     return retval;

src/ua_securechannel.h

@@ -73,8 +73,16 @@ typedef TAILQ_HEAD(UA_MessageQueue, UA_Message) UA_MessageQueue;
 struct UA_SecureChannel {
     UA_SecureChannelState   state;
     UA_MessageSecurityMode  securityMode;
+    /* We use three tokens because when switching tokens the client is allowed to accept
+     * messages with the old token for up to 25% of the lifetime after the token would have timed out.
+     * For messages that are sent, the new token is already used, which is contained in the securityToken
+     * variable. The nextSecurityToken variable holds a newly issued token, that will be automatically
+     * revolved into the securityToken variable. This could be done with two variables, but would require
+     * greater changes to the current code. This could be done in the future after the client and networking
+     * structure has been reworked, which would make this easier to implement. */
     UA_ChannelSecurityToken securityToken; /* the channelId is contained in the securityToken */
     UA_ChannelSecurityToken nextSecurityToken;
+    UA_ChannelSecurityToken previousSecurityToken;
 
     /* The endpoint and context of the channel */
     const UA_SecurityPolicy *securityPolicy;
@@ -185,7 +193,8 @@ UA_MessageContext_abort(UA_MessageContext *mc);
 
 /* Decrypt a chunk and add it to the message. Create a new message if necessary. */
 UA_StatusCode
-UA_SecureChannel_decryptAddChunk(UA_SecureChannel *channel, const UA_ByteString *chunk);
+UA_SecureChannel_decryptAddChunk(UA_SecureChannel *channel, const UA_ByteString *chunk,
+                                 UA_Boolean allowPreviousToken);
 
 /* The network buffer is about to be cleared. Copy all chunks that point into
  * the network buffer into dedicated memory. */

tests/fuzz/ua_debug_dump_pkgs_file.c

@@ -157,7 +157,7 @@ UA_debug_dumpCompleteChunk(UA_Server *const server, UA_Connection *const connect
         TAILQ_INIT(&dummy.messages);
         UA_ByteString messageBufferCopy;
         UA_ByteString_copy(messageBuffer, &messageBufferCopy);
-        UA_SecureChannel_decryptAddChunk(&dummy, &messageBufferCopy);
+        UA_SecureChannel_decryptAddChunk(&dummy, &messageBufferCopy, UA_TRUE);
         UA_SecureChannel_processCompleteMessages(&dummy, &dump_filename, UA_debug_dump_setName_withChannel);
         UA_SecureChannel_deleteMessages(&dummy);
         UA_ByteString_deleteMembers(&messageBufferCopy);