lose null pointer check in isTopicMatched() inMQTT

[TheSilentDawn]

Description of defect

URL: https://os.mbed.com/teams/mqtt/ Upstream URL: https://github.com/eclipse/paho.mqtt.embedded-c Type: DoS

The MQTT library is used to receive, parse and send mqtt packet between a broker and a client. The function readMQTTLenString() is called by the function MQTTDeserialize_publish() to get the length and content of the MQTT topic name. It parses the MQTT input linearly. Once a type-length-value tuple is parsed, the index is increased correspondingly. The maximum index is restricted by the length of the received packet size, as shown in line 4 of the code snippet below.

int readMQTTLenString(MQTTString* mqttstring, unsigned char** pptr, unsigned char* enddata)
{
    ...
    if (&(*pptr)[mqttstring->lenstring.len] <= enddata)
    {
        mqttstring->lenstring.data = (char*)*pptr;
        *pptr += mqttstring->lenstring.len;
        rc = 1;
    }
    ...
}

Note that mqttstring->lenstring.len is a part of user input, which can be manipulated. An attacker can simply change it to a larger value to invalidate the if statement so that the statements from line 6 to 8 are skipped, leaving the value of mqttstring->lenstring.data default to zero. Later, the value of mqttstring->lenstring.data is assigned to curn (line 4 of the code snippet below), which is zero under the attack. In line 9, *curn is accessed. In an ARM cortex-M chip, the value at address 0x0 is actually the initialization value for the MSP register. It is highly dependent on the actual firmware. Therefore, the behavior of the program is unpredictable from this time on.

bool MQTT::Client<Network, Timer, a, b>::isTopicMatched(char* topicFilter, MQTTString& topicName)
{
    char* curf = topicFilter;
    char* curn = topicName.lenstring.data;
    char* curn_end = curn + topicName.lenstring.len;

    while (*curf && curn < curn_end)
    {
        if (*curn == '/' && *curf != '/')
            break;
        if (*curf != '+' && *curf != '#' && *curf != *curn)
            break;
        if (*curf == '+')
        {   // skip until we meet the next separator, or end of string
            char* nextpos = curn + 1;
            while (nextpos < curn_end && *nextpos != '/')
                nextpos = ++curn + 1;
        }
        else if (*curf == '#')
            curn = curn_end - 1;    // skip until end of string
        curf++;
        curn++;
    };

    return (curn == curn_end) && (*curf == '\0');
}

Result: A malformed MQTT packet may cause unexpected behaviors depending on the value stored at the address zero on the board.

Target(s) affected by this defect ?

MQTT library of MbedOS

Toolchain(s) (name and version) displaying this defect ?

N/A

What version of Mbed-os are you using (tag or sha) ?

Mbed OS MQTT library 02 Nov 2017 Mbed OS latest release

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

mbed-cli latest version

How is this defect reproduced ?

Use bug_mqtt_1 and bug_mqtt_2 as received mqtt packet after mqtt connect and subscribe bug_mqtt_1.log bug_mqtt_2.log

[0xc0170]

Hi @TheSilentDawn . thanks for the reports today. We will triage them.

As you reported couple of them with nice detailed reports, I am curious if you run some analysis or found them during using for instance in this case MQTT.

[0xc0170]

If this one has the same issue https://github.com/ARMmbed/mbed-mqtt ?

cc @michalpasztamobica

[michalpasztamobica]

@0xc0170 , yes, the bug is in Paho library code which both the old and the new MQTT libs are using under the hood. We have submitted a patch for this last week in https://github.com/ARMmbed/mbed-mqtt/pull/7. I believe the old lib is not maintained any more. @TheSilentDawn , please take a look at the patch and let us know if it is sufficient to close this issue? @AnttiKauppila , FYI.

[0xc0170]

I believe the old lib is not maintained any more.

Shouldn't we add a note to that team page ("Deprecated, use the updated MQTT library for Mbed OS url here" for instance) ? I expected before opening that it would provide the ARMmbed/mbed-mqtt library.

Not related to the bug report, but once we are on this topic. We might create a separate issue about this. @michalpasztamobica please create an internal ticket for this if you consider this page MQTT team incorrect.

[TheSilentDawn]

Hi @michalpasztamobica . Our team reported this bug to Arm MbedOS team through support@mbed.com at 10/2/2019 but get no reley about it's patched. Do you fix the bug based on the previous report or find it by yourself?

[michalpasztamobica]

@TheSilentDawn , yes, we fixed the issue based on the report which came to support@mbed.com and looks identical to this issue's description. I am sorry to hear that you got no response from the support team, perhaps they waited for the issue to be addressed before replying? Many thanks for the detailed and helpful report.

Are you also planning to report it to https://github.com/eclipse/paho.mqtt.embedded-c? (That's the source of the core logic, for which mbed-mqtt is more of a wrapper)

[TheSilentDawn]

@michalpasztamobica Thanks. It's ok. We just want to make sure the bug is fixed ASAP. No, we haven't reported it to https://github.com/eclipse/paho.mqtt.embedded-c. I'm not sure have your team reported it to the eclipse team? If not, we're pleasure to report it the eclipse team to help them fix the same bug.

[michalpasztamobica]

We haven't reported it and I think it's best if you do :)

[TheSilentDawn]

@michalpasztamobica Ok, we will report it. 👍

[TheSilentDawn]

@0xc0170 @michalpasztamobica Yes, we run some analysis. And we could email you a copy of our work when we submit it to the academic conference recently. Could you please give me the email of your team?

[0xc0170]

@0xc0170 @michalpasztamobica Yes, we run some analysis. And we could email you a copy of our work when we submit it to the academic conference recently. Could you please give me the email of your team?

You can find ours in the git log or check our profiles, mine is there for instance. Happy to read it once it is out.

[TheSilentDawn]

@0xc0170 @michalpasztamobica Yes, we run some analysis. And we could email you a copy of our work when we submit it to the academic conference recently. Could you please give me the email of your team?

You can find ours in the git log or check our profiles, mine is there for instance. Happy to read it once it is out.

[TheSilentDawn]

Thanks.

[0xc0170]

this can be closed as it's fixed in MQTT library, or what else we are missing ?

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOTRIAGE-2330

[TheSilentDawn]

Internal Jira reference: https://jira.arm.com/browse/MBOTRIAGE-2330

This link is unavailable.

[AnttiKauppila]

The link is for our internal JIRA for tracking. Therefore you don't have access to it.