Browse Source

Fix incorrect padding calculation (#1551)

* Fix incorrect padding calculation

* Fix extra padding as well
Mark Giraud 6 years ago
parent
commit
a836a1c9f4
1 changed files with 15 additions and 10 deletions
  1. 15 10
      src/ua_securechannel.c

+ 15 - 10
src/ua_securechannel.c

@@ -192,7 +192,7 @@ calculatePaddingAsym(const UA_SecurityPolicy *securityPolicy, const void *channe
         getLocalSignatureSize(securityPolicy, channelContext);
         getLocalSignatureSize(securityPolicy, channelContext);
     size_t paddingBytes = 1;
     size_t paddingBytes = 1;
     if(securityPolicy->asymmetricModule.cryptoModule.
     if(securityPolicy->asymmetricModule.cryptoModule.
-       getRemoteEncryptionKeyLength(securityPolicy, channelContext) > 2048)
+        getRemoteEncryptionKeyLength(securityPolicy, channelContext) > 2048)
         ++paddingBytes;
         ++paddingBytes;
     size_t padding = (plainTextBlockSize - ((bytesToWrite + signatureSize + paddingBytes) %
     size_t padding = (plainTextBlockSize - ((bytesToWrite + signatureSize + paddingBytes) %
                                             plainTextBlockSize));
                                             plainTextBlockSize));
@@ -294,7 +294,7 @@ UA_SecureChannel_sendAsymmetricOPNMessage(UA_SecureChannel *channel, UA_UInt32 r
             ++buf_pos;
             ++buf_pos;
         }
         }
         if(securityPolicy->asymmetricModule.cryptoModule.
         if(securityPolicy->asymmetricModule.cryptoModule.
-           getRemoteEncryptionKeyLength(securityPolicy, channel->channelContext) > 2048) {
+            getRemoteEncryptionKeyLength(securityPolicy, channel->channelContext) > 2048) {
             *buf_pos = extraPaddingSize;
             *buf_pos = extraPaddingSize;
             ++buf_pos;
             ++buf_pos;
         }
         }
@@ -401,7 +401,7 @@ calculatePaddingSym(const UA_SecurityPolicy *securityPolicy, const void *channel
 
 
 static void
 static void
 setBufPos(UA_MessageContext *mc) {
 setBufPos(UA_MessageContext *mc) {
-    const UA_SecureChannel *channel= mc->channel;
+    const UA_SecureChannel *channel = mc->channel;
     const UA_SecurityPolicy *securityPolicy = channel->securityPolicy;
     const UA_SecurityPolicy *securityPolicy = channel->securityPolicy;
 
 
     /* Forward the data pointer so that the payload is encoded after the
     /* Forward the data pointer so that the payload is encoded after the
@@ -481,7 +481,7 @@ sendSymmetricChunk(UA_MessageContext *mc) {
     UA_SecureConversationMessageHeader respHeader;
     UA_SecureConversationMessageHeader respHeader;
     respHeader.secureChannelId = channel->securityToken.channelId;
     respHeader.secureChannelId = channel->securityToken.channelId;
     respHeader.messageHeader.messageTypeAndChunkType = mc->messageType;
     respHeader.messageHeader.messageTypeAndChunkType = mc->messageType;
-    respHeader.messageHeader.messageSize = (UA_UInt32)total_length;
+    respHeader.messageHeader.messageSize = (UA_UInt32) total_length;
     if(mc->final)
     if(mc->final)
         respHeader.messageHeader.messageTypeAndChunkType += UA_CHUNKTYPE_FINAL;
         respHeader.messageHeader.messageTypeAndChunkType += UA_CHUNKTYPE_FINAL;
     else
     else
@@ -536,7 +536,7 @@ sendSymmetricChunk(UA_MessageContext *mc) {
 static UA_StatusCode
 static UA_StatusCode
 sendSymmetricEncodingCallback(void *data, UA_Byte **buf_pos, const UA_Byte **buf_end) {
 sendSymmetricEncodingCallback(void *data, UA_Byte **buf_pos, const UA_Byte **buf_end) {
     /* Set buf values from encoding in the messagecontext */
     /* Set buf values from encoding in the messagecontext */
-    UA_MessageContext *mc = (UA_MessageContext*)data;
+    UA_MessageContext *mc = (UA_MessageContext *) data;
     mc->buf_pos = *buf_pos;
     mc->buf_pos = *buf_pos;
     mc->buf_end = *buf_end;
     mc->buf_end = *buf_end;
 
 
@@ -755,19 +755,24 @@ decryptChunk(UA_SecureChannel *channel, const UA_SecurityPolicyCryptoModule *cry
        channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT ||
        channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT ||
        messageType == UA_MESSAGETYPE_OPN) {
        messageType == UA_MESSAGETYPE_OPN) {
         /* Compute the padding size */
         /* Compute the padding size */
-        sigsize = cryptoModule-> getRemoteSignatureSize(securityPolicy, channel->channelContext);
+        sigsize = cryptoModule->getRemoteSignatureSize(securityPolicy, channel->channelContext);
 
 
         if(channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT ||
         if(channel->securityMode == UA_MESSAGESECURITYMODE_SIGNANDENCRYPT ||
            (messageType == UA_MESSAGETYPE_OPN &&
            (messageType == UA_MESSAGETYPE_OPN &&
-            channel->securityMode != UA_MESSAGESECURITYMODE_NONE)) {
-            paddingSize = chunk->data[chunkSizeAfterDecryption - sigsize - 1];
+            channel->securityMode > UA_MESSAGESECURITYMODE_NONE)) {
+            paddingSize = (size_t) chunk->data[chunkSizeAfterDecryption - sigsize - 1];
 
 
-            size_t keyLength = cryptoModule->
-                getRemoteEncryptionKeyLength(securityPolicy, channel->channelContext);
+            size_t keyLength = cryptoModule->getRemoteEncryptionKeyLength(securityPolicy, channel->channelContext);
             if(keyLength > 2048) {
             if(keyLength > 2048) {
                 paddingSize <<= 8; /* Extra padding size */
                 paddingSize <<= 8; /* Extra padding size */
                 paddingSize += chunk->data[chunkSizeAfterDecryption - sigsize - 2];
                 paddingSize += chunk->data[chunkSizeAfterDecryption - sigsize - 2];
+                // see comment below but for extraPaddingSize
+                paddingSize += 1;
             }
             }
+
+            // we need to add one to the padding size since the paddingSize byte itself need to be removed as well.
+            // TODO: write unit test for correct padding calculation
+            paddingSize += 1;
         }
         }
         if(offset + paddingSize + sigsize >= chunkSizeAfterDecryption)
         if(offset + paddingSize + sigsize >= chunkSizeAfterDecryption)
             return UA_STATUSCODE_BADSECURITYCHECKSFAILED;
             return UA_STATUSCODE_BADSECURITYCHECKSFAILED;