isisd: misusing strdup leads to stack overflow

[whichbug]

At Line 470 in the code below, we call yang_data_new, which will further call strdup(raw_pdu). However, raw_pdu is not guaranteed to be a zero-terminated string and, thus, will lead to a stack overflow in strdup. When I set raw_pdu[raw_pdu_len - 1] to \0, then the bug disappears. Note that strdup should be used with a C-string.

In the same file, isis_nb_notifications.c, there are 8 places where yang_data_new are used with raw_pdu and, thus, may have the overflow bug. Please check and suggest a fix. I can give a pull request then.

frr/isisd/isis_nb_notifications.c

Lines 454 to 471 in eef8006

	void isis_notif_id_len_mismatch(const struct isis_circuit *circuit,
	uint8_t rcv_id_len, const char *raw_pdu,
	size_t raw_pdu_len)
	{
	const char *xpath = "/frr-isisd:id-len-mismatch";
	struct list *arguments = yang_data_list_new();
	char xpath_arg[XPATH_MAXLEN];
	struct yang_data *data;
	struct isis_area *area = circuit->area;
	notif_prep_instance_hdr(xpath, area, "default", arguments);
	notif_prepr_iface_hdr(xpath, circuit, arguments);
	snprintf(xpath_arg, sizeof(xpath_arg), "%s/pdu-field-len", xpath);
	data = yang_data_new_uint8(xpath_arg, rcv_id_len);
	listnode_add(arguments, data);
	snprintf(xpath_arg, sizeof(xpath_arg), "%s/raw-pdu", xpath);
	data = yang_data_new(xpath_arg, raw_pdu);
	listnode_add(arguments, data);

What follows is the output of the address sanitizer:

==48351==ERROR: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7ffe596a3a76 at pc 0x000000543e97 bp 0x7ffe596a3020 sp 0x7ffe596a27e0
READ of size 23 at 0x7ffe596a3a76 thread T0
    #0 0x543e96 in strdup (/home/parallels/myfrr/isisd/isisd+0x543e96)
    #1 0x84f73d in yang_data_new /home/parallels/myfrr/lib/yang.c:608:17
    #2 0x6716eb in isis_notif_id_len_mismatch /home/parallels/myfrr/isisd/isis_nb_notifications.c:470:9
    #3 0x5cedcf in isis_handle_pdu /home/parallels/myfrr/isisd/isis_pdu.c:1706:3

[whichbug]

@idryzhov Could you also have a look at this issue?

[idryzhov]

Hi @whichbug, I checked the issue. The problem here is that we're trying to represent binary data as a simple string, which is wrong.

The only correct solution here would be to implement new function yang_data_new_binary(const char *xpath, const char *binary, size_t len) which will create a correct textual representation of YANG binary data, which is Base 64 Encoding. And then use this function instead of simple yang_data_new whenever we need to convert raw_pdu.
I probably won't find time to implement it in the next couple of days, so feel free to submit a PR.

[whichbug]

Hi @whichbug, I checked the issue. The problem here is that we're trying to represent binary data as a simple string, which is wrong.

The only correct solution here would be to implement new function yang_data_new_binary(const char *xpath, const char *binary, size_t len) which will create a correct textual representation of YANG binary data, which is Base 64 Encoding. And then use this function instead of simple yang_data_new whenever we need to convert raw_pdu. I probably won't find time to implement it in the next couple of days, so feel free to submit a PR.

Hi, @idryzhov thanks for your reply. I think your idea makes sense and I will try.

[qlyoung]

Nice find. By the way, when you find issues like this, if you want you can just submit a pull request without filing an issue. We patch these kinds of things regularly without making an issue, since the pull request itself is records of both the issue and the appropriate fix for it. It's a little easier for us since we can track it in one place, and it helps keep our issues a little cleaner. It's up to you though.

[whichbug]

Nice find. By the way, when you find issues like this, if you want you can just submit a pull request without filing an issue. We patch these kinds of things regularly without making an issue, since the pull request itself is records of both the issue and the appropriate fix for it. It's a little easier for us since we can track it in one place, and it helps keep our issues a little cleaner. It's up to you though.

Sure. Many thanks!