urlencode icon indicating copy to clipboard operation
urlencode copied to clipboard

Lack of checking for NULL-pointer, string termination and hexa-convertibility

Open TCH68k opened this issue 5 years ago • 0 comments

Hello.

Your url_decode() function does not check for string termination when it finds a percentage sign, which may result in a SIGSEGV if it tries to read beyond the terminator. (Example: 'abcd%') I suggest inserting a pre-check before the read into buffer.

Also, it does not check if it can convert the next two bytes and they may not be hexadecimal numbers at all (examples: '%qr' or '%?!'), which will cause strtol to give back a zero which will be written back into working and thus terminating it in the middle of the string. Since strtol does not set errno on invalid values but range errors, i suggest checking their hexa-convertibility manually.

In code, the row if(*input == '%') should look like this:

if
(
	(*input == '%') &&
	(input[1] != 0) &&
	(input[2] != 0) &&
	(
		((input[1] >= '0') && (input[1] <= '9')) ||
		((input[1] >= 'A') && (input[1] <= 'F')) ||
		((input[1] >= 'a') && (input[1] <= 'f'))
	) &&
	(
		((input[2] >= '0') && (input[2] <= '9')) ||
		((input[2] >= 'A') && (input[2] <= 'F')) ||
		((input[2] >= 'a') && (input[2] <= 'f'))
	)
)

Furthermore, it does not sanitize the input variable: if input is NULL it should abort the decoding immediately.

And while it most probably will never happen with small buffers of text, you should also check for NULL, when you are allocating memory for working in case someone tries to URL-decode a text which is too large to allocate a decoding buffer for it.

TCH68k avatar Feb 18 '20 19:02 TCH68k