Address code review

wolfSSL · Sep 20, 2024 · bf946d2 · bf946d2
1 parent d7ab338
commit bf946d2
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)