There is a logical defect that causes a denial of service vulnerability

[zztytu]

src/libnyoci/coap.c lines 58-116:

len = (*buffer & 0x0F);

switch((*buffer >> 4)) {
    default:
        if(key) *key += (*buffer >> 4);
        buffer += 1;
        break;

    case 13:
        buffer += 1;
        if(key)*key += 13+*buffer;
        buffer += 1;
        break;

    case 14:
        buffer += 1;
        if(key)*key += 269+buffer[1]+(buffer[0]<<8);

        buffer += 2;
        break;

    case 15:
        // End of option marker...?
        // TODO: Fail harder if len doesn't equal 15 as well!
        if (key) *key = COAP_OPTION_INVALID;
        if (value) *value = NULL;
        if (lenP) *lenP = 0;
        return NULL;
        break;
}

switch(len) {
    default:
        break;

    case 13:
        len = 13 + *buffer;
        buffer += 1;
        break;

    case 14:
        len = 269+buffer[1]+(coap_size_t)(buffer[0]<<8);
        buffer += 2;
        break;

    case 15:
        // End of option marker...?
        // TODO: Fail harder if len doesn't equal 15 as well!
        if(key)*key = COAP_OPTION_INVALID;
        if(value)*value = NULL;
        if(lenP)*lenP = 0;
        return NULL;
        break;
}

if(lenP) *lenP = len;
if(value) *value = buffer;

return (uint8_t*)buffer + len;

If the data packet is processed as shown below Then the function coap_decode_option will set the length parameter to 0,and the value_len in the function nyoci_inbound_option_strequal in src/libnyoci/nyoci-inbound.c is 0(lines 157-168)

    coap_decode_option(self->inbound.this_option, &curr_key, (const uint8_t**)&value, &value_len);

    if (curr_key != key) {
        return false;
    }

    for (i = 0; i < value_len; i++) {
        if(!cstr[i] || (value[i] != cstr[i])) {
            return false;
        }
    }
    return cstr[i]==0;
}

If value_len is 0, the subsequent loop is not executed (line 163), and if the second argument cstr is an empty string, the return value is true (normal logic will return false in the loop) The function nyoci_node_list_request_handler in src/libnyociextra/nyoci-list.c for handling requests calls nyoci_inbound_option_strequal_const, and the second argument passed in is an empty string(lines 85-89).

    if (nyoci_inbound_option_strequal_const(COAP_OPTION_URI_PATH,"")) {
        // Eat the trailing '/'.
        nyoci_inbound_next_option(NULL, NULL);
        if(prefix[0]) prefix = NULL;
    }

Therefore, the special data packet will pass the judgment, the program will enter the assignment to the variable prefix (the variable prefix is ​​empty at this time), and the program eventually crashes.

Listening on port 5683
Segmentation fault

[darconeous]

Yep, that's a pretty clear logic error. Will fix.

[darconeous]

https://www.cvedetails.com/cve/CVE-2019-12101/

[darconeous]

Ok, I think I see where things might be going wrong. It seems like this problem is specific to libnyociextra's nyoci_node_list_request_handler(). I don't see any logic errors in the code you pasted until you get to nyoci_node_list_request_handler().

If a Uri-path has a length of zero (totally valid), then I would expect nyoci_inbound_option_strequal_const(COAP_OPTION_URI_PATH,"") to return true when it reaches that option. Empty Uri-path options are totally normal.

If value_len is 0, the subsequent loop is not executed (line 163), and if the second argument cstr is an empty string, the return value is true (normal logic will return false in the loop)

I'm not sure what you mean. Why would "normal logic" return false in the loop? If our Uri-path option is empty, what else would value_len be?

In any case, fixing the handling of the prefix variable in nyoci_node_list_request_handler seems like what is needed.

[darconeous]

I'm not immediately able to reproduce the issue. Could you send me a dump of a packet that reproduces the issue?

[darconeous]

Or at least show me a larger amount of information about the CoAP packet from wireshark.

[darconeous]

Ah, I figured out why I can't reproduce it. This issue was fixed by #12 (specifically d96723feaf24c260832ad44bb82d9947a9797424) last year. It was discovered by Bruno Menlo. The changes haven't yet made it into an official release.