ERROR: heap-buffer-overflow in group.c found with AddressSanitizer

[peirick]

In group.c in method handle_message_packet_group the variable index can change if message is GROUP_MESSAGE_KILL_PEER_ID see:

c-toxcore/toxcore/group.c

Line 2695 in 03a5114

delpeer(g_c, groupnumber, index, userdata);

At the end of the method index is pointing to an eventually deleted entry:

c-toxcore/toxcore/group.c

Line 2764 in 03a5114

bool relay_back = id_equal(g->group[index].real_pk, real_pk);

[iphydf]

peirick: can you reproduce this in a test case? See https://github.com/TokTok/c-toxcore/tree/master/auto_tests (conference_*test.c) for examples.

[peirick]

Sorry no. Tests are expensive to write. A simple fix would be to move the lines

c-toxcore/toxcore/group.c

Lines 2758 to 2764 in 03a5114

	/* If the packet was received from the peer who sent the message, relay it
	* back. When the sender only has one group connection (e.g. because there
	* are only two peers in the group), this is the only way for them to
	* receive their own message. */
	uint8_t real_pk[CRYPTO_PUBLIC_KEY_SIZE];
	get_friendcon_public_keys(real_pk, nullptr, g_c->fr_c, g->connections[connection_index].number);
	bool relay_back = id_equal(g->group[index].real_pk, real_pk);

above

c-toxcore/toxcore/group.c

Line 2666 in 03a5114

switch (message_id) {

Kind regards

[iphydf]

Thanks, what situation would this occur in, so I can write the test?

[peirick]

Given two peers in a group. A and B
A is first online, B comes later online.
B goes offline first.

flow:

GROUP_MESSAGE_KILL_PEER_ID or GROUP_MESSAGE_FREEZE_PEER_ID is send.
peer_number resolves to index of last added peer in group.

delpeer / freeze_peer is called. Because index is the last peer realloc on g->group is called. Thus Memory to that group is invalid.

After call to delpeer invalid memory on g->group[index] is accessed to set relay_back .