Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integer overflow when decoding mqtt variable length #1055

Closed
HarDToBelieve opened this issue Oct 2, 2019 · 4 comments
Closed

Integer overflow when decoding mqtt variable length #1055

HarDToBelieve opened this issue Oct 2, 2019 · 4 comments

Comments

@HarDToBelieve
Copy link

The problem occurs in function parse_mqtt, when the broker proceeds a message , first it decodes a sequence bytes from the 2nd to get the length of data and then determine the end of data by sum up p and len:

len = len_len = 0;
p = io->buf + 1;
while (p < eop) {
    lc = *((const unsigned char *) p++);
    len += (lc & 0x7f) << 7 * len_len;
    len_len++;
    if (!(lc & 0x80)) break;
    if (len_len > 4) return MG_MQTT_ERROR_MALFORMED_MSG;
}
end = p + len;

However, if len is too large, the value of end become less than value of p. And the root cause here is this line:

len += (lc & 0x7f) << 7 * len_len;

Using modern gcc compiler on linux, when the broker casts down to data type of len ( which is size_t ), the value will be auto cast to 64 bit.

#include <stdio.h>

int main(void) {
	unsigned char lc = '\x7f';
	size_t len = 4;
	
	printf("%lu\n", (lc & 0x7f) << 7 * len);
	printf("%lu\n", (size_t)((lc & 0x7f) << 7 * len));
	
	return 0;
}

and the result is:

4026531840
18446744073441116160

Exploit concept of 2's complement, we can control the value of end from p-122 to p-1.

Impact:

  • Remote DoS: if value of end equals to io->buf, then the current message won't be removed from IO buffer, which leads to an infinite loop ( for example: the broker is always busy if we send this payload \x00\xfa\xff\xff\xff\x7f , tested on the latest version of Mongoose on Ubuntu )
  • Potential out-of-bound access: because the value of end may be less than io->buf, it can be exploited as out-of-bound read/write in further developments
@HarDToBelieve HarDToBelieve changed the title Integer overflow when decode mqtt variable length Integer overflow when decoding mqtt variable length Oct 2, 2019
@HarDToBelieve
Copy link
Author

HarDToBelieve commented Oct 9, 2019

@cpq, sorry for tagging you but I dont know who is the right person to review and I see you merge PR.

Anyway, I think we can easily fix the bug by not using data type size_t for len , use unsigned int instead. I have also tested this change , and if you want I can pull a request

@cpq
Copy link
Member

cpq commented Dec 23, 2019

Thank you! I would like to integrate this change. Could you sign the CLA, please? The link is at the bottom of the project's README.md page.

@HarDToBelieve
Copy link
Author

HarDToBelieve commented Dec 23, 2019

Sure, I have already signed and made a PR ( #1089 )

@cpq
Copy link
Member

cpq commented Feb 11, 2020

released https://github.com/cesanta/mongoose/releases/tag/6.17
Thank you so much.

@cpq cpq closed this as completed Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants