Address code review

wolfSSL · Sep 20, 2024 · cf96ab2 · cf96ab2
1 parent 99a99e3
commit cf96ab2
Show file tree

Hide file tree

Showing 5 changed files with 103 additions and 110 deletions.
diff --git a/src/dtls.c b/src/dtls.c
@@ -1063,22 +1063,20 @@ static int DtlsCidGetSize(WOLFSSL* ssl, unsigned int* size, int rx)
     ConnectionID* id;
     CIDInfo* info;
 
-    if (ssl == NULL)
+    if (ssl == NULL || size == NULL)
         return BAD_FUNC_ARG;
 
     info = DtlsCidGetInfo(ssl);
     if (info == NULL)
         return WOLFSSL_FAILURE;
 
     id = rx ? info->rx : info->tx;
-    if (id == NULL || id->length == 0) {
-        if (size != NULL)
-            *size = 0;
-        return WOLFSSL_FAILURE;
+    if (id == NULL) {
+        *size = 0;
+        return WOLFSSL_SUCCESS;
     }
 
-    if (size != NULL)
-        *size = id->length;
+    *size = id->length;
     return WOLFSSL_SUCCESS;
 }
 
@@ -1234,46 +1232,42 @@ int TLSX_ConnectionID_Parse(WOLFSSL* ssl, const byte* input, word16 length,
         }
     }
 
+    if (length < OPAQUE8_LEN)
+        return BUFFER_ERROR;
+
+    cidSz = *input;
+    if (cidSz + OPAQUE8_LEN > length)
+        return BUFFER_ERROR;
+
     info = DtlsCidGetInfo(ssl);
     if (info == NULL)
         return BAD_STATE_E;
 
     /* it may happen if we process two ClientHello because the server sent an
      * HRR/HVR request */
-    if (info->tx != NULL) {
+    if (info->tx != NULL || info->negotiated) {
         if (ssl->options.side != WOLFSSL_SERVER_END &&
             ssl->options.serverState != SERVER_HELLO_RETRY_REQUEST_COMPLETE &&
             !IsSCR(ssl))
             return BAD_STATE_E;
 
-        if (!info->negotiated) {
-            XFREE(info->tx, ssl->heap, DYNAMIC_TYPE_TLSX);
-            info->tx = NULL;
-        }
-    }
-
-    if (length < OPAQUE8_LEN)
-        return BUFFER_ERROR;
-
-    cidSz = *input;
-    if (cidSz + OPAQUE8_LEN > length)
-        return BUFFER_ERROR;
+        /* Should not be null if negotiated */
+        if (info->tx == NULL)
+            return BAD_STATE_E;
 
-    if (cidSz > 0) {
-        if (!info->negotiated) {
-            ConnectionID* id = (ConnectionID*)XMALLOC(sizeof(*id) + cidSz,
-                    ssl->heap, DYNAMIC_TYPE_TLSX);
-            if (id == NULL)
-                return MEMORY_ERROR;
-            XMEMCPY(id->id, input + OPAQUE8_LEN, cidSz);
-            id->length = cidSz;
-            info->tx = id;
-        }
-        else {
-            /* For now we don't support changing the CID on a rehandshake */
-            if (XMEMCMP(info->tx->id, input + OPAQUE8_LEN, cidSz) != 0)
-                return DTLS_CID_ERROR;
-        }
+        /* For now we don't support changing the CID on a rehandshake */
+        if (cidSz != info->tx->length ||
+                XMEMCMP(info->tx->id, input + OPAQUE8_LEN, cidSz) != 0)
+            return DTLS_CID_ERROR;
+    }
+    else if (cidSz > 0) {
+        ConnectionID* id = (ConnectionID*)XMALLOC(sizeof(*id) + cidSz,
+                ssl->heap, DYNAMIC_TYPE_TLSX);
+        if (id == NULL)
+            return MEMORY_ERROR;
+        XMEMCPY(id->id, input + OPAQUE8_LEN, cidSz);
+        id->length = cidSz;
+        info->tx = id;
     }
 
     info->negotiated = 1;
@@ -1382,8 +1376,38 @@ int wolfSSL_dtls_cid_max_size(void)
 {
     return DTLS_CID_MAX_SIZE;
 }
-
 #endif /* WOLFSSL_DTLS_CID */
+
+byte DtlsGetCidTxSize(WOLFSSL* ssl)
+{
+#ifdef WOLFSSL_DTLS_CID
+    unsigned int cidSz;
+    int ret;
+    ret = wolfSSL_dtls_cid_get_tx_size(ssl, &cidSz);
+    if (ret != WOLFSSL_SUCCESS)
+        return 0;
+    return (byte)cidSz;
+#else
+    (void)ssl;
+    return 0;
+#endif
+}
+
+byte DtlsGetCidRxSize(WOLFSSL* ssl)
+{
+#ifdef WOLFSSL_DTLS_CID
+    unsigned int cidSz;
+    int ret;
+    ret = wolfSSL_dtls_cid_get_rx_size(ssl, &cidSz);
+    if (ret != WOLFSSL_SUCCESS)
+        return 0;
+    return (byte)cidSz;
+#else
+    (void)ssl;
+    return 0;
+#endif
+}
+
 #endif /* WOLFSSL_DTLS */
 
 #endif /* WOLFCRYPT_ONLY */
diff --git a/src/dtls13.c b/src/dtls13.c
@@ -1054,25 +1054,6 @@ static WC_INLINE word8 Dtls13GetEpochBits(w64wrapper epoch)
 }
 
 #ifdef WOLFSSL_DTLS_CID
