diff --git a/examples/firmware/fwpush.c b/examples/firmware/fwpush.c index 31c09b26b..841ea8865 100644 --- a/examples/firmware/fwpush.c +++ b/examples/firmware/fwpush.c @@ -50,6 +50,7 @@ #include #endif +#include #include "fwpush.h" #include "firmware.h" #include "examples/mqttexample.h" @@ -84,51 +85,58 @@ static int mqtt_message_cb(MqttClient *client, MqttMessage *msg, static int mqtt_publish_cb(MqttPublish *publish) { int ret = -1; #if !defined(NO_FILESYSTEM) - size_t bytes_read; FwpushCBdata *cbData; FirmwareHeader *header; word32 headerSize; + int avail, chunk; /* Structure was stored in ctx pointer */ if (publish != NULL) { cbData = (FwpushCBdata*)publish->ctx; - if (cbData != NULL) { + if (cbData != NULL && cbData->fwBuf != NULL) { header = (FirmwareHeader*)cbData->data; + /* Guard a corrupt offset so avail/chunk can never go negative and + * feed a negative length back into the publish state machine. */ + if (cbData->fwOffset < 0 || cbData->fwOffset > cbData->fwLen) { + return -1; + } + avail = cbData->fwLen - cbData->fwOffset; - /* Check for first iteration of callback */ - if (cbData->fp == NULL) { - /* Get FW size from FW header struct */ + /* Stream from the in-memory signed firmware rather than reopening + * the file, so the bytes sent are exactly the bytes that were + * signed (no file-swap TOCTOU between build and send). */ + if (!cbData->headerSent) { headerSize = sizeof(FirmwareHeader) + header->sigLen + header->pubKeyLen; if (headerSize > publish->buffer_len) { PRINTF("Error: Firmware Header %d larger than max buffer %d", - headerSize, publish->buffer_len); + (int)headerSize, publish->buffer_len); return -1; } - - /* Copy header to buffer */ XMEMCPY(publish->buffer, header, headerSize); - - /* Open file */ - cbData->fp = fopen(cbData->filename, "rb"); - if (cbData->fp != NULL) { - /* read a buffer of data from the file */ - bytes_read = fread(&publish->buffer[headerSize], - 1, publish->buffer_len - headerSize, cbData->fp); - if (bytes_read != 0) { - ret = (int)bytes_read + headerSize; - } + chunk = publish->buffer_len - (int)headerSize; + if (chunk > avail) { + chunk = avail; } + if (chunk > 0) { + XMEMCPY(&publish->buffer[headerSize], + &cbData->fwBuf[cbData->fwOffset], chunk); + cbData->fwOffset += chunk; + } + cbData->headerSent = 1; + ret = (int)headerSize + chunk; } else { - /* read a buffer of data from the file */ - bytes_read = fread(publish->buffer, 1, publish->buffer_len, - cbData->fp); - ret = (int)bytes_read; - } - if (cbData->fp && feof(cbData->fp)) { - fclose(cbData->fp); - cbData->fp = NULL; + chunk = publish->buffer_len; + if (chunk > avail) { + chunk = avail; + } + if (chunk > 0) { + XMEMCPY(publish->buffer, + &cbData->fwBuf[cbData->fwOffset], chunk); + cbData->fwOffset += chunk; + } + ret = chunk; } } } @@ -139,7 +147,7 @@ static int mqtt_publish_cb(MqttPublish *publish) { } static int fw_message_build(MQTTCtx *mqttCtx, const char* fwFile, - byte **p_msgBuf, int *p_msgLen) + byte **p_msgBuf, int *p_msgLen, byte **p_fwBuf, int *p_fwLen) { int rc; byte *msgBuf = NULL, *sigBuf = NULL, *keyBuf = NULL, *fwBuf = NULL; @@ -232,6 +240,12 @@ static int fw_message_build(MQTTCtx *mqttCtx, const char* fwFile, #endif /* Assemble message */ + /* guard the int msgLen sum against overflow on huge firmware */ + if (fwLen > INT_MAX - (int)(sizeof(FirmwareHeader) + sigLen + keyLen)) { + PRINTF("Firmware file too large: %d bytes", fwLen); + rc = EXIT_FAILURE; + goto exit; + } msgLen = sizeof(FirmwareHeader) + sigLen + keyLen + fwLen; /* The firmware will be copied by the callback */ @@ -265,6 +279,14 @@ static int fw_message_build(MQTTCtx *mqttCtx, const char* fwFile, } if (p_msgLen) *p_msgLen = msgLen; + + /* Transfer ownership of the signed firmware buffer so the callback + * streams the exact signed bytes instead of re-reading the file. */ + if (p_fwBuf) { + *p_fwBuf = fwBuf; + fwBuf = NULL; + } + if (p_fwLen) *p_fwLen = fwLen; } else { if (msgBuf) WOLFMQTT_FREE(msgBuf); @@ -445,7 +467,8 @@ int fwpush_test(MQTTCtx *mqttCtx) cbData->filename = mqttCtx->pub_file; rc = fw_message_build(mqttCtx, cbData->filename, &cbData->data, - (int*)&mqttCtx->publish.total_len); + (int*)&mqttCtx->publish.total_len, + &cbData->fwBuf, &cbData->fwLen); /* The publish->ctx is available for use by the application to pass * data to the callback routine. */ @@ -539,6 +562,7 @@ int fwpush_test(MQTTCtx *mqttCtx) if (cbData) { if (cbData->fp) fclose(cbData->fp); if (cbData->data) WOLFMQTT_FREE(cbData->data); + if (cbData->fwBuf) WOLFMQTT_FREE(cbData->fwBuf); WOLFMQTT_FREE(cbData); } if (mqttCtx->publish.buffer) WOLFMQTT_FREE(mqttCtx->publish.buffer); diff --git a/examples/firmware/fwpush.h b/examples/firmware/fwpush.h index f837f9c26..b5898ac29 100644 --- a/examples/firmware/fwpush.h +++ b/examples/firmware/fwpush.h @@ -33,6 +33,10 @@ typedef struct FwpushCBdata_s { const char *filename; byte *data; FILE *fp; + byte *fwBuf; /* signed firmware held in memory, streamed by the callback */ + int fwLen; + int fwOffset; + int headerSent; } FwpushCBdata; /* Exposed functions */ diff --git a/examples/mqttclient/mqttclient.c b/examples/mqttclient/mqttclient.c index 6b2d421b2..215379a32 100644 --- a/examples/mqttclient/mqttclient.c +++ b/examples/mqttclient/mqttclient.c @@ -143,11 +143,15 @@ static int mqtt_property_cb(MqttClient *client, MqttProp *head, void *ctx) /* Store client ID in global */ mqttCtx->client_id = &gClientId[0]; - /* Store assigned client ID from CONNACK*/ - XSTRNCPY((char*)mqttCtx->client_id, prop->data_str.str, - MAX_CLIENT_ID_LEN - 1); - /* should use strlcpy() semantics, but non-portable */ - ((char*)mqttCtx->client_id)[MAX_CLIENT_ID_LEN - 1] = '\0'; + /* copy bounded by decoded len; data_str.str is not NUL-terminated */ + { + word16 idLen = prop->data_str.len; + if (idLen >= MAX_CLIENT_ID_LEN) { + idLen = MAX_CLIENT_ID_LEN - 1; + } + XMEMCPY((char*)mqttCtx->client_id, prop->data_str.str, idLen); + ((char*)mqttCtx->client_id)[idLen] = '\0'; + } break; case MQTT_PROP_SUBSCRIPTION_ID_AVAIL: diff --git a/examples/nbclient/nbclient.c b/examples/nbclient/nbclient.c index 1efbd3cbf..dbd1ef9e8 100644 --- a/examples/nbclient/nbclient.c +++ b/examples/nbclient/nbclient.c @@ -132,11 +132,15 @@ static int mqtt_property_cb(MqttClient *client, MqttProp *head, void *ctx) /* Store client ID in global */ mqttCtx->client_id = &gClientId[0]; - /* Store assigned client ID from CONNACK*/ - XSTRNCPY((char*)mqttCtx->client_id, prop->data_str.str, - MAX_CLIENT_ID_LEN - 1); - /* should use strlcpy() semantics, but non-portable */ - ((char*)mqttCtx->client_id)[MAX_CLIENT_ID_LEN - 1] = '\0'; + /* copy bounded by decoded len; data_str.str is not NUL-terminated */ + { + word16 idLen = prop->data_str.len; + if (idLen >= MAX_CLIENT_ID_LEN) { + idLen = MAX_CLIENT_ID_LEN - 1; + } + XMEMCPY((char*)mqttCtx->client_id, prop->data_str.str, idLen); + ((char*)mqttCtx->client_id)[idLen] = '\0'; + } break; case MQTT_PROP_SUBSCRIPTION_ID_AVAIL: diff --git a/examples/pub-sub/mqtt-pub.c b/examples/pub-sub/mqtt-pub.c index 6c92a77db..abf313022 100644 --- a/examples/pub-sub/mqtt-pub.c +++ b/examples/pub-sub/mqtt-pub.c @@ -75,11 +75,15 @@ static int mqtt_property_cb(MqttClient *client, MqttProp *head, void *ctx) /* Store client ID in global */ mqttCtx->client_id = &gClientId[0]; - /* Store assigned client ID from CONNACK*/ - XSTRNCPY((char*)mqttCtx->client_id, prop->data_str.str, - MAX_CLIENT_ID_LEN - 1); - /* should use strlcpy() semantics, but non-portable */ - ((char*)mqttCtx->client_id)[MAX_CLIENT_ID_LEN - 1] = '\0'; + /* copy bounded by decoded len; data_str.str is not NUL-terminated */ + { + word16 idLen = prop->data_str.len; + if (idLen >= MAX_CLIENT_ID_LEN) { + idLen = MAX_CLIENT_ID_LEN - 1; + } + XMEMCPY((char*)mqttCtx->client_id, prop->data_str.str, idLen); + ((char*)mqttCtx->client_id)[idLen] = '\0'; + } break; case MQTT_PROP_SUBSCRIPTION_ID_AVAIL: diff --git a/examples/pub-sub/mqtt-sub.c b/examples/pub-sub/mqtt-sub.c index c2905a1a4..dcffc628f 100644 --- a/examples/pub-sub/mqtt-sub.c +++ b/examples/pub-sub/mqtt-sub.c @@ -145,11 +145,15 @@ static int mqtt_property_cb(MqttClient *client, MqttProp *head, void *ctx) /* Store client ID in global */ mqttCtx->client_id = &gClientId[0]; - /* Store assigned client ID from CONNACK*/ - XSTRNCPY((char*)mqttCtx->client_id, prop->data_str.str, - MAX_CLIENT_ID_LEN - 1); - /* should use strlcpy() semantics, but non-portable */ - ((char*)mqttCtx->client_id)[MAX_CLIENT_ID_LEN - 1] = '\0'; + /* copy bounded by decoded len; data_str.str is not NUL-terminated */ + { + word16 idLen = prop->data_str.len; + if (idLen >= MAX_CLIENT_ID_LEN) { + idLen = MAX_CLIENT_ID_LEN - 1; + } + XMEMCPY((char*)mqttCtx->client_id, prop->data_str.str, idLen); + ((char*)mqttCtx->client_id)[idLen] = '\0'; + } break; case MQTT_PROP_SUBSCRIPTION_ID_AVAIL: diff --git a/src/mqtt_broker.c b/src/mqtt_broker.c index 917a4420e..9f555ef32 100644 --- a/src/mqtt_broker.c +++ b/src/mqtt_broker.c @@ -245,13 +245,36 @@ static int BrokerBufCompare(const byte* a, int len_a, return result; } +/* Length of a NUL-terminated string bounded to max_len, computed with a + * fixed number of iterations so an attacker cannot infer a server secret's + * length from timing. Never reads past the terminating NUL (the read index + * is clamped once NUL is seen), so it is safe on strings shorter than + * max_len. Returns max_len for strings that reach max_len without a NUL. */ +static int BrokerStrLenBounded(const char* s, int max_len) +{ + int len = 0; + int idx = 0; + int done = 0; + int i; + for (i = 0; i < max_len; i++) { + int atNul = ((byte)s[idx] == 0); + done |= atNul; + len += (done == 0); + idx += (done == 0); + } + return len; +} + /* Constant-time C-string comparison wrapper. Both inputs are assumed to * be NUL-terminated UTF-8 (e.g. configured username, decoded username - * field which is rejected if it contains U+0000). */ + * field which is rejected if it contains U+0000). Lengths are computed with + * BrokerStrLenBounded so the server secret's length is not leaked by a + * variable-time XSTRLEN. */ static int BrokerStrCompare(const char* a, const char* b, int cmp_len) { - return BrokerBufCompare((const byte*)a, (int)XSTRLEN(a), - (const byte*)b, (int)XSTRLEN(b), cmp_len); + return BrokerBufCompare((const byte*)a, BrokerStrLenBounded(a, cmp_len), + (const byte*)b, BrokerStrLenBounded(b, cmp_len), + cmp_len); } #endif /* WOLFMQTT_BROKER_AUTH */ @@ -1595,10 +1618,12 @@ static void BrokerOutPub_Free(BrokerOutPub* e) return; } if (e->topic != NULL) { + BROKER_FORCE_ZERO(e->topic, XSTRLEN(e->topic) + 1); WOLFMQTT_FREE(e->topic); e->topic = NULL; } if (e->payload != NULL) { + BROKER_FORCE_ZERO(e->payload, e->payload_len); WOLFMQTT_FREE(e->payload); e->payload = NULL; } @@ -2033,8 +2058,8 @@ static void BrokerClient_Free(BrokerClient* bc) (void)BrokerNetDisconnect(bc); MqttClient_DeInit(&bc->client); #ifdef WOLFMQTT_STATIC_MEMORY - XMEMSET(bc, 0, sizeof(*bc)); - /* in_use is now 0 after memset */ + BROKER_FORCE_ZERO(bc, sizeof(*bc)); + /* in_use is now 0 after zeroization */ #else if (bc->client_id) { WOLFMQTT_FREE(bc->client_id); @@ -3377,7 +3402,7 @@ static void BrokerRetained_Delete(MqttBroker* broker, const char* topic) XSTRCMP(broker->retained[i].topic, topic) == 0) { WBLOG_DBG(broker, "broker: retained delete topic=%s", BrokerLog_Sanitize(topic)); - XMEMSET(&broker->retained[i], 0, sizeof(BrokerRetainedMsg)); + BROKER_FORCE_ZERO(&broker->retained[i], sizeof(BrokerRetainedMsg)); found = 1; break; } @@ -3404,8 +3429,10 @@ static void BrokerRetained_Delete(MqttBroker* broker, const char* topic) else { broker->retained = next; } + BROKER_FORCE_ZERO(cur->topic, XSTRLEN(cur->topic) + 1); WOLFMQTT_FREE(cur->topic); if (cur->payload) { + BROKER_FORCE_ZERO(cur->payload, cur->payload_len); WOLFMQTT_FREE(cur->payload); } WOLFMQTT_FREE(cur); @@ -3439,15 +3466,17 @@ static void BrokerRetained_FreeAll(MqttBroker* broker) #ifdef WOLFMQTT_STATIC_MEMORY for (i = 0; i < BROKER_MAX_RETAINED; i++) { - XMEMSET(&broker->retained[i], 0, sizeof(BrokerRetainedMsg)); + BROKER_FORCE_ZERO(&broker->retained[i], sizeof(BrokerRetainedMsg)); } #else while (cur) { BrokerRetainedMsg* next = cur->next; if (cur->topic) { + BROKER_FORCE_ZERO(cur->topic, XSTRLEN(cur->topic) + 1); WOLFMQTT_FREE(cur->topic); } if (cur->payload) { + BROKER_FORCE_ZERO(cur->payload, cur->payload_len); WOLFMQTT_FREE(cur->payload); } WOLFMQTT_FREE(cur); @@ -3476,8 +3505,9 @@ static void BrokerClient_ClearWill(BrokerClient* bc) bc->will_retain = 0; bc->will_delay_sec = 0; #ifdef WOLFMQTT_STATIC_MEMORY + BROKER_FORCE_ZERO(bc->will_topic, sizeof(bc->will_topic)); + BROKER_FORCE_ZERO(bc->will_payload, sizeof(bc->will_payload)); bc->will_payload_len = 0; - bc->will_topic[0] = '\0'; #else if (bc->will_topic) { BROKER_FORCE_ZERO(bc->will_topic, XSTRLEN(bc->will_topic) + 1); @@ -3631,7 +3661,7 @@ static void BrokerPendingWill_Cancel(MqttBroker* broker, XSTRCMP(broker->pending_wills[i].client_id, client_id) == 0) { WBLOG_DBG(broker, "broker: will cancelled client_id=%s", BrokerLog_Sanitize(client_id)); - XMEMSET(&broker->pending_wills[i], 0, + BROKER_FORCE_ZERO(&broker->pending_wills[i], sizeof(BrokerPendingWill)); return; } @@ -3678,7 +3708,7 @@ static void BrokerPendingWill_FreeAll(MqttBroker* broker) return; } #ifdef WOLFMQTT_STATIC_MEMORY - XMEMSET(broker->pending_wills, 0, sizeof(broker->pending_wills)); + BROKER_FORCE_ZERO(broker->pending_wills, sizeof(broker->pending_wills)); #else pw = broker->pending_wills; while (pw) { @@ -3732,7 +3762,7 @@ static int BrokerPendingWill_Process(MqttBroker* broker) (unsigned)pw->payload_len); BrokerClient_PublishWillImmediate(broker, pw->topic, pw->payload, pw->payload_len, pw->qos, pw->retain); - XMEMSET(pw, 0, sizeof(BrokerPendingWill)); + BROKER_FORCE_ZERO(pw, sizeof(BrokerPendingWill)); activity = 1; } } @@ -4064,6 +4094,7 @@ static void BrokerClient_PublishWillImmediate(MqttBroker* broker, if (enc_rc > 0) { (void)MqttPacket_Write(&sub->client->client, sub->client->tx_buf, enc_rc); + BROKER_FORCE_ZERO(sub->client->tx_buf, enc_rc); } } #ifndef WOLFMQTT_STATIC_MEMORY @@ -4608,7 +4639,7 @@ static int BrokerHandle_Connect(BrokerClient* bc, int rx_len, #endif bc->password_len == 0 || BrokerBufCompare((const byte*)broker->auth_pass, - (int)XSTRLEN(broker->auth_pass), + BrokerStrLenBounded(broker->auth_pass, BROKER_MAX_PASSWORD_LEN), (const byte*)bc->password, (int)bc->password_len, BROKER_MAX_PASSWORD_LEN) != 0)) { auth_ok = 0; @@ -5018,16 +5049,32 @@ static int BrokerHandle_Subscribe(BrokerClient* bc, int rx_len, } #ifdef WOLFMQTT_BROKER_RETAINED else { - /* Deliver retained messages matching this filter */ + /* Deliver retained messages matching this filter. */ char filter_z[BROKER_MAX_FILTER_LEN]; word16 copy_len = flen; - if (copy_len >= BROKER_MAX_FILTER_LEN) { - copy_len = BROKER_MAX_FILTER_LEN - 1; + #ifndef WOLFMQTT_STATIC_MEMORY + /* Dynamic builds store filters longer than the stack buffer in + * full; use a heap copy so retained matching uses the same + * filter as future PUBLISH fan-out instead of a truncation. + * Static builds reject over-length filters at subscribe time. */ + if (flen >= BROKER_MAX_FILTER_LEN) { + char* filter_dyn = (char*)WOLFMQTT_MALLOC(flen + 1); + if (filter_dyn != NULL) { + XMEMCPY(filter_dyn, f, flen); + filter_dyn[flen] = '\0'; + BrokerRetained_DeliverToClient(broker, bc, filter_dyn, + topic_qos); + WOLFMQTT_FREE(filter_dyn); + } + } + else + #endif + if (copy_len < BROKER_MAX_FILTER_LEN) { + XMEMCPY(filter_z, f, copy_len); + filter_z[copy_len] = '\0'; + BrokerRetained_DeliverToClient(broker, bc, filter_z, + topic_qos); } - XMEMCPY(filter_z, f, copy_len); - filter_z[copy_len] = '\0'; - BrokerRetained_DeliverToClient(broker, bc, filter_z, - topic_qos); } #endif } @@ -5573,6 +5620,9 @@ static int BrokerHandle_PublishRel(BrokerClient* bc, int rx_len) { int rc; MqttPublishResp resp; +#ifdef WOLFMQTT_V5 + int was_tracked; +#endif XMEMSET(&resp, 0, sizeof(resp)); #ifdef WOLFMQTT_V5 @@ -5586,6 +5636,9 @@ static int BrokerHandle_PublishRel(BrokerClient* bc, int rx_len) return rc; } +#ifdef WOLFMQTT_V5 + was_tracked = BrokerInboundQos2_Contains(bc, resp.packet_id); +#endif /* [MQTT-4.3.3] QoS 2 step 3: discard the stored Packet Identifier so a * later PUBLISH with the same ID is treated as a fresh delivery. PUBREL * for an unknown ID is idempotent - we still PUBCOMP it. */ @@ -5595,7 +5648,9 @@ static int BrokerHandle_PublishRel(BrokerClient* bc, int rx_len) if (resp.props) { (void)MqttProps_Free(resp.props); } - resp.reason_code = MQTT_REASON_SUCCESS; + /* [MQTT-3.7.2.1] Report 0x92 when the broker had no record of this ID. */ + resp.reason_code = was_tracked ? + MQTT_REASON_SUCCESS : MQTT_REASON_PACKET_ID_NOT_FOUND; resp.props = NULL; #endif rc = MqttEncode_PublishResp(bc->tx_buf, BROKER_CLIENT_TX_SZ(bc), @@ -6523,12 +6578,11 @@ int MqttBroker_Free(MqttBroker* broker) defined(WOLFMQTT_BROKER_PERSIST_ENCRYPT) /* Zero the cached AES key on teardown. ForceZero so the compiler * cannot elide the wipe (plain XMEMSET on a value that becomes - * dead-on-return is at the compiler's discretion). */ - if (broker->persist_key_loaded) { - BROKER_FORCE_ZERO(broker->persist_key_cache, - sizeof(broker->persist_key_cache)); - broker->persist_key_loaded = 0; - } + * dead-on-return is at the compiler's discretion). Unconditional: a + * failed/partial derive_key can leave bytes without setting the flag. */ + BROKER_FORCE_ZERO(broker->persist_key_cache, + sizeof(broker->persist_key_cache)); + broker->persist_key_loaded = 0; #endif return MQTT_CODE_SUCCESS; diff --git a/src/mqtt_broker_persist.c b/src/mqtt_broker_persist.c index f25e722c3..de22f1f0e 100644 --- a/src/mqtt_broker_persist.c +++ b/src/mqtt_broker_persist.c @@ -242,6 +242,19 @@ static int wmqb_get_key(MqttBroker* broker) * rekeying only when persist_key_cache changes would amortize the AES * key schedule. Requires deciding the threading model first - today * the broker is single-threaded so a single shared context is safe. */ +/* Volatile wipe the compiler cannot elide, for plaintext about to be freed. */ +static void wmqb_force_zero(void* mem, word32 len) +{ + volatile byte* p = (volatile byte*)mem; + if (p == NULL) { + return; + } + while (len > 0) { + *p++ = 0; + len--; + } +} + static int wmqb_encrypt_blob(MqttBroker* broker, const byte* plain, word32 plain_len, byte** ct_out, word32* ct_out_len) { @@ -445,6 +458,7 @@ static int wmqb_iter_decrypt_cb(const byte* key, word16 key_len, return d->inner_cb(key, key_len, blob, 0, d->inner_ctx); } stop = d->inner_cb(key, key_len, plain, plain_len, d->inner_ctx); + wmqb_force_zero(plain, plain_len); WOLFMQTT_FREE(plain); return stop; } @@ -508,11 +522,13 @@ static int wmqb_kv_get_decrypt_meta(MqttBroker* broker, byte ns, return rc; } if (plain_len > *inout_len) { + wmqb_force_zero(plain, plain_len); WOLFMQTT_FREE(plain); return MQTT_CODE_ERROR_OUT_OF_BUFFER; } XMEMCPY(out, plain, plain_len); *inout_len = plain_len; + wmqb_force_zero(plain, plain_len); WOLFMQTT_FREE(plain); return 0; } @@ -599,6 +615,9 @@ static int wmqb_put_session_record(MqttBroker* broker, const char* cid, rc = wmqb_kv_put_commit(broker, BROKER_PERSIST_NS_SESSION, (const byte*)cid, cid_len, buf, total_len); +#ifdef WOLFMQTT_BROKER_PERSIST_ENCRYPT + wmqb_force_zero(buf, total_len); +#endif WOLFMQTT_FREE(buf); return rc; } @@ -782,6 +801,9 @@ int BrokerPersist_PutSubs(MqttBroker* broker, const char* client_id) rc = wmqb_kv_put_commit(broker, BROKER_PERSIST_NS_SUBS, (const byte*)client_id, cid_len, buf, total_len); +#ifdef WOLFMQTT_BROKER_PERSIST_ENCRYPT + wmqb_force_zero(buf, total_len); +#endif WOLFMQTT_FREE(buf); return rc; } @@ -855,6 +877,9 @@ int BrokerPersist_PutRetained(MqttBroker* broker, rc = wmqb_kv_put_commit(broker, BROKER_PERSIST_NS_RETAINED, (const byte*)topic, topic_len, buf, total_len); +#ifdef WOLFMQTT_BROKER_PERSIST_ENCRYPT + wmqb_force_zero(buf, total_len); +#endif WOLFMQTT_FREE(buf); return rc; } @@ -975,6 +1000,9 @@ int BrokerPersist_PutOutPub(MqttBroker* broker, const char* client_id, rc = wmqb_kv_put_commit(broker, BROKER_PERSIST_NS_OUTQ, key, key_len, buf, total_len); +#ifdef WOLFMQTT_BROKER_PERSIST_ENCRYPT + wmqb_force_zero(buf, total_len); +#endif WOLFMQTT_FREE(buf); return rc; #endif /* WOLFMQTT_STATIC_MEMORY */ diff --git a/src/mqtt_client.c b/src/mqtt_client.c index ec39f455b..1b8a91b49 100644 --- a/src/mqtt_client.c +++ b/src/mqtt_client.c @@ -1674,7 +1674,8 @@ int MqttClient_Init(MqttClient *client, MqttNet* net, /* Check arguments */ if (client == NULL || tx_buf == NULL || tx_buf_len <= 0 || - rx_buf == NULL || rx_buf_len <= 0) { + rx_buf == NULL || rx_buf_len <= 0 || + cmd_timeout_ms < 0) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } @@ -1738,10 +1739,10 @@ void MqttClient_DeInit(MqttClient *client) (void)wm_SemFree(&client->lockCURL); #endif #endif - } #ifdef WOLFMQTT_V5 - (void)MqttProps_ShutDown(); + (void)MqttProps_ShutDown(); #endif + } } #ifdef WOLFMQTT_DISCONNECT_CB @@ -1782,8 +1783,8 @@ int MqttClient_Connect(MqttClient *client, MqttConnect *mc_connect) } if (mc_connect->stat.write == MQTT_MSG_BEGIN) { - #ifdef DEBUG_WOLFMQTT /* Warn if credentials are being sent without TLS */ + #ifdef WOLFMQTT_DEBUG_CLIENT if ((mc_connect->username != NULL || mc_connect->password != NULL) && !(MqttClient_Flags(client, 0, 0) & MQTT_CLIENT_FLAG_IS_TLS)) { PRINTF("Warning: MQTT credentials are being sent without TLS"); @@ -2235,7 +2236,7 @@ static int MqttPublishMsg(MqttClient *client, MqttPublish *publish, /* Validate publish request against server properties */ if ((publish->qos > client->max_qos) || - ((publish->retain == 1) && (client->retain_avail == 0))) + ((publish->retain != 0) && (client->retain_avail == 0))) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_SERVER_PROP); } @@ -2552,15 +2553,22 @@ int MqttClient_Subscribe(MqttClient *client, MqttSubscribe *subscribe) * that filter. */ if (rc == MQTT_CODE_SUCCESS) { byte any_rejected = 0; - for (i = 0; i < subscribe->topic_count && i < MAX_MQTT_TOPICS; i++) { - topic = &subscribe->topics[i]; - topic->return_code = subscribe->ack.return_codes[i]; - if (topic->return_code & MQTT_SUBSCRIBE_ACK_CODE_FAILURE) { - any_rejected = 1; - } + /* [MQTT-3.9.3-1] a SUBACK carries exactly one reason code per + * requested topic; too few would be read as granted QoS 0 (fail-open). */ + if (subscribe->ack.return_code_count != subscribe->topic_count) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); } - if (any_rejected) { - rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_SUBSCRIBE_REJECTED); + else { + for (i = 0; i < subscribe->topic_count && i < MAX_MQTT_TOPICS; i++) { + topic = &subscribe->topics[i]; + topic->return_code = subscribe->ack.return_codes[i]; + if (topic->return_code & MQTT_SUBSCRIBE_ACK_CODE_FAILURE) { + any_rejected = 1; + } + } + if (any_rejected) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_SUBSCRIBE_REJECTED); + } } } @@ -2833,6 +2841,10 @@ int MqttClient_Disconnect_ex(MqttClient *client, MqttDisconnect *p_disconnect) MQTT_PACKET_TYPE_DISCONNECT, 0, 0); #endif if (rc <= 0) { + /* Encode failed: tx_buf may hold partial v5 DISCONNECT property + * data. Zero the full buffer before MqttWriteStop releases + * lockSend so no other thread can see residual data. */ + CLIENT_FORCE_ZERO(client->tx_buf, client->tx_buf_len); MqttWriteStop(client, &disconnect->stat); return rc; } @@ -2851,10 +2863,15 @@ int MqttClient_Disconnect_ex(MqttClient *client, MqttDisconnect *p_disconnect) && client->write.total > 0 #endif ) { - /* keep send locked and return early */ + /* keep send locked and return early (tx_buf still holds property + * data until the write completes) */ return rc; } #endif + /* Clear tx_buf to remove any v5 DISCONNECT property data BEFORE + * MqttWriteStop releases lockSend, so another thread cannot race in and + * populate tx_buf before it is scrubbed. */ + CLIENT_FORCE_ZERO(client->tx_buf, xfer); MqttWriteStop(client, &disconnect->stat); if (rc == xfer) { rc = MQTT_CODE_SUCCESS; @@ -3099,6 +3116,10 @@ int MqttClient_CancelMessage(MqttClient *client, MqttObject* msg) #ifdef WOLFMQTT_DEBUG_CLIENT PRINTF("Cancel Write Lock"); #endif + /* An abandoned write (e.g. a partial nonblocking CONNECT) leaves the + * encoded packet - possibly plaintext credentials - in tx_buf. Scrub + * it before MqttWriteStop releases lockSend. */ + CLIENT_FORCE_ZERO(client->tx_buf, client->tx_buf_len); MqttWriteStop(client, mms_stat); } diff --git a/src/mqtt_packet.c b/src/mqtt_packet.c index fc541e495..8b5ea3156 100644 --- a/src/mqtt_packet.c +++ b/src/mqtt_packet.c @@ -250,6 +250,27 @@ int MqttPacket_SubAckReturnCodeValid(byte code, byte protocol_level) return 0; } +#ifdef WOLFMQTT_V5 +/* [MQTT-4.8.0-1] Validate a v5 UNSUBACK Reason Code against the fixed set in + * MQTT 5.0 section 3.11.3. Returns 1 if allowed, 0 if reserved. */ +static int MqttPacket_UnsubAckReasonCodeValid(byte code) +{ + switch (code) { + case MQTT_REASON_SUCCESS: /* 0x00 */ + case MQTT_REASON_NO_SUB_EXIST: /* 0x11 */ + case MQTT_REASON_UNSPECIFIED_ERR: /* 0x80 */ + case MQTT_REASON_IMPL_SPECIFIC_ERR: /* 0x83 */ + case MQTT_REASON_NOT_AUTHORIZED: /* 0x87 */ + case MQTT_REASON_TOPIC_FILTER_INVALID: /* 0x8F */ + case MQTT_REASON_PACKET_ID_IN_USE: /* 0x91 */ + return 1; + default: + break; + } + return 0; +} +#endif + /* Validate an MQTT Topic Filter against the syntax rules from * [MQTT-4.7.3-1] (minimum length one character), [MQTT-4.7.1-2] * (multi-level wildcard '#' must be either the whole filter or directly @@ -367,6 +388,7 @@ static int MqttDecode_FixedHeader(byte *rx_buf, int rx_buf_len, int *remain_len, byte type, MqttQoS *p_qos, byte *p_retain, byte *p_duplicate) { int header_len; + word32 remain_len_u32 = 0; MqttPacket* header = (MqttPacket*)rx_buf; /* Every MQTT packet is at least 2 bytes: the type/flags byte plus at @@ -377,11 +399,15 @@ static int MqttDecode_FixedHeader(byte *rx_buf, int rx_buf_len, int *remain_len, if (rx_buf_len < 2) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); } - header_len = MqttDecode_Vbi(header->len, (word32*)remain_len, + header_len = MqttDecode_Vbi(header->len, &remain_len_u32, (word32)(rx_buf_len - 1)); if (header_len < 0) { return header_len; } + if (remain_len_u32 > MQTT_PACKET_MAX_REMAIN_LEN) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + } + *remain_len = (int)remain_len_u32; /* Validate packet type */ if (MQTT_PACKET_TYPE_GET(header->type_flags) != type) { @@ -675,9 +701,15 @@ int MqttEncode_Data(byte *buf, const byte *data, word16 data_len) { int len = MqttEncode_Num(buf, data_len); - if ((buf != NULL) && (data != NULL)) { + if (buf != NULL) { buf += len; - XMEMCPY(buf, data, data_len); + if (data != NULL) { + XMEMCPY(buf, data, data_len); + } + else { + /* never emit stale tx_buf for a declared-length field */ + XMEMSET(buf, 0, data_len); + } } return len + data_len; } @@ -733,6 +765,11 @@ int MqttEncode_Props(MqttPacketType packet, MqttProp* props, byte* buf) } case MQTT_DATA_TYPE_SHORT: { + /* [MQTT-3.3.2-7] A Topic Alias of 0 is a Protocol Error. */ + if (cur_prop->type == MQTT_PROP_TOPIC_ALIAS && + cur_prop->data_short == 0) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY); + } tmp = MqttEncode_Num(buf, cur_prop->data_short); rc += tmp; if (buf != NULL) { @@ -762,6 +799,12 @@ int MqttEncode_Props(MqttPacketType packet, MqttProp* props, byte* buf) } case MQTT_DATA_TYPE_VAR_INT: { + /* [MQTT-3.8.2.1.2] A Subscription Identifier of 0 is a + * Protocol Error. */ + if (cur_prop->type == MQTT_PROP_SUBSCRIPTION_ID && + cur_prop->data_int == 0) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY); + } tmp = MqttEncode_Vbi(buf, cur_prop->data_int); if (tmp < 0) { return tmp; @@ -834,6 +877,7 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, int total, tmp; int prop_count = 0; word32 seen_lo = 0, seen_hi = 0; /* singleton-property duplicate guard */ + word32 prop_type; MqttProp* cur_prop; byte* buf = pbuf; @@ -860,11 +904,13 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, break; } /* Decode the Identifier */ - rc = MqttDecode_Vbi(buf, (word32*)&cur_prop->type, + prop_type = 0; + rc = MqttDecode_Vbi(buf, &prop_type, (word32)(buf_len - (buf - pbuf))); if (rc < 0) { break; } + cur_prop->type = (MqttPropertyType)prop_type; tmp = rc; buf += tmp; total += tmp; @@ -917,6 +963,13 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, tmp++; total++; prop_len--; + /* [MQTT-3.1.2-28/29] Request Response/Problem Information + * MUST be 0 or 1; any other value is a Protocol Error. */ + if ((cur_prop->type == MQTT_PROP_REQ_RESP_INFO || + cur_prop->type == MQTT_PROP_REQ_PROB_INFO) && + cur_prop->data_byte > 1) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY); + } break; } case MQTT_DATA_TYPE_SHORT: @@ -931,6 +984,10 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, rc = tmp; break; } + if ((word32)tmp > prop_len) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + break; + } buf += tmp; total += tmp; prop_len -= (word32)tmp; @@ -976,6 +1033,10 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, rc = tmp; } else if ((word32)tmp <= (buf_len - (buf - pbuf))) { + if ((word32)tmp > prop_len) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + break; + } buf += tmp; total += tmp; prop_len -= (word32)tmp; @@ -1006,6 +1067,10 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, rc = tmp; break; } + if ((word32)tmp > prop_len) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + break; + } buf += tmp; total += tmp; prop_len -= (word32)tmp; @@ -1027,11 +1092,19 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, rc = tmp; break; } + if ((word32)tmp > prop_len) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + break; + } buf += tmp; total += tmp; prop_len -= tmp; if (cur_prop->data_bin.len <= (buf_len - (buf - pbuf))) { + if (cur_prop->data_bin.len > prop_len) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + break; + } cur_prop->data_bin.data = buf; buf += cur_prop->data_bin.len; total += (int)cur_prop->data_bin.len; @@ -1055,6 +1128,10 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, rc = tmp; } else if ((word32)tmp <= (buf_len - (buf - pbuf))) { + if ((word32)tmp > prop_len) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + break; + } buf += tmp; total += tmp; prop_len -= (word32)tmp; @@ -1068,6 +1145,11 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, } else if ((word32)tmp <= (buf_len - (buf - pbuf))) { + if ((word32)tmp > prop_len) { + rc = MQTT_TRACE_ERROR( + MQTT_CODE_ERROR_MALFORMED_DATA); + break; + } buf += tmp; total += tmp; prop_len -= (word32)tmp; @@ -1126,6 +1208,11 @@ int MqttEncode_Connect(byte *tx_buf, int tx_buf_len, MqttConnect *mc_connect) return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } + /* 0 means use build default; normalize so wire level and v5 prop gating agree */ + if (mc_connect->protocol_level == 0) { + mc_connect->protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL; + } + /* [MQTT-3.1.2-22]: If the User Name Flag is set to 0, the Password Flag * MUST be set to 0 */ if (mc_connect->password != NULL && mc_connect->username == NULL) { @@ -1180,6 +1267,14 @@ int MqttEncode_Connect(byte *tx_buf, int tx_buf_len, MqttConnect *mc_connect) if (str_len > (size_t)0xFFFF) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } + /* [MQTT-3.1.3.3] Will Topic is a Topic Name: no wildcards */ + if (!MqttPacket_TopicNameValid(mc_connect->lwt_msg->topic_name, + (word16)str_len, mc_connect->protocol_level)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + } + if (mc_connect->lwt_msg->qos > MQTT_QOS_2) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); + } remain_len += (int)str_len; remain_len += MQTT_DATA_LEN_SIZE; @@ -1748,6 +1843,11 @@ int MqttEncode_ConnectAck(byte *tx_buf, int tx_buf_len, return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } + /* [MQTT-3.2.2-1] Connect Acknowledge Flags bits 1-7 are reserved (0) */ + if (connect_ack->flags & 0xFE) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); + } + /* Determine packet length */ remain_len = 2; /* flags + return code */ #ifdef WOLFMQTT_V5 @@ -1824,6 +1924,9 @@ int MqttEncode_Publish(byte *tx_buf, int tx_buf_len, MqttPublish *publish, if (tx_buf == NULL || publish == NULL) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } + if (publish->qos > MQTT_QOS_2) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); + } /* MQTT UTF-8 strings are limited to 65535 bytes [MQTT-1.5.3]. Check here * before writing the fixed header so a later MqttEncode_String failure * cannot corrupt tx_payload via `tx_payload += -1`. NULL topic_name @@ -1832,6 +1935,10 @@ int MqttEncode_Publish(byte *tx_buf, int tx_buf_len, MqttPublish *publish, { size_t str_len; byte level = 0; + #ifdef WOLFMQTT_V5 + MqttProp* prop_iter; + int has_topic_alias = 0; + #endif if (publish->topic_name == NULL) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } @@ -1849,6 +1956,22 @@ int MqttEncode_Publish(byte *tx_buf, int tx_buf_len, MqttPublish *publish, (word16)str_len, level)) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); } + #ifdef WOLFMQTT_V5 + /* [MQTT-3.3.2-8] An empty v5 Topic Name requires a Topic Alias so the + * decoder (and any conformant broker) can resolve the topic. */ + if (level >= MQTT_CONNECT_PROTOCOL_LEVEL_5 && str_len == 0) { + for (prop_iter = publish->props; prop_iter != NULL; + prop_iter = prop_iter->next) { + if (prop_iter->type == MQTT_PROP_TOPIC_ALIAS) { + has_topic_alias = 1; + break; + } + } + if (!has_topic_alias) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); + } + } + #endif /* Determine packet length */ variable_len = (int)str_len + MQTT_DATA_LEN_SIZE; @@ -1886,6 +2009,15 @@ int MqttEncode_Publish(byte *tx_buf, int tx_buf_len, MqttPublish *publish, } #endif + /* Reject inconsistent length fields: a non-empty payload buffer paired + * with a zero total_len would silently emit an empty PUBLISH (which on a + * retained topic deletes the stored message). An intentional empty + * publish leaves buffer NULL or buffer_len 0. */ + if ((publish->buffer != NULL) && (publish->buffer_len > 0) && + (publish->total_len == 0)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); + } + if (((publish->buffer != NULL) || (use_cb == 1)) && (publish->total_len > 0)) { payload_len = publish->total_len; @@ -2132,6 +2264,11 @@ int MqttEncode_PublishResp(byte* tx_buf, int tx_buf_len, byte type, return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } + /* [MQTT-2.3.1-1] PUBACK/PUBREC/PUBREL/PUBCOMP require a non-zero packet id */ + if (publish_resp->packet_id == 0) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PACKET_ID); + } + /* Determine packet length */ remain_len = MQTT_DATA_LEN_SIZE; /* For packet_id */ @@ -2342,6 +2479,11 @@ int MqttEncode_Subscribe(byte *tx_buf, int tx_buf_len, return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PACKET_ID); } + /* [MQTT-3.8.3-3] SUBSCRIBE must carry at least one topic filter */ + if (subscribe->topic_count <= 0) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); + } + /* Determine packet length */ remain_len = MQTT_DATA_LEN_SIZE; /* For packet_id */ for (i = 0; i < subscribe->topic_count; i++) { @@ -2352,6 +2494,15 @@ int MqttEncode_Subscribe(byte *tx_buf, int tx_buf_len, if (str_len > (size_t)0xFFFF) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } + if (topic->qos > MQTT_QOS_2) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); + } + /* Reject filters the decoder would treat as malformed so the + * encoder cannot emit an undecodable SUBSCRIBE [MQTT-4.7]. */ + if (!MqttPacket_TopicFilterValid(topic->topic_filter, + (word16)str_len)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + } remain_len += (int)str_len + MQTT_DATA_LEN_SIZE; remain_len++; /* For QoS */ } @@ -2673,6 +2824,8 @@ int MqttDecode_SubscribeAck(byte* rx_buf, int rx_buf_len, if (payload_len > buf_remain) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); } + subscribe_ack->return_code_count = + (word16)((payload_len > 0xFFFF) ? 0xFFFF : payload_len); if (payload_len > MAX_MQTT_TOPICS) payload_len = MAX_MQTT_TOPICS; #ifdef WOLFMQTT_V5 @@ -2724,6 +2877,11 @@ int MqttEncode_Unsubscribe(byte *tx_buf, int tx_buf_len, return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PACKET_ID); } + /* [MQTT-3.10.3-2] UNSUBSCRIBE must carry at least one topic filter */ + if (unsubscribe->topic_count <= 0) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); + } + /* Determine packet length */ remain_len = MQTT_DATA_LEN_SIZE; /* For packet_id */ for (i = 0; i < unsubscribe->topic_count; i++) { @@ -2734,6 +2892,12 @@ int MqttEncode_Unsubscribe(byte *tx_buf, int tx_buf_len, if (str_len > (size_t)0xFFFF) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } + /* Reject filters the decoder would treat as malformed so the + * encoder cannot emit an undecodable UNSUBSCRIBE [MQTT-4.7]. */ + if (!MqttPacket_TopicFilterValid(topic->topic_filter, + (word16)str_len)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + } remain_len += (int)str_len + MQTT_DATA_LEN_SIZE; } else { @@ -2982,6 +3146,7 @@ int MqttDecode_UnsubscribeAck(byte *rx_buf, int rx_buf_len, * here would push rx_payload past this packet and underflow * reason_code_count to a huge word16. */ byte* packet_end = &rx_buf[header_len + remain_len]; + word16 rc_idx; if (remain_len > MQTT_DATA_LEN_SIZE) { word32 props_len = 0; @@ -3018,6 +3183,20 @@ int MqttDecode_UnsubscribeAck(byte *rx_buf, int rx_buf_len, unsubscribe_ack->reason_codes = rx_payload; unsubscribe_ack->reason_code_count = (word16)(packet_end - rx_payload); + + /* [MQTT-4.8.0-1] Reject reserved UNSUBACK reason codes, mirroring + * the SUBACK path; free any decoded v5 props before returning. */ + for (rc_idx = 0; rc_idx < unsubscribe_ack->reason_code_count; + rc_idx++) { + if (!MqttPacket_UnsubAckReasonCodeValid( + unsubscribe_ack->reason_codes[rc_idx])) { + if (unsubscribe_ack->props != NULL) { + (void)MqttProps_Free(unsubscribe_ack->props); + unsubscribe_ack->props = NULL; + } + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + } + } } #endif } @@ -3355,6 +3534,10 @@ int MqttDecode_Disconnect(byte *rx_buf, int rx_buf_len, MqttDisconnect *disc) } } } + else { + /* [MQTT-3.14.2.1] Remaining Length < 1 means Normal Disconnection */ + disc->reason_code = MQTT_REASON_SUCCESS; + } (void)rx_payload; @@ -3367,12 +3550,26 @@ int MqttEncode_Auth(byte *tx_buf, int tx_buf_len, MqttAuth *auth) int header_len, remain_len = 0; byte* tx_payload; int props_len = 0; + MqttProp* auth_prop; /* Validate required arguments */ if ((tx_buf == NULL) || (tx_buf_len <= 0) || (auth == NULL)) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } + /* [MQTT-4.12] AUTH (REAUTH/CONT_AUTH) must carry an Authentication Method + * property; reject if the list is empty or lacks it. */ + if ((auth->reason_code == MQTT_REASON_CONT_AUTH) || + (auth->reason_code == MQTT_REASON_REAUTH)) { + auth_prop = auth->props; + while (auth_prop != NULL && auth_prop->type != MQTT_PROP_AUTH_METHOD) { + auth_prop = auth_prop->next; + } + if (auth_prop == NULL) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); + } + } + /* Length of Reason Code */ remain_len++; @@ -3403,8 +3600,10 @@ int MqttEncode_Auth(byte *tx_buf, int tx_buf_len, MqttAuth *auth) } tx_payload = &tx_buf[header_len]; - /* Encode variable header */ - if ((auth->reason_code == MQTT_REASON_CONT_AUTH) || + /* Encode variable header. [MQTT-3.15.2.1] Success (0x00) is valid too and + * is accepted by MqttDecode_Auth, so encode it to keep the roundtrip. */ + if ((auth->reason_code == MQTT_REASON_SUCCESS) || + (auth->reason_code == MQTT_REASON_CONT_AUTH) || (auth->reason_code == MQTT_REASON_REAUTH)) { *tx_payload++ = auth->reason_code; @@ -3724,6 +3923,7 @@ int MqttPacket_Read(MqttClient *client, byte* rx_buf, int rx_buf_len, case MQTT_PK_READ_HEAD: { int i; + word32 remain_len_u32 = 0; client->packet.stat = MQTT_PK_READ_HEAD; for (i = (client->packet.header_len - MQTT_PACKET_HEADER_MIN_SIZE); @@ -3735,6 +3935,14 @@ int MqttPacket_Read(MqttClient *client, byte* rx_buf, int rx_buf_len, break; } + /* [MQTT-1.5.5-1] A VBI is at most 4 bytes: if the last allowed + * byte still has the continuation bit set, reject without + * reading a spurious 5th byte from the socket. */ + if (i == MQTT_PACKET_MAX_LEN_BYTES - 1) { + return MqttPacket_HandleNetError(client, + MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA)); + } + /* Read next byte and try decode again */ len = 1; rc = MqttSocket_Read(client, &rx_buf[client->packet.header_len], @@ -3759,13 +3967,18 @@ int MqttPacket_Read(MqttClient *client, byte* rx_buf, int rx_buf_len, return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); } rc = MqttDecode_Vbi(header->len, - (word32*)&client->packet.remain_len, + &remain_len_u32, rx_buf_len - (client->packet.header_len - (i + 1))); if (rc < 0) { /* Indicates error */ return MqttPacket_HandleNetError(client, rc); } /* Indicates decode success and rc is len of header */ else { + if (remain_len_u32 > MQTT_PACKET_MAX_REMAIN_LEN) { + return MqttPacket_HandleNetError(client, + MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA)); + } + client->packet.remain_len = (int)remain_len_u32; /* Add size of type and flags */ rc += sizeof(header->type_flags); client->packet.header_len = rc; diff --git a/src/mqtt_sn_client.c b/src/mqtt_sn_client.c index 77b75443e..0c45b66b0 100644 --- a/src/mqtt_sn_client.c +++ b/src/mqtt_sn_client.c @@ -223,6 +223,11 @@ static int SN_Client_HandlePacket(MqttClient* client, SN_MsgType packet_type, type = (p_pub->qos == MQTT_QOS_1) ? SN_MSG_TYPE_PUBACK : SN_MSG_TYPE_PUBREC; + /* [MQTT-SN-1.2 5.4.4] The response MUST echo the received + * PUBLISH TopicId; topic_name holds the 2-byte TopicId. */ + (void)MqttDecode_Num((byte*)p_pub->topic_name, + &p_pub->resp.topicId, MQTT_DATA_LEN_SIZE); + p_pub->resp.return_code = SN_RC_ACCEPTED; p_pub->resp.packet_id = packet_id; #ifdef WOLFMQTT_MULTITHREAD @@ -672,7 +677,7 @@ static int SN_Client_WaitType(MqttClient *client, void* packet_obj, } /* Clear shared union for next call */ - MqttSNClient_PacketReset(packet_type, &client->msg); + MqttSNClient_PacketReset(packet_type, &client->msgSN); #ifdef WOLFMQTT_DEBUG_CLIENT PRINTF("Read Packet: Len %d, Type %d, ID %d", diff --git a/src/mqtt_sn_packet.c b/src/mqtt_sn_packet.c index 4236c3961..0e8202a86 100644 --- a/src/mqtt_sn_packet.c +++ b/src/mqtt_sn_packet.c @@ -1423,6 +1423,11 @@ int SN_Decode_PublishResp(byte* rx_buf, int rx_buf_len, byte type, return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); } + /* Fixed size: PUBACK is 7 bytes, PUBREC/PUBREL/PUBCOMP are 4 */ + if (total_len != ((type == SN_MSG_TYPE_PUBACK) ? 7 : 4)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + } + /* Validate packet type */ rec_type = *rx_payload++; if (rec_type != type) { @@ -1654,7 +1659,11 @@ int SN_Encode_Ping(byte *tx_buf, int tx_buf_len, SN_PingReq *ping, byte type) if ((type == SN_MSG_TYPE_PING_REQ) && (ping != NULL) && (ping->clientId != NULL)) { - total_len += clientId_len = (int)XSTRLEN(ping->clientId); + clientId_len = (int)XSTRLEN(ping->clientId); + if (clientId_len > SN_CLIENTID_MAX_LEN) { + clientId_len = SN_CLIENTID_MAX_LEN; + } + total_len += clientId_len; } if (total_len > tx_buf_len) { diff --git a/tests/test_mqtt_packet.c b/tests/test_mqtt_packet.c index 18245ccf7..ff63f828f 100644 --- a/tests/test_mqtt_packet.c +++ b/tests/test_mqtt_packet.c @@ -1053,15 +1053,37 @@ TEST(encode_publish_v5_empty_topic_accepted) { byte tx_buf[64]; MqttPublish pub; + MqttProp prop; int rc; + /* An empty v5 Topic Name is only valid when a Topic Alias is supplied; + * the encoder now mirrors the decoder and rejects it otherwise. */ XMEMSET(&pub, 0, sizeof(pub)); + XMEMSET(&prop, 0, sizeof(prop)); + prop.type = MQTT_PROP_TOPIC_ALIAS; + prop.data_short = 1; + prop.next = NULL; pub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; pub.topic_name = ""; pub.qos = MQTT_QOS_0; + pub.props = ∝ rc = MqttEncode_Publish(tx_buf, (int)sizeof(tx_buf), &pub, 0); ASSERT_TRUE(rc > 0); } + +TEST(encode_publish_v5_empty_topic_no_alias_rejected) +{ + byte tx_buf[64]; + MqttPublish pub; + int rc; + + XMEMSET(&pub, 0, sizeof(pub)); + pub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + pub.topic_name = ""; + pub.qos = MQTT_QOS_0; + rc = MqttEncode_Publish(tx_buf, (int)sizeof(tx_buf), &pub, 0); + ASSERT_TRUE(rc < 0); +} #endif /* WOLFMQTT_V5 */ /* ============================================================================ @@ -3678,6 +3700,7 @@ TEST(decode_suback_multiple_return_codes) ASSERT_EQ(MQTT_QOS_1, ack.return_codes[0]); ASSERT_EQ(MQTT_QOS_2, ack.return_codes[1]); ASSERT_EQ(MQTT_SUBSCRIBE_ACK_CODE_FAILURE, ack.return_codes[2]); + ASSERT_EQ(3, ack.return_code_count); } TEST(decode_suback_malformed_remain_len_zero) @@ -4891,6 +4914,7 @@ void run_mqtt_packet_tests(void) RUN_TEST(encode_publish_null_topic_returns_bad_arg); #ifdef WOLFMQTT_V5 RUN_TEST(encode_publish_v5_empty_topic_accepted); + RUN_TEST(encode_publish_v5_empty_topic_no_alias_rejected); #endif /* MqttDecode_Publish */ diff --git a/wolfmqtt/mqtt_packet.h b/wolfmqtt/mqtt_packet.h index fdda86772..04452d967 100644 --- a/wolfmqtt/mqtt_packet.h +++ b/wolfmqtt/mqtt_packet.h @@ -547,6 +547,7 @@ typedef struct _MqttSubscribeAck { word16 packet_id; byte return_codes[MAX_MQTT_TOPICS]; + word16 return_code_count; /* number of return codes the broker sent */ #ifdef WOLFMQTT_V5 MqttProp* props; byte protocol_level;