internal: reject inbound packets that are not block aligned

- DoReceive() validates the peeked packet_length in
  PROCESS_PACKET_LENGTH: UINT32_SZ plus curSz for non-AEAD, curSz
  alone for AEAD, against peerBlockSz floored at MIN_BLOCK_SZ. A
  non-zero remainder sets ssh->error to WS_BUFFER_E and returns
  WS_FATAL_ERROR.
- BuildPacket() in regress.c and BuildMacTestPacketPrefix() in
  unit.c pad to a block-aligned total, the latter taking padLen
  from the caller; test_DoReceive_VerifyMacFailure,
  test_DoReceive_AeadTagFailure, and
  test_DoReceive_RejectsShortPadding follow.
- test_DoReceive_RejectsMisalignedPacket and
  test_DoReceive_RejectsMisalignedAead cover the cleartext and
  AES-GCM paths.

Issue: F-8834

Author: yosuke-wolfssl

src/internal.c

@@ -12813,10 +12813,12 @@ int DoReceive(WOLFSSH* ssh)
     int ret = WS_SUCCESS;
     int verifyResult;
     word32 readSz;
+    word32 alignSz;
     byte peerBlockSz = ssh->peerBlockSz;
     byte peerMacSz = ssh->peerMacSz;
     byte aeadMode = ssh->peerAeadMode;
     byte bufferConsumed = 0;
+    byte alignBlockSz;
 
     switch (ssh->processReplyState) {
         case PROCESS_INIT:
@@ -12855,6 +12857,19 @@ int DoReceive(WOLFSSH* ssh)
                 ssh->error = WS_OVERFLOW_E;
                 return WS_FATAL_ERROR;
             }
+
+            /* RFC 4253 section 6: the packet is a multiple of the cipher
+             * block size, or 8, whichever is larger. */
+            alignBlockSz = peerBlockSz < MIN_BLOCK_SZ ?
+                    MIN_BLOCK_SZ : peerBlockSz;
+            alignSz = aeadMode ? ssh->curSz : UINT32_SZ + ssh->curSz;
+            if (alignSz % alignBlockSz != 0) {
+                WLOG(WS_LOG_DEBUG,
+                        "Packet not block aligned: size = %u, block = %u",
+                        alignSz, (word32)alignBlockSz);
+                ssh->error = WS_BUFFER_E;
+                return WS_FATAL_ERROR;
+            }
             ssh->processReplyState = PROCESS_PACKET_FINISH;
             FALL_THROUGH;
 

tests/regress.c

