mqtt: verify Remaining Length for CONNACK and PUBACK

Verified in test 1132

Closes #20513

lib/mqtt.c

@@ -408,10 +408,15 @@ static CURLcode mqtt_verify_connack(struct Curl_easy *data)
   DEBUGASSERT(mq);
   if(!mq)
     return CURLE_FAILED_INIT;
+  if(mq->remaining_length != 2) {
+    failf(data, "CONNACK expected Remaining Length 2, got %zu",
+          mq->remaining_length);
+    return CURLE_WEIRD_SERVER_REPLY;
+  }
 
   result = mqtt_recv_atleast(data, MQTT_CONNACK_LEN);
   if(result)
-    goto fail;
+    return result;
 
   /* verify CONNACK */
   DEBUGASSERT(curlx_dyn_len(&mq->recvbuf) >= MQTT_CONNACK_LEN);
@@ -422,12 +427,10 @@ static CURLcode mqtt_verify_connack(struct Curl_easy *data)
     failf(data, "Expected %02x%02x but got %02x%02x",
           0x00, 0x00, ptr[0], ptr[1]);
     curlx_dyn_reset(&mq->recvbuf);
-    result = CURLE_WEIRD_SERVER_REPLY;
-    goto fail;
+    return CURLE_WEIRD_SERVER_REPLY;
   }
   mqtt_recv_consume(data, MQTT_CONNACK_LEN);
-fail:
-  return result;
+  return CURLE_OK;
 }
 
 static CURLcode mqtt_get_topic(struct Curl_easy *data,
@@ -511,6 +514,12 @@ static CURLcode mqtt_verify_suback(struct Curl_easy *data)
   if(!mqtt || !mq)
     return CURLE_FAILED_INIT;
 
+  if(mq->remaining_length != 3) {
+    failf(data, "SUBACK expected Remaining Length 3, got %zu",
+          mq->remaining_length);
+    return CURLE_WEIRD_SERVER_REPLY;
+  }
+
   result = mqtt_recv_atleast(data, MQTT_SUBACK_LEN);
   if(result)
     goto fail;

tests/data/Makefile.am

@@ -151,7 +151,7 @@ test1093 test1094 test1095 test1096 test1097 test1098 test1099 test1100 \
 test1101 test1102 test1103 test1104 test1105 test1106 test1107 test1108 \
 test1109 test1110 test1111 test1112 test1113 test1114 test1115 test1116 \
 test1117 test1118 test1119 test1120 test1121 test1122 test1123 test1124 \
-test1125 test1126 test1127 test1128 test1129 test1130 test1131 \
+test1125 test1126 test1127 test1128 test1129 test1130 test1131 test1132 \
 test1133 test1134 test1135 test1136 test1137 test1138 test1139 test1140 \
 test1141 test1142 test1143 test1144 test1145 test1146 test1147 test1148 \
 test1149 test1150 test1151 test1152 test1153 test1154 test1155 test1156 \

tests/data/test1132

@@ -0,0 +1,59 @@
+<?xml version="1.0" encoding="US-ASCII"?>
+<testcase>
+<info>
+<keywords>
+MQTT
+MQTT CONNACK
+</keywords>
+</info>
+
+# Server-side
+<reply>
+<data nocheck="yes">
+hello
+</data>
+<datacheck hex="yes">
+00 04 31 31 39 30   68 65 6c 6c 6f 5b 4c 46 5d 0a
+</datacheck>
+
+# Remaining Length must be 2 in a CONNACK
+<servercmd>
+remlen-CONNACK 3
+</servercmd>
+</reply>
+
+# Client-side
+<client>
+<features>
+mqtt
+</features>
+<server>
+mqtt
+</server>
+<name>
+MQTT CONNACK with bad Remaining Length
+</name>
+<command option="binary-trace">
+mqtt://%HOSTIP:%MQTTPORT/%TESTNUMBER
+</command>
+</client>
+
+# Verify data after the test has been "shot"
+<verify>
+# These are hexadecimal protocol dumps from the client
+# Strip out the random part of the client id from the CONNECT message
+# before comparison
+<strippart>
+s/^(.* 00044d5154540402003c000c6375726c).*/$1/
+</strippart>
+<protocol>
+client CONNECT 18 00044d5154540402003c000c6375726c
+server CONNACK 3 20030000
+</protocol>
+
+# 8 == CURLE_WEIRD_SERVER_REPLY
+<errorcode>
+8
+</errorcode>
+</verify>
+</testcase>

tests/server/mqttd.c

@@ -43,13 +43,14 @@
 #define MQTT_MSG_DISCONNECT 0xe0
 
 struct mqttd_configurable {
+  int testnum;
   unsigned char version; /* initial version byte in the request must match
                             this */
   bool publish_before_suback;
   bool short_publish;
   bool excessive_remaining;
   unsigned char error_connack;
-  int testnum;
+  unsigned char remlen_connack;
 };
 
 #define REQUEST_DUMP   "server.input"
@@ -65,6 +66,7 @@ static void mqttd_resetdefaults(void)
   m_config.short_publish = FALSE;
   m_config.excessive_remaining = FALSE;
   m_config.error_connack = 0;
+  m_config.remlen_connack = 0;
   m_config.testnum = 0;
 }
 
@@ -103,6 +105,13 @@ static void mqttd_getconfig(void)
             logmsg("error-CONNACK = %d", m_config.error_connack);
           }
         }
+        else if(!strcmp(key, "remlen-CONNACK")) {
+          pval = value;
+          if(!curlx_str_number(&pval, &num, 0xff)) {
+            m_config.remlen_connack = (unsigned char)num;
+            logmsg("remlen-CONNACK = %d", m_config.remlen_connack);
+          }
+        }
         else if(!strcmp(key, "Testnum")) {
           pval = value;
           if(!curlx_str_number(&pval, &num, INT_MAX)) {
@@ -158,13 +167,16 @@ static int connack(FILE *dump, curl_socket_t fd)
   };
   ssize_t rc;
 
+  if(m_config.remlen_connack)
+    packet[1] = m_config.remlen_connack;
   packet[3] = m_config.error_connack;
 
   rc = swrite(fd, packet, sizeof(packet));
   if(rc > 0) {
     logmsg("WROTE %zd bytes [CONNACK]", rc);
     loghex(packet, rc);
-    logprotocol(FROM_SERVER, "CONNACK", 2, dump, packet, sizeof(packet));
+    logprotocol(FROM_SERVER, "CONNACK", packet[1], dump,
+                packet, sizeof(packet));
   }
   if(rc == sizeof(packet)) {
     return 0;