Fix bugs preventing encryption from working correctly

src/server/ua_server_binary.c

@@ -47,6 +47,8 @@ sendServiceFault(UA_SecureChannel *channel, const UA_ByteString *msg,
     retval = UA_SecureChannel_sendSymmetricMessage(channel, requestId, UA_MESSAGETYPE_MSG,
                                                    response, responseType);
     UA_RequestHeader_deleteMembers(&requestHeader);
+    UA_LOG_DEBUG(channel->securityPolicy->logger, UA_LOGCATEGORY_SERVER, "Sent ServiceFault"
+                 "with error code %i", error);
     return retval;
 }
 
@@ -314,7 +316,7 @@ processOPN(UA_Server *server, UA_SecureChannel *channel,
         UA_SecureChannelManager_close(&server->secureChannelManager, channel->securityToken.channelId);
         return retval;
     }
-	UA_NodeId_deleteMembers(&requestType);
+    UA_NodeId_deleteMembers(&requestType);
 
     /* Call the service */
     UA_OpenSecureChannelResponse openScResponse;
@@ -405,16 +407,16 @@ processMSG(UA_Server *server, UA_SecureChannel *channel,
             (const UA_CreateSessionRequest *)request,
                               (UA_CreateSessionResponse *)response);
         #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-		// store the authentication token and session ID so we can help fuzzing by setting
+        // store the authentication token and session ID so we can help fuzzing by setting
         // these values in the next request automatically
         UA_CreateSessionResponse *res = (UA_CreateSessionResponse *)response;
         UA_NodeId_copy(&res->authenticationToken, &unsafe_fuzz_authenticationToken);
-		#endif
+        #endif
         goto send_response;
     }
 
     #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-	// set the authenticationToken from the create session request to help fuzzing cover more lines
+    // set the authenticationToken from the create session request to help fuzzing cover more lines
     if (!UA_NodeId_isNull(&unsafe_fuzz_authenticationToken))
         UA_NodeId_copy(&unsafe_fuzz_authenticationToken, &requestHeader->authenticationToken);
     #endif