-static byte Dtls13GetCidTxSize(WOLFSSL* ssl)
-{
-    unsigned int cidSz;
-    int ret;
-    ret = wolfSSL_dtls_cid_get_tx_size(ssl, &cidSz);
-    if (ret != WOLFSSL_SUCCESS)
-        return 0;
-    return (byte)cidSz;
-}
-
-static byte Dtls13GetCidRxSize(WOLFSSL* ssl)
-{
-    unsigned int cidSz;
-    int ret;
-    ret = wolfSSL_dtls_cid_get_rx_size(ssl, &cidSz);
-    if (ret != WOLFSSL_SUCCESS)
-        return 0;
-    return (byte)cidSz;
-}
 
 static int Dtls13AddCID(WOLFSSL* ssl, byte* flags, byte* out, word16* idx)
 {
@@ -1082,7 +1063,7 @@ static int Dtls13AddCID(WOLFSSL* ssl, byte* flags, byte* out, word16* idx)
     if (!wolfSSL_dtls_cid_is_enabled(ssl))
         return 0;
 
-    cidSz = Dtls13GetCidTxSize(ssl);
+    cidSz = DtlsGetCidTxSize(ssl);
 
     /* no cid */
     if (cidSz == 0)
@@ -1138,8 +1119,6 @@ static int Dtls13UnifiedHeaderParseCID(WOLFSSL* ssl, byte flags,
 
 #else
 #define Dtls13AddCID(a, b, c, d) 0
-#define Dtls13GetCidRxSize(a) 0
-#define Dtls13GetCidTxSize(a) 0
 #define Dtls13UnifiedHeaderParseCID(a, b, c, d, e) 0
 #endif /* WOLFSSL_DTLS_CID */
 
@@ -1245,7 +1224,7 @@ int Dtls13EncryptRecordNumber(WOLFSSL* ssl, byte* hdr, word16 recordLength)
 
     seqLength = (*hdr & DTLS13_LEN_BIT) ? DTLS13_SEQ_16_LEN : DTLS13_SEQ_8_LEN;
 
-    cidSz = Dtls13GetCidTxSize(ssl);
+    cidSz = DtlsGetCidTxSize(ssl);
     /* header flags + seq number + CID size*/
     hdrLength = OPAQUE8_LEN + seqLength + cidSz;
 
@@ -1276,7 +1255,7 @@ word16 Dtls13GetRlHeaderLength(WOLFSSL* ssl, byte isEncrypted)
     if (!isEncrypted)
         return DTLS_RECORD_HEADER_SZ;
 
-    return DTLS13_UNIFIED_HEADER_SIZE + Dtls13GetCidTxSize(ssl);
+    return DTLS13_UNIFIED_HEADER_SIZE + DtlsGetCidTxSize(ssl);
 }
 
 /**
@@ -1403,7 +1382,7 @@ int Dtls13GetUnifiedHeaderSize(WOLFSSL* ssl, const byte input, word16* size)
         return BAD_FUNC_ARG;
 
     /* flags (1) + CID + seq 8bit (1) */
-    *size = OPAQUE8_LEN + Dtls13GetCidRxSize(ssl) + OPAQUE8_LEN;
+    *size = OPAQUE8_LEN + DtlsGetCidRxSize(ssl) + OPAQUE8_LEN;
     if (input & DTLS13_SEQ_LEN_BIT)
         *size += OPAQUE8_LEN;
     if (input & DTLS13_LEN_BIT)