Change Details

A brief overview of routing sockets: 1. All route messages that are generated in the kernel, whether a response to a route message written to a routing socket from userland or an asynchronous route event from within the kernel, are queued to all open routing sockets (this is a slight oversimplification that ignores request-loopback and fib-based filtering, but those complications don't affect what follows). 2. There are multiple route message formats and only the first three members of the message (length, version, and type) are common across all formats. (1) means that when reading a routing socket looking for a response to a request that was just written, there may be an arbitrary number of route messages waiting in the route socket ahead of the response of interest. (2) means that when processing messages read from a routing socket, the type of the received message must first be tested before evaulating format-specific fields in the message. The following userland tools make invalid assumptions about the format of messages read from a routing socket - that is, they don't ensure the message is of a given format before interpreting message contents according to that format: sbin/route sbin/routed usr/bin/netstat usr/sbin/arp usr/sbin/ndp usr/sbin/rarpd usr/sbin/route6d I examined all users of struct rt_msghdr and PF_ROUTE in the userland sources and have only identified issues in the above. In addition to fixing the above programs, this patch includes the following additional cleanup and doc: - In route.h, the route message types are annotated with the message format used - In route.h, the NULL pointer check has been removed from SA_SIZE() along with the comment casting doubt on its usefulness. There is no uncertainty - no consumer of SA_SIZE() expects it to work with a NULL pointer - and the presence of the check obscures the fact that the route message format does not encode non-present sockaddrs as sizeof(long) zeroes - it omits them entirely from the message contents. - In usr.sbin/arp/arp.c, in set(), there is no point to passing &sdl_m to rtmsg() for the initial RTM_GET as the gateway address is not used nor modified in any way for an RTM_GET. Pasing &sdl_m to RTM_GET requests only impedes understanding of the code. The same goes for the RTM_GET in delete(). - In sbin/route/route.c, in rtmsg(), replaced the size computation in NEXTADDR() with the identical SA_SIZE(). - In usr.bin/netsat/route.c, in p_rtentry_sysctl(), the address list processing logic introduced in r287351 winds up adding arbitrary offsets to the sa pointer, then dereferences the result to compute the size, following any gap in the list of possible addresses in the message, and there is always a gap. This hasn't corrupted output because coincidentally the only address fields of interest after this loop are the first three in the message, which are always present. The lack of coredumps is probably due to the random offsets being added to the sa pointer being 16 bits. I searched for all other uses of RTAX_MAX in the userland code to find other such address list processing loops, and found none of the other to be defective. - There is a fix included for contrib/traceroute/findsaddr-socket.c, although it appears to me that we don't include that file as part of our traceroute build. I'm also not sure of the upstream situation for traceroute.

A brief overview of routing sockets: 1. All route messages that are generated in the kernel, whether a response to a route message written to a routing socket from userland or an asynchronous route event from within the kernel, are queued to all open routing sockets (this is a slight oversimplification that ignores request-loopback and fib-based filtering, but those complications don't affect what follows). 2. There are multiple route message formats and only the first three members of the message (length, version, and type) are common across all formats. (1) means that when reading a routing socket looking for a response to a request that was just written, there may be an arbitrary number of route messages waiting in the route socket ahead of the response of interest. (2) means that when processing messages read from a routing socket, the type of the received message must first be tested before evaulating format-specific fields in the message. The following userland tools make invalid assumptions about the format of messages read from a routing socket - that is, they don't ensure the message is of a given format before interpreting message contents according to that format: sbin/route sbin/routed usr/bin/netstat usr/sbin/arp usr/sbin/ndp usr/sbin/rarpd usr/sbin/route6d I examined all users of struct rt_msghdr and PF_ROUTE in the userland sources and have only identified issues in the above. In addition to fixing the above programs, this patch includes the following additional cleanup and doc: - In route.h, the route message types are annotated with the message format used - In route.h, the NULL pointer check has been removed from SA_SIZE() along with the comment casting doubt on its usefulness. There is no uncertainty - no consumer of SA_SIZE() expects it to work with a NULL pointer - and the presence of the check obscures the fact that the route message format does not encode non-present sockaddrs as sizeof(long) zeroes - it omits them entirely from the message contents. - In usr.sbin/arp/arp.c, in set(), there is no point to passing &sdl_m to rtmsg() for the initial RTM_GET as the gateway address is not used nor modified in any way for an RTM_GET. Pasing &sdl_m to RTM_GET requests only impedes understanding of the code. The same goes for the RTM_GET in delete(). - In sbin/route/route.c, in rtmsg(), replaced the size computation in NEXTADDR() with the identical SA_SIZE(). - In usr.bin/netsat/route.c, in p_rtentry_sysctl(), the address list processing logic introduced in r287351 winds up adding arbitrary offsets to the sa pointer, then dereferences the result to compute the size, following any gap in the list of possible addresses in the message, and there is always a gap. This hasn't corrupted output because coincidentally the only address fields of interest after this loop are the first three in the message, which are always present. The lack of coredumps is probably due to the random offsets being added to the sa pointer being 16 bits. I searched for all other uses of RTAX_MAX in the userland code to find other such address list processing loops, and found none of the other to be defective. - There is a fix included for contrib/traceroute/findsaddr-socket.c, although it appears to me that we don't include that file as part of our traceroute build. I'm also not sure of the upstream situation for traceroute.

A brief overview of routing sockets: 1. All route messages that are generated in the kernel, whether a response to a route message written to a routing socket from userland or an asynchronous route event from within the kernel, are queued to all open routing sockets (this is a slight oversimplification that ignores request-loopback and fib-based filtering, but those complications don't affect what follows). 2. There are multiple route message formats and only the first three members of the message (length, version, and type) are common across all formats. (1) means that when reading a routing socket looking for a response to a request that was just written, there may be an arbitrary number of route messages waiting in the route socket ahead of the response of interest. (2) means that when processing messages read from a routing socket, the type of the received message must first be tested before evaulating format-specific fields in the message. The following userland tools make invalid assumptions about the format of messages read from a routing socket - that is, they don't ensure the message is of a given format before interpreting message contents according to that format: sbin/route sbin/routed usr/bin/netstat usr/sbin/arp usr/sbin/ndp usr/sbin/rarpd usr/sbin/route6d I examined all users of struct rt_msghdr and PF_ROUTE in the userland sources and have only identified issues in the above. In addition to fixing the above programs, this patch includes the following additional cleanup and doc: - In route.h, the route message types are annotated with the message format used - In route.h, the NULL pointer check has been removed from SA_SIZE() along with the comment casting doubt on its usefulness. There is no uncertainty - no consumer of SA_SIZE() expects it to work with a NULL pointer - and the presence of the check obscures the fact that the route message format does not encode non-present sockaddrs as sizeof(long) zeroes - it omits them entirely from the message contents. - In usr.sbin/arp/arp.c, in set(), there is no point to passing &sdl_m to rtmsg() for the initial RTM_GET as the gateway address is not used nor modified in any way for an RTM_GET. Pasing &sdl_m to RTM_GET requests only impedes understanding of the code. The same goes for the RTM_GET in delete(). - In sbin/route/route.c, in rtmsg(), replaced the size computation in NEXTADDR() with the identical SA_SIZE(). - In usr.bin/netsat/route.c, in p_rtentry_sysctl(), the address list processing logic introduced in r287351 winds up adding arbitrary offsets to the sa pointer, then dereferences the result to compute the size, following any gap in the list of possible addresses in the message, and there is always a gap. This hasn't corrupted output because coincidentally the only address fields of interest after this loop are the first three in the message, which are always present. The lack of coredumps is probably due to the random offsets being added to the sa pointer being 16 bits. I searched for all other uses of RTAX_MAX in the userland code to find other such address list processing loops, and found none of the other to be defective. - There is a fix included for contrib/traceroute/findsaddr-socket.c, although it appears to me that we don't include that file as part of our traceroute build. I'm also not sure of the upstream situation for traceroute.