Skip to content

Fix four memory-safety defects: sline timeout, message-tag lifetime, process timer, IPv4 numnick encoding - #125

Open
MrIron-no wants to merge 4 commits into
UndernetIRC:mainfrom
MrIron-no:fix/sline-timeout-uaf
Open

MrIron-no wants to merge 4 commits into
UndernetIRC:mainfrom
MrIron-no:fix/sline-timeout-uaf

Conversation

@MrIron-no

Copy link
Copy Markdown
Contributor

Summary

Four small memory-safety fixes, one per commit. Two are stale reads of a
freed structure, one is a use-after-free of a connection buffer that is
also a logic bug, and one is undefined behaviour in address encoding.

sline.c — stale read after the timeout callback releases an entry

sline_hold_timeout_callback() calls sline_release_hold(entry->token),
which unlinks and frees the entry, then reads entry->token again for
the two Debug() lines that follow. Copy the token before the call.
Debug builds only; reads only.

numnicks.c — signed shift overflow in iptobase64()

ntohs() yields a 16-bit value that is promoted to int, so
(ntohs(x) << 16) overflows for any IPv4 address whose first two octets
are 192.0 or higher. Every compiler in use produces the intended bit
pattern, but it is undefined behaviour. Shift as unsigned. Present since
the 2004 IPv6 work.

parse.c / packet.c — message tags outlive the line they came from

msg_tag_parse() does not copy: each tag's key and value point into the
sending client's con_buffer. parse.c keeps the list in a static that
is only reset when the next line is parsed, and send_buffer() falls
back to that list whenever it is called without an explicit tag context.

So any send made outside a command handler picked up the previous line's
tags: exit_client() from a socket EOF/error callback, anything driven
by a timer. That is wrong even when nothing has been freed (an unrelated
client's ERROR :Closing Link carries someone else's label/msgid),
and once the client that sent that line is gone it is a read from a
freed connection.

Clear the list in packet.c right after each dispatch, so the fallback
is only live while the handler that owns the line runs. Every send made
from a handler is unaffected.

s_bsd.c — process timer touches a client read_packet() freed

client_timer_callback() drains pending input with read_packet(),
which can exit the client (a QUIT in the buffer, a registration failure)
and free its struct Client. The trailing consistency assert then read
cli_connect() from the freed structure. Return when read_packet()
reports CPTR_KILLED.

Testing

Builds warning-free (--enable-warnings); ircd/test unit suite passes;
the docker integration suite passes with the ircds built with
-fsanitize=address,undefined.

sline_release_hold() unlinks and frees the entry it is given.  The
hold-queue timeout callback then read entry->token from the freed
memory to build its debug messages.  Copy the token before releasing.

Only debug builds compile the offending reads, and they are reads, so
the visible effect was garbage in a Debug() line rather than a crash.
ntohs() yields a 16-bit value that is promoted to int before the shift,
so (ntohs(x) << 16) overflows for any IPv4 address whose first two
octets are 192.0 or higher (0xC000 << 16 does not fit in a signed int).
That is undefined behaviour, even though every compiler in use happens
to produce the intended bit pattern.  Shift as unsigned.
msg_tag_parse() does not copy: each tag's key and value point into the
sending client's connection buffer.  parse.c kept the list in a static
that was only reset when the next line was parsed, and send_buffer()
falls back to that list whenever it is called without an explicit tag
context.

So any send made outside a command handler -- exit_client() from a socket
EOF/error callback, a timer -- was tagged with whatever the last parsed
line carried.  That is wrong on its own (an unrelated client's ERROR
gets someone else's label/msgid), and once the client that sent that
line has been freed it is a use-after-free of its connection.

Clear the list in packet.c right after each dispatch, so the fallback is
only live while the handler that owns the line is running.
client_timer_callback() drains pending input with read_packet(), which
can exit the client (a QUIT in the buffer, a registration failure) and
free its struct Client.  The trailing consistency assert then read
cli_connect() from the freed structure.  Return early when read_packet()
reports CPTR_KILLED.
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

Successfully merging this pull request may close these issues.

1 participant