@@ -443,6 +445,7 @@ processMSG(UA_Server *server, UA_SecureChannel *channel,
     /* Set an anonymous, inactive session for services that need no session */
     UA_Session anonymousSession;
     if(!session) {
+
         if(sessionRequired) {
             UA_LOG_INFO_CHANNEL(server->config.logger, channel,
                                 "Service request %i without a valid session",
@@ -451,6 +454,7 @@ processMSG(UA_Server *server, UA_SecureChannel *channel,
             return sendServiceFault(channel, msg, requestPos, responseType,
                                     requestId, UA_STATUSCODE_BADSESSIONIDINVALID);
         }
+
         UA_Session_init(&anonymousSession);
         anonymousSession.sessionId = UA_NODEID_GUID(0, UA_GUID_NULL);
         anonymousSession.channel = channel;

src/server/ua_services_session.c

@@ -68,6 +68,17 @@ nonceAndSignCreateSessionResponse(UA_Server *server, UA_SecureChannel *channel,
 void Service_CreateSession(UA_Server *server, UA_SecureChannel *channel,
                            const UA_CreateSessionRequest *request,
                            UA_CreateSessionResponse *response) {
+    UA_LOG_DEBUG_CHANNEL(server->config.logger, channel, "Trying to create session");
+    if(channel == NULL) {
+        response->responseHeader.serviceResult = UA_STATUSCODE_BADINTERNALERROR;
+        return;
+    }
+
+    if(channel->connection == NULL) {
+        response->responseHeader.serviceResult = UA_STATUSCODE_BADINTERNALERROR;
+        return;
+    }
+
     if(channel->securityMode == UA_MESSAGESECURITYMODE_SIGN ||
        channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT) {
         if(!UA_ByteString_equal(&request->clientCertificate,
@@ -89,6 +100,8 @@ void Service_CreateSession(UA_Server *server, UA_SecureChannel *channel,
         return;
     }
 
+    ////////////////////// TODO: Compare application URI with certificate uri (decode certificate)
+
     /* Allocate the response */
     response->serverEndpoints = (UA_EndpointDescription*)
         UA_Array_new(server->config.endpointsSize,
@@ -139,11 +152,10 @@ void Service_CreateSession(UA_Server *server, UA_SecureChannel *channel,
     response->responseHeader.serviceResult =
         UA_String_copy(&request->sessionName, &newSession->sessionName);
 
-    /* Todo: Copy from the session's endpoint */
-    /* if(server->config.endpointsSize > 0) */
-    /*     response->responseHeader.serviceResult |= */
-    /*     UA_ByteString_copy(&channel->endpoint->endpointDescription.serverCertificate, */
-    /*                        &response->serverCertificate); */
+    if(server->config.endpointsSize > 0)
+         response->responseHeader.serviceResult |=
+         UA_ByteString_copy(&channel->securityPolicy->localCertificate,
+                            &response->serverCertificate);
 
     /* Create a signed nonce */
     response->responseHeader.serviceResult =

src/ua_securechannel.c

@@ -219,10 +219,11 @@ calculateAsymAlgSecurityHeaderLength(const UA_SecureChannel *channel) {
     size_t asymHeaderLength = UA_ASYMMETRIC_ALG_SECURITY_HEADER_FIXED_LENGTH +
         channel->securityPolicy->policyUri.length;
     if(channel->securityMode == UA_MESSAGESECURITYMODE_SIGN ||
-       channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT)
-        asymHeaderLength += channel->securityPolicy->localCertificate.length;
-    if(channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT)
+       channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT) {
+        // OPN is always encrypted even if mode sign only
         asymHeaderLength += 20; /* Thumbprints are always 20 byte long */
+        asymHeaderLength += channel->securityPolicy->localCertificate.length;
+    }
     return asymHeaderLength;
 }
 
@@ -298,7 +299,9 @@ UA_SecureChannel_sendAsymmetricOPNMessage(UA_SecureChannel *channel, UA_UInt32 r
         UA_UInt16 totalPaddingSize =
             calculatePaddingAsym(securityPolicy, channel->channelContext,
                                  bytesToWrite, &paddingSize, &extraPaddingSize);
-        for(UA_UInt16 i = 0; i < totalPaddingSize; ++i) {
+
+        // This is <= because the paddingSize byte also has to be written.
+        for(UA_UInt16 i = 0; i <= totalPaddingSize; ++i) {
             *buf_pos = paddingSize;
             ++buf_pos;
         }
@@ -333,9 +336,8 @@ UA_SecureChannel_sendAsymmetricOPNMessage(UA_SecureChannel *channel, UA_UInt32 r
     UA_AsymmetricAlgorithmSecurityHeader_init(&asymHeader);
     asymHeader.securityPolicyUri = channel->securityPolicy->policyUri;
     if(channel->securityMode == UA_MESSAGESECURITYMODE_SIGN ||
-       channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT)
+       channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT) {
         asymHeader.senderCertificate = channel->securityPolicy->localCertificate;
-    if(channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT) {
         asymHeader.receiverCertificateThumbprint.length = 20;
         asymHeader.receiverCertificateThumbprint.data = channel->remoteCertificateThumbprint;
     }
@@ -445,7 +447,9 @@ sendChunkSymmetric(UA_ChunkInfo* ci, UA_Byte **buf_pos, const UA_Byte **buf_end)
         UA_UInt16 totalPaddingSize =
             calculatePaddingSym(securityPolicy, channel->channelContext,
                                 bytesToWrite, &paddingSize, &extraPaddingSize);
-        for(UA_UInt16 i = 0; i < totalPaddingSize; ++i) {
+
+        // This is <= because the paddingSize byte also has to be written.
+        for(UA_UInt16 i = 0; i <= totalPaddingSize; ++i) {
             **buf_pos = paddingSize;
             ++(*buf_pos);
         }
@@ -713,6 +717,7 @@ decryptChunk(UA_SecureChannel *channel, const UA_SecurityPolicyCryptoModule *cry
              UA_MessageType messageType) {
     UA_StatusCode retval = UA_STATUSCODE_GOOD;
     const UA_SecurityPolicy *securityPolicy = channel->securityPolicy;
+    size_t chunkSizeAfterDecryption = chunk->length;
 
     if(cryptoModule == NULL)
         return UA_STATUSCODE_BADINTERNALERROR;
@@ -721,7 +726,9 @@ decryptChunk(UA_SecureChannel *channel, const UA_SecurityPolicyCryptoModule *cry
     if(channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT ||
        messageType == UA_MESSAGETYPE_OPN) {
         UA_ByteString cipherText = { chunk->length - offset, chunk->data + offset };
+        size_t sizeBeforeDecryption = cipherText.length;
         retval = cryptoModule->decrypt(securityPolicy, channel->channelContext, &cipherText);
+        chunkSizeAfterDecryption -= (sizeBeforeDecryption - cipherText.length);
         if(retval != UA_STATUSCODE_GOOD)
             return retval;
     }
@@ -735,23 +742,25 @@ decryptChunk(UA_SecureChannel *channel, const UA_SecurityPolicyCryptoModule *cry
         /* Compute the padding size */
         sigsize = cryptoModule->getRemoteSignatureSize(securityPolicy, channel->channelContext);
 
-        if(channel->securityMode != UA_MESSAGESECURITYMODE_NONE)
-            paddingSize = chunk->data[chunk->length - sigsize - 1];
-
-        size_t keyLength =
-            cryptoModule->getRemoteEncryptionKeyLength(securityPolicy, channel->channelContext);
-        if(keyLength > 2048) {
-            paddingSize <<= 8; /* Extra padding size */
-            paddingSize += chunk->data[chunk->length - sigsize - 2];
+        if(channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT ||
+            (messageType == UA_MESSAGETYPE_OPN &&
+             channel->securityMode != UA_MESSAGESECURITYMODE_NONE)) {
+            paddingSize = chunk->data[chunkSizeAfterDecryption - sigsize - 1];
+
+            size_t keyLength =
+                cryptoModule->getRemoteEncryptionKeyLength(securityPolicy, channel->channelContext);
+            if(keyLength > 2048) {
+                paddingSize <<= 8; /* Extra padding size */
+                paddingSize += chunk->data[chunkSizeAfterDecryption - sigsize - 2];
+            }
         }
-        if(offset + paddingSize + sigsize >= chunk->length)
+        if(offset + paddingSize + sigsize >= chunkSizeAfterDecryption)
             return UA_STATUSCODE_BADSECURITYCHECKSFAILED;
 
         /* Verify the signature */
-        const UA_ByteString chunkDataToVerify = { chunk->length - sigsize, chunk->data };
-        const UA_ByteString signature = { sigsize, chunk->data + chunk->length - sigsize };
-        retval = securityPolicy->asymmetricModule.cryptoModule.
-            verify(securityPolicy, channel->channelContext, &chunkDataToVerify, &signature);
+        const UA_ByteString chunkDataToVerify = { chunkSizeAfterDecryption - sigsize, chunk->data };
+        const UA_ByteString signature = { sigsize, chunk->data + chunkSizeAfterDecryption - sigsize };
+        retval = cryptoModule->verify(securityPolicy, channel->channelContext, &chunkDataToVerify, &signature);
         if(retval != UA_STATUSCODE_GOOD)
             return retval;
     }
@@ -768,7 +777,7 @@ decryptChunk(UA_SecureChannel *channel, const UA_SecurityPolicyCryptoModule *cry
     *requestId = sequenceHeader.requestId;
     *sequenceNumber = sequenceHeader.sequenceNumber;
     payload->data = chunk->data + offset;
-    payload->length = chunk->length - offset - sigsize - paddingSize;
+    payload->length = chunkSizeAfterDecryption - offset - sigsize - paddingSize;
     return UA_STATUSCODE_GOOD;
 }