@@ -107,11 +107,10 @@ static HandshakeInfo* AllocHandshake(WOLFSSH* ssh)
 }
 
 /* Build a minimal SSH binary packet carrying only a message ID.
- * Layout: uint32 packetLen, byte padLen, payload[msgId], pad[padLen].
- * Choose padLen so total is 8-byte aligned for the clear transport case. */
+ * Layout: uint32 packetLen, byte padLen, payload[msgId], pad[padLen]. */
 static word32 BuildPacket(byte msgId, byte* out, word32 outSz)
 {
-    byte padLen = 6; /* 1 (msgId) +1 (padLen) +6 = 8 */
+    byte padLen = 10; /* 4 (len) +1 (padLen) +1 (msgId) +10 = 16 */
     word32 packetLen = 1 + 1 + padLen; /* payload + padLen field + pad */
     word32 need = 4 + packetLen;
 

tests/unit.c

@@ -1246,10 +1246,10 @@ static int test_OpenSshFormatNonCompositeRejected(void)
      !defined(WOLFSSH_NO_AES_GCM))
 
 /* Minimal SSH binary packet: uint32 length, padding_length, msgId, padding.
- * Same layout as tests/regress.c BuildPacket (8-byte aligned body). */
-static word32 BuildMacTestPacketPrefix(byte msgId, byte* out, word32 outSz)
+ * The caller picks padLen to get the alignment it wants. */
+static word32 BuildMacTestPacketPrefix(byte msgId, byte padLen,
+        byte* out, word32 outSz)
 {
-    byte padLen = 6;
     word32 packetLen = (word32)(1 + 1 + padLen);
     word32 need = UINT32_SZ + packetLen;
 
@@ -1284,7 +1284,7 @@ static int test_DoReceive_VerifyMacFailure(void)
     Hmac hmac;
     word32 prefixLen;
     word32 totalLen;
-    byte pkt[UINT32_SZ + 8 + MAX_HMAC_SZ];
+    byte pkt[UINT32_SZ + 12 + MAX_HMAC_SZ];
     int i;
     struct {
         byte macId;
@@ -1320,7 +1320,8 @@ static int test_DoReceive_VerifyMacFailure(void)
     WMEMSET(macKey, 0xA5, sizeof(macKey));
 
     for (i = 0; i < (int)(sizeof(cases) / sizeof(cases[0])); i++) {
-        prefixLen = BuildMacTestPacketPrefix(MSGID_IGNORE, pkt, sizeof(pkt));
+        prefixLen = BuildMacTestPacketPrefix(MSGID_IGNORE, 10,
+                pkt, sizeof(pkt));
         if (prefixLen == 0) {
             result = -202;
             goto done;
@@ -1412,8 +1413,8 @@ static int test_DoReceive_AeadTagFailure(void)
     int aesInited = 0;
     byte key[AES_256_KEY_SIZE];
     byte iv[GCM_NONCE_MID_SZ];
-    byte pkt[UINT32_SZ + 8];
-    byte record[UINT32_SZ + 8 + AES_BLOCK_SIZE];
+    byte pkt[UINT32_SZ + 16];
+    byte record[UINT32_SZ + 16 + AES_BLOCK_SIZE];
     word32 prefixLen, payloadSz, totalLen;
 
     ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
@@ -1428,7 +1429,8 @@ static int test_DoReceive_AeadTagFailure(void)
     WMEMSET(key, 0x5A, sizeof(key));
     WMEMSET(iv, 0x31, sizeof(iv));
 
-    prefixLen = BuildMacTestPacketPrefix(MSGID_IGNORE, pkt, sizeof(pkt));
+    prefixLen = BuildMacTestPacketPrefix(MSGID_IGNORE, 14,
+            pkt, sizeof(pkt));
     if (prefixLen == 0) {
         result = -222;
         goto done;
@@ -1466,7 +1468,7 @@ static int test_DoReceive_AeadTagFailure(void)
     ssh->decryptCipher.cipherType = ID_AES256_GCM;
     ssh->peerEncryptId = ID_AES256_GCM;
     ssh->peerAeadMode = 1;
-    ssh->peerBlockSz = UINT32_SZ;
+    ssh->peerBlockSz = AES_BLOCK_SIZE;
     ssh->peerMacSz = AES_BLOCK_SIZE;
     WMEMCPY(ssh->peerKeys.iv, iv, sizeof(iv));
     ssh->peerKeys.ivSz = sizeof(iv);
@@ -1685,11 +1687,11 @@ static int test_DoReceive_RejectsShortPadding(void)
     int ret;
     int result = 0;
     /* A well-formed MSGID_IGNORE packet carrying an empty string, but with
-     * padding_length = 1 (below MIN_PAD_LENGTH). Aside from the short padding
-     * the packet parses cleanly, so the padding check is the only thing that
-     * can reject it. Layout: uint32 packet_length=7, padding_length=1,
-     * msgId, uint32 string_len=0, 1 pad byte => 11 bytes total. */
-    byte pkt[11];
+     * padding_length = 1 (below MIN_PAD_LENGTH). The 16-byte total keeps the
+     * packet block aligned so the padding check is the only thing that can
+     * reject it. Layout: uint32 packet_length=12, padding_length=1, msgId,
+     * uint32 string_len=0, 6 pad bytes => 16 bytes total. */
+    byte pkt[16];
     word32 totalLen = (word32)sizeof(pkt);
 
     ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
@@ -1701,11 +1703,11 @@ static int test_DoReceive_RejectsShortPadding(void)
         return -761;
     }
 
-    pkt[0] = 0; pkt[1] = 0; pkt[2] = 0; pkt[3] = 7; /* packet_length */
+    WMEMSET(pkt, 0, sizeof(pkt));
+    pkt[3] = 12;            /* packet_length */
     pkt[4] = 1;             /* padding_length, below MIN_PAD_LENGTH (4) */
     pkt[5] = MSGID_IGNORE;
-    pkt[6] = 0; pkt[7] = 0; pkt[8] = 0; pkt[9] = 0; /* string_len = 0 */
-    pkt[10] = 0;            /* padding */
+                            /* string_len = 0, then padding, both zero */
 
     ssh->peerEncryptId = ID_NONE;
     ssh->peerAeadMode = 0;
@@ -1743,6 +1745,182 @@ static int test_DoReceive_RejectsShortPadding(void)
     return result;
 }
 
+
+/* Verify DoReceive rejects a cleartext binary packet whose length field is
+ * not block aligned. The packet is valid in every other respect, so the
+ * RFC 4253 section 6 alignment check is the only thing that can reject it. */
+static int test_DoReceive_RejectsMisalignedPacket(void)
+{
+    WOLFSSH_CTX* ctx = NULL;
+    WOLFSSH* ssh = NULL;
+    int ret;
+    int result = 0;
+    /* Layout: uint32 packet_length=10, padding_length=4, msgId,
+     * uint32 string_len=0, 4 pad bytes => 14 bytes total. 14 is not a
+     * multiple of the 8-byte minimum block size. */
+    byte pkt[14];
+    word32 totalLen = (word32)sizeof(pkt);
+
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
+    if (ctx == NULL)
+        return -770;
+    ssh = wolfSSH_new(ctx);
+    if (ssh == NULL) {
+        wolfSSH_CTX_free(ctx);
+        return -771;
+    }
+
+    WMEMSET(pkt, 0, sizeof(pkt));
+    pkt[3] = 10;            /* packet_length */
+    pkt[4] = MIN_PAD_LENGTH;
+    pkt[5] = MSGID_IGNORE;
+                            /* string_len = 0, then padding, both zero */
+
+    ssh->peerEncryptId = ID_NONE;
+    ssh->peerAeadMode = 0;
+    ssh->peerBlockSz = MIN_BLOCK_SZ;
+    ssh->peerMacId = ID_NONE;
+    ssh->peerMacSz = 0;
+    ssh->peerSeq = 0;
+    ssh->curSz = 0;
+    ssh->processReplyState = PROCESS_INIT;
+    ssh->error = 0;
+
+    ShrinkBuffer(&ssh->inputBuffer, 1);
+    ret = GrowBuffer(&ssh->inputBuffer, totalLen);
+    if (ret != WS_SUCCESS) {
+        result = -772;
+        goto done3;
+    }
+    WMEMCPY(ssh->inputBuffer.buffer, pkt, totalLen);
+    ssh->inputBuffer.length = totalLen;
+    ssh->inputBuffer.idx = 0;
+
+    ret = wolfSSH_TestDoReceive(ssh);
+    if (ret != WS_FATAL_ERROR) {
+        result = -773;
+        goto done3;
+    }
+    if (ssh->error != WS_BUFFER_E) {
+        result = -774;
+        goto done3;
+    }
+
+done3:
+    wolfSSH_free(ssh);
+    wolfSSH_CTX_free(ctx);
+    return result;
+}
+#endif /* WOLFSSH_TEST_INTERNAL */
+
+
+#if defined(WOLFSSH_TEST_INTERNAL) && !defined(WOLFSSH_NO_AES_GCM)
+/* Verify DoReceive rejects an AES-GCM record whose length field is not a
+ * multiple of the cipher block size. RFC 5647 aligns the body alone, so a
+ * 12-byte body is misaligned even though the tag over it is valid. */
+static int test_DoReceive_RejectsMisalignedAead(void)
+{
+    WOLFSSH_CTX* ctx = NULL;
+    WOLFSSH* ssh = NULL;
+    Aes encAes;
+    int ret;
+    int result = 0;
+    int aesInited = 0;
+    byte key[AES_256_KEY_SIZE];
+    byte iv[GCM_NONCE_MID_SZ];
+    /* Layout: uint32 packet_length=12, padding_length=6, msgId,
+     * uint32 string_len=0, 6 pad bytes => a 12-byte body, which is not a
+     * multiple of AES_BLOCK_SIZE. Valid in every other respect. */
+    byte pkt[UINT32_SZ + 12];
+    byte record[UINT32_SZ + 12 + AES_BLOCK_SIZE];
+    word32 prefixLen, payloadSz, totalLen;
+
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
+    if (ctx == NULL)
+        return -780;
+    ssh = wolfSSH_new(ctx);
+    if (ssh == NULL) {
+        wolfSSH_CTX_free(ctx);
+        return -781;
+    }
+
+    WMEMSET(key, 0x5A, sizeof(key));
+    WMEMSET(iv, 0x31, sizeof(iv));
+
+    WMEMSET(pkt, 0, sizeof(pkt));
+    pkt[3] = 12;            /* packet_length */
+    pkt[4] = 6;             /* padding_length */
+    pkt[5] = MSGID_IGNORE;
+                            /* string_len = 0, then padding, both zero */
+    prefixLen = (word32)sizeof(pkt);
+    payloadSz = prefixLen - UINT32_SZ;
+
+    ret = wc_AesInit(&encAes, NULL, INVALID_DEVID);
+    if (ret != 0) {
+        result = -783;
+        goto done4;
+    }
+    aesInited = 1;
+    if (wc_AesGcmSetKey(&encAes, key, sizeof(key)) != 0) {
+        result = -784;
+        goto done4;
+    }
+    WMEMCPY(record, pkt, UINT32_SZ);
+    if (wc_AesGcmEncrypt(&encAes, record + UINT32_SZ, pkt + UINT32_SZ,
+            payloadSz, iv, sizeof(iv), record + UINT32_SZ + payloadSz,
+            AES_BLOCK_SIZE, record, UINT32_SZ) != 0) {
+        result = -785;
+        goto done4;
+    }
+    totalLen = UINT32_SZ + payloadSz + AES_BLOCK_SIZE;
+
+    if (wc_AesInit(&ssh->decryptCipher.aes, ssh->ctx->heap, INVALID_DEVID) != 0
+            || wc_AesGcmSetKey(&ssh->decryptCipher.aes, key, sizeof(key)) != 0) {
+        result = -786;
+        goto done4;
+    }
+    ssh->decryptCipher.isInit = 1;
+    ssh->decryptCipher.cipherType = ID_AES256_GCM;
+    ssh->peerEncryptId = ID_AES256_GCM;
+    ssh->peerAeadMode = 1;
+    ssh->peerBlockSz = AES_BLOCK_SIZE;
+    ssh->peerMacSz = AES_BLOCK_SIZE;
+    WMEMCPY(ssh->peerKeys.iv, iv, sizeof(iv));
+    ssh->peerKeys.ivSz = sizeof(iv);
+    ssh->curSz = 0;
+    ssh->processReplyState = PROCESS_INIT;
+    ssh->error = 0;
+
+    ShrinkBuffer(&ssh->inputBuffer, 1);
+    if (GrowBuffer(&ssh->inputBuffer, totalLen) != WS_SUCCESS) {
+        result = -787;
+        goto done4;
+    }
+    WMEMCPY(ssh->inputBuffer.buffer, record, totalLen);
+    ssh->inputBuffer.length = totalLen;
+    ssh->inputBuffer.idx = 0;
+
+    ret = wolfSSH_TestDoReceive(ssh);
+    if (ret != WS_FATAL_ERROR) {
+        result = -788;
+        goto done4;
+    }
+    if (ssh->error != WS_BUFFER_E) {
+        result = -789;
+        goto done4;
+    }
+
+done4:
+    if (aesInited)
+        wc_AesFree(&encAes);
+    wolfSSH_free(ssh);
+    wolfSSH_CTX_free(ctx);
+    return result;
+}
+#endif /* WOLFSSH_TEST_INTERNAL && !WOLFSSH_NO_AES_GCM */
+
+
+#ifdef WOLFSSH_TEST_INTERNAL
 /* Send sink, so a test can run a real send path (build the packet, drain it)
  * without a live transport. */
 static int UnitIoSendSink(WOLFSSH* ssh, void* buf, word32 sz, void* ctx)
@@ -16164,6 +16342,20 @@ int wolfSSH_UnitTest(int argc, char** argv)
     testResult = testResult || unitResult;
 #endif
 
+#ifdef WOLFSSH_TEST_INTERNAL
+    unitResult = test_DoReceive_RejectsMisalignedPacket();
+    printf("DoReceiveRejectsMisalignedPacket: %s\n",
+            (unitResult == 0 ? "SUCCESS" : "FAILED"));
+    testResult = testResult || unitResult;
+#endif
+
+#if defined(WOLFSSH_TEST_INTERNAL) && !defined(WOLFSSH_NO_AES_GCM)
+    unitResult = test_DoReceive_RejectsMisalignedAead();
+    printf("DoReceiveRejectsMisalignedAead: %s\n",
+            (unitResult == 0 ? "SUCCESS" : "FAILED"));
+    testResult = testResult || unitResult;
+#endif
+
 #if defined(WOLFSSH_TEST_INTERNAL) && !defined(WOLFSSH_NO_DH_GEX_SHA256)
     unitResult = test_DhGexGroupValidate();
     printf("DhGexGroupValidate: %s\n",