Merge pull request #11163 from kareem-wolfssl/vanessa

Fix a few sniffer issues.  Document DES function size requirements.

Author: philljj

src/sniffer.c

@@ -478,8 +478,10 @@ typedef struct Flags {
 
 /* Out of Order FIN capture */
 typedef struct FinCapture {
-    word32 cliFinSeq;               /* client relative sequence FIN  0 is no */
-    word32 srvFinSeq;               /* server relative sequence FIN, 0 is no */
+    word32 cliFinSeq;               /* client relative sequence FIN (may be 0) */
+    word32 srvFinSeq;               /* server relative sequence FIN (may be 0) */
+    byte   cliHasFin;               /* client FIN captured (seq value may be 0) */
+    byte   srvHasFin;               /* server FIN captured (seq value may be 0) */
     byte   cliCounted;              /* did we count yet, detects duplicates */
     byte   srvCounted;              /* did we count yet, detects duplicates */
 } FinCapture;
@@ -5612,6 +5614,11 @@ static int CheckHeaders(IpInfo* ipInfo, TcpInfo* tcpInfo, const byte* packet,
      * data after the IP record for the FCS for Ethernet. */
     *sslBytes = (int)(packet + ipInfo->total - *sslFrame);
 
+    if (*sslBytes < 0) {
+        SetError(PACKET_HDR_SHORT_STR, error, NULL, 0);
+        return WOLFSSL_FATAL_ERROR;
+    }
+
     /* Ensure sslBytes does not exceed the actual size. */
     if (*sslBytes > (int)(length - (ipInfo->length + tcpInfo->length))) {
         SetError(PACKET_HDR_SHORT_STR, error, NULL, 0);
@@ -5821,12 +5828,16 @@ static int AddToReassembly(byte from, word32 seq, const byte* sslFrame,
 static int AddFinCapture(SnifferSession* session, word32 sequence)
 {
     if (session->flags.side == WOLFSSL_SERVER_END) {
-        if (session->finCapture.cliCounted == 0)
+        if (session->finCapture.cliCounted == 0) {
             session->finCapture.cliFinSeq = sequence;
+            session->finCapture.cliHasFin = 1;
+        }
     }
     else {
-        if (session->finCapture.srvCounted == 0)
+        if (session->finCapture.srvCounted == 0) {
             session->finCapture.srvFinSeq = sequence;
+            session->finCapture.srvHasFin = 1;
+        }
     }
     return 1;
 }
@@ -5837,6 +5848,7 @@ static int AdjustSequence(TcpInfo* tcpInfo, SnifferSession* session,
                           int* sslBytes, const byte** sslFrame, char* error)
 {
     int ret = 0;
+    sword32 seqDiff;
     word32  seqStart = (session->flags.side == WOLFSSL_SERVER_END) ?
                                     session->cliSeqStart : session->srvSeqStart;
     word32* seqLast = (session->flags.side == WOLFSSL_SERVER_END) ?
@@ -5854,12 +5866,17 @@ static int AdjustSequence(TcpInfo* tcpInfo, SnifferSession* session,
     if (tcpInfo->sequence < seqStart)
         real = 0xffffffffU - seqStart + tcpInfo->sequence + 1;
 
+    /* Relative sequence numbers wrap at 2^32, so order them with signed
+     * (RFC 1982) serial-number arithmetic; plain unsigned </> mis-handles
+     * the wrap boundary and would drop wrapped segments as already-seen. */
+    seqDiff = (sword32)(real - *expected);
+
     TraceRelativeSequence(*expected, real);
 
-    if (real < *expected) {
+    if (seqDiff < 0) {
         int overlap = *expected - real;
 
-        if (real + *sslBytes > *expected) {
+        if ((sword32)(real + (word32)*sslBytes - *expected) > 0) {
         #ifdef WOLFSSL_ASYNC_CRYPT
             if (session->sslServer->error != WC_NO_ERR_TRACE(WC_PENDING_E) &&
                 session->pendSeq != tcpInfo->sequence)
@@ -5901,7 +5918,7 @@ static int AdjustSequence(TcpInfo* tcpInfo, SnifferSession* session,
                 }
             }
             else if (*sslBytes > 0) {
-                if (real + *sslBytes - 1 > *seqLast) {
+                if ((sword32)(real + (word32)*sslBytes - 1 - *seqLast) > 0) {
                     /* fix segment overlap */
                 #ifdef DEBUG_SNIFFER
                     WOLFSSL* ssl = (session->flags.side == WOLFSSL_SERVER_END) ?
@@ -5931,7 +5948,7 @@ static int AdjustSequence(TcpInfo* tcpInfo, SnifferSession* session,
                     session->sslServer->error != WC_NO_ERR_TRACE(WC_PENDING_E) &&
                     session->pendSeq != tcpInfo->sequence &&
                 #endif
-                    real + *sslBytes -1 <= *seqLast) {
+                    (sword32)(real + (word32)*sslBytes - 1 - *seqLast) <= 0) {
                     Trace(DUPLICATE_STR);
                     ret = 1;
                 }
@@ -5947,7 +5964,7 @@ static int AdjustSequence(TcpInfo* tcpInfo, SnifferSession* session,
             }
         }
     }
-    else if (real > *expected) {
+    else if (seqDiff > 0) {
         Trace(OUT_OF_ORDER_STR);
         if (*sslBytes > 0) {
             int addResult = AddToReassembly(session->flags.side, real,
@@ -6141,7 +6158,13 @@ static int CheckAck(TcpInfo* tcpInfo, SnifferSession* session)
 
         TraceAck(real, expected);
 
-        if (real > expected)
+        /* Relative sequence numbers wrap at 2^32; compare with signed
+         * (RFC 1982) serial-number arithmetic to avoid a false positive at
+         * the wrap boundary. Expected is still 0 when that side's SYN was
+         * never seen, leaving no base to be relative to, so any data being
+         * ACKed there is data we missed. */
+        if ((expected == 0) ? (real != 0) :
+                ((sword32)(real - expected) > 0))
             return WOLFSSL_FATAL_ERROR;  /* we missed a packet, ACKing data we never saw */
     }
     return 0;
@@ -6180,6 +6203,12 @@ static int CheckSequence(IpInfo* ipInfo, TcpInfo* tcpInfo,
 
     /* adjust potential ethernet trailer */
     actualLen = ipInfo->total - ipInfo->length - tcpInfo->length;
+    /* CheckHeaders already rejects this for the current callers; kept so the
+     * clamp below cannot be reached with a negative bound. */
+    if (actualLen < 0) {
+        SetError(PACKET_HDR_SHORT_STR, error, session, FATAL_ERROR_STATE);
+        return WOLFSSL_FATAL_ERROR;
+    }
     if (*sslBytes > actualLen) {
         *sslBytes = actualLen;
     }
@@ -6725,17 +6754,23 @@ static int CheckFinCapture(IpInfo* ipInfo, TcpInfo* tcpInfo,
                             SnifferSession* session)
 {
     int ret = 0;
-    if (session->finCapture.cliFinSeq && session->finCapture.cliFinSeq <=
-                                         session->cliExpected) {
+    /* FIN sequences are relative and wrap at 2^32, so compare "reached" with
+     * signed (RFC 1982) serial-number arithmetic. A dedicated has-FIN flag
+     * marks capture, since a relative sequence of 0 is itself a valid FIN
+     * position at the wrap boundary. */
+    if (session->finCapture.cliHasFin &&
+            (sword32)(session->finCapture.cliFinSeq - session->cliExpected)
+                <= 0) {
         if (session->finCapture.cliCounted == 0) {
             session->flags.finCount += 1;
             session->finCapture.cliCounted = 1;
             TraceClientFin(session->finCapture.cliFinSeq, session->cliExpected);
         }
     }
 
-    if (session->finCapture.srvFinSeq && session->finCapture.srvFinSeq <=
-                                         session->srvExpected) {
+    if (session->finCapture.srvHasFin &&
+            (sword32)(session->finCapture.srvFinSeq - session->srvExpected)
+                <= 0) {
         if (session->finCapture.srvCounted == 0) {
             session->flags.finCount += 1;
             session->finCapture.srvCounted = 1;

src/ssl_crypto.c

@@ -2672,6 +2672,12 @@ WOLFSSL_DES_LONG wolfSSL_DES_cbc_cksum(const unsigned char* in,
  *       we are padding the last block. This is not a padding API.
  * TODO: Validate parameters?
  *
+ * A length that is not a multiple of DES_BLOCK_SIZE is rounded up to a whole
+ * block: on encrypt the trailing partial block is 0 padded and a full block is
+ * written to output, and on decrypt a full block is read from input. Both
+ * buffers must therefore hold length rounded up to DES_BLOCK_SIZE, not just
+ * length bytes.
+ *
  * @param [in]  input     Data to encipher.
  * @param [out] output    Enciphered data.
  * @param [in]  length    Length of data to encipher.
@@ -2740,6 +2746,10 @@ void wolfSSL_DES_cbc_encrypt(const unsigned char* input, unsigned char* output,
  *       we are padding the last block. This is not a padding API.
  * TODO: Validate parameters?
  *
+ * A length that is not a multiple of DES_BLOCK_SIZE is rounded up to a whole
+ * block, and the new IV is taken from that last whole block. Both buffers must
+ * therefore hold length rounded up to DES_BLOCK_SIZE, not just length bytes.
+ *
  * @param [in]      input     Data to encipher.
  * @param [out]     output    Enciphered data.
  * @param [in]      length    Length of data to encipher.
@@ -2792,10 +2802,18 @@ void wolfSSL_DES_ncbc_encrypt(const unsigned char* input, unsigned char* output,
  *       we are padding the last block. This is not a padding API.
  * TODO: Validate parameters?
  *
+ * A size that is not a multiple of DES_BLOCK_SIZE is rounded up to a whole
+ * block: on encrypt the trailing partial block is 0 padded and a full block is
+ * written to output, and on decrypt a full block is read from input. Both
+ * buffers must therefore hold sz rounded up to DES_BLOCK_SIZE, not just sz
+ * bytes.
+ *
  * @param [in]      input     Data to encipher.
  * @param [out]     output    Enciphered data.
- * @param [in]      length    Length of data to encipher.
- * @param [in]      schedule  Key schedule.
+ * @param [in]      sz        Length of data to encipher.
+ * @param [in]      ks1       First key schedule.
+ * @param [in]      ks2       Second key schedule.
+ * @param [in]      ks3       Third key schedule.
  * @param [in, out] ivec      IV for CBC operation.
  * @param [in]      enc       Whether to encrypt.
  */