Skip to content
This repository has been archived by the owner on Jun 10, 2022. It is now read-only.

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

Closed
zztytu opened this issue May 14, 2019 · 6 comments
Closed
Assignees

Comments

@zztytu
Copy link

zztytu commented May 14, 2019

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
image
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
Copy link
Owner

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

@darconeous
Copy link
Owner

@darconeous
Copy link
Owner

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
Copy link
Owner

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

@darconeous
Copy link
Owner

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

@darconeous
Copy link
Owner

darconeous commented May 25, 2019

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

darconeous added a commit that referenced this issue May 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants