MqttNet context overwritten in MqttConnect corner case circumstance

[wolfssl]

MqttNet context overwritten in MqttConnect corner case circumstance

[dgarske]

Hi @matharman : Thank you for this detailed report. I will take a look at this use case shortly.

As a general note... make sure any Mqtt structures you use are memset to 0. We have some "state" structure members that expect zero initialization.

Also can you tell us a bit more about your project and use-case? Feel free to email us directly at [email protected].

Thanks,
David Garske, wolfSSL

[matharman]

@dgarske Thanks for your quick response. The client application is used to receive simple commands via MQTT on an nrf9160 like flash an LED or talk to an I2C peripheral and return the data read. It's a proof of concept project atm.

I dug a little deeper into the issue, and the overwriting of the MqttNet context pointer is incidental due to my implementation and platform. I have a struct used as a concept of mqtt context:

struct mqtt_ctx {
    struct tls_ctx tls; // Has the definition in my original issue comment
    MqttClient client;
    MqttConnect conn;
    MqttNet net;
    MqttMsgCb cb;
};

See the bottom of this comment for a snippet of the initialization code.

If I rearrange this struct with MqttConnect as the last member of the struct, nothing fatal is overwritten. In the scenario I outlined originally, with the above orientation of WolfMQTT data structures, it so happens that the next address in memory after the end of the conn->ack member is the MqttNet context pointer.

The relevance of conn->ack to this problem is that it is being passed as the packet_obj in MqttClient_HandlePacket, and the packet type that's received is a publish. I think the code flow after that results in conn->ack being aliased as the starting address of an MqttPublish structure.

Other struct orientations and relocating the MqttConnect either coincidentally function as expected (I suspect this is when userspace memory that is not used later is overwritten) or cause a segfault.

Let me know if I can provide any further useful information.

MQTT Context initialization
The tx and rx buffers are static in the caller and 128 bytes each. The longest published message length in use is on the order of 10-15 characters.

int demo_mqtt_init(struct mqtt_ctx *ctx,
        uint8_t *tx_buf, int tx_buf_len,
        uint8_t *rx_buf, int rx_buf_len)
{
    if(!ctx || !tx_buf || !rx_buf) {
        LOG_ERR("Invalid MQTT Init parameters!");
        return MQTT_CODE_ERROR_BAD_ARG;
    }

    ctx->mqtt_msg_cb = wolfmqtt_msg_callback;
    memset(&ctx->client, 0, sizeof(ctx->client));
    memset(&ctx->net, 0, sizeof(ctx->net));

    ctx->net.context = &ctx->tls;
    ctx->net.connect = wolfmqtt_connect;
    ctx->net.read = wolfmqtt_read;
    ctx->net.write = wolfmqtt_write;
    ctx->net.disconnect = wolfmqtt_disconnect;

    int rc = MqttClient_Init(&ctx->client, &ctx->net, ctx->mqtt_msg_cb,
                tx_buf, tx_buf_len,
                rx_buf, rx_buf_len,
                1000);

    if(rc != MQTT_CODE_SUCCESS) {
        LOG_ERR("MQTT INIT failed! WolfMQTTErr: %d", rc);
        return rc;
    }

    rc = MqttClient_SetDisconnectCallback(&ctx->client, wolfmqtt_disconnect_cb, NULL);
    if(rc != MQTT_CODE_SUCCESS) {
        LOG_ERR("MQTT SETDISCONNECTCALLBACK failed! WolfMQTTErr: %d", rc);
    }

    return rc;
}

int demo_mqtt_connect(struct mqtt_ctx *ctx)
{
    if(!ctx) {
        LOG_ERR("MQTT Connect bad parameters!");
        return MQTT_CODE_ERROR_BAD_ARG;
    }

    int rc = MqttClient_NetConnect(&ctx->client,
                CONFIG_DEMO_MQTT_BROKER, 8883, 0, false, NULL);
    if(rc != MQTT_CODE_SUCCESS) {
        LOG_ERR("TLS-Layer CONNECT failed! WolfMQTTErr: %d", rc);
        return rc;
    }

    memset(&ctx->conn, 0, sizeof(ctx->conn));
    ctx->conn.keep_alive_sec = CONFIG_DEMO_MQTT_KEEP_ALIVE;
    ctx->conn.clean_session = true;
    ctx->conn.client_id = CONFIG_DEMO_SE_ID;
    ctx->conn.enable_lwt = false;

    rc = MqttClient_Connect(&ctx->client, &ctx->conn);
    if(rc != MQTT_CODE_SUCCESS) {
        LOG_ERR("MQTT-Layer CONNECT failed! WolfMQTTErr: %d", rc);
    }

    return rc;
}

[embhorn]

Hello @matharman

One thing I see right away is

    ctx->net.context = &ctx->tls;

The MqttNet structure in the examples uses the context pointer to store the SocketContext. The TLS context pointer is stored in the tls element of the MqttClient structure.

I do not know how the application's mqttnet.c functions are implemented without seeing all the application specific files.

I saw you mention that ENABLE_MQTT_TLS is disabled. I would suggest adding a conditional in the application context structure:

struct mqtt_ctx {
#ifdef ENABLE_MQTT_TLS
    struct tls_ctx tls; // Has the definition in my original issue comment
#endif
    MqttClient client;
    MqttConnect conn;
    MqttNet net;
    MqttMsgCb cb;
};

I suspect the corruption issue is related to either a bad structure dereference, or perhaps an include using different macro settings between the library and the application.

Thanks,
Eric
wolfSSL Support

[matharman]

Sorry for the delayed response, I've had other projects that I had to prioritize but I've found time to get back to this one. Thank you for your help.

TL;DR Revisiting the macro visibility before defining ENABLE_MQTT_TLS seems to have resolved the issue. Sorry to bug the issue tracker with something so simple :)

---

Longer version for future readers:
Originally we opted to treat the WolfSSL data structures as part of the socket context (not library authors' intended use at all) as ENABLE_MQTT_TLS seemed to invoke pthreads in the WolfSSL library (incompatible with the RTOS we're developing for).

I revisited my macro settings for both libraries and realized that while WolfSSL was properly configured, its macro settings were not properly defined to the WolfMQTT library in our build order. I resolved that problem and was able to use the MqttTlsCb no problem. That in turn stopped the mangling of our struct.