Page MenuHomeFreeBSD

Fix dst/netmask handling in routing socket code.
ClosedPublic

Authored by melifaro on Feb 14 2021, 8:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 12, 1:48 AM
Unknown Object (File)
Jan 13 2024, 8:34 PM
Unknown Object (File)
Dec 20 2023, 8:21 AM
Unknown Object (File)
Dec 12 2023, 12:57 AM
Unknown Object (File)
Nov 21 2023, 9:15 AM
Unknown Object (File)
Nov 21 2023, 9:14 AM
Unknown Object (File)
Nov 21 2023, 7:25 AM
Unknown Object (File)
Nov 21 2023, 5:50 AM
Subscribers

Details

Summary

Traditionally routing socket code did almost zero checks on the input message except for the most basic size checks.

This resulted in the unclear KPI boundary for the routing system code (rtrequest* and now rib_action()) w.r.t message validness.

Multiple potential problems and nuances exists:

  • Host bits in RTAX_DST sockaddr. Existing applications do send prefixes with hostbits uncleared. Even route(8) does this, as they hope the kernel would do the job. Code inside rib_action() needs to handle it on its own (see rt_maskedcopy() ugly hack).
  • There are multiple way of adding the host route: it can be DST without netmask or DST with /32(/128) netmask. Also, RTF_HOST has to be set correspondingly. Currently, these 2 options create 2 DIFFERENT routes in the kernel.
  • no sockaddr length/content checking for the "secondary" fields exists: nothing stops rtsock application to send sockaddr_in with length of 25 (instead of 16). Kernel will accept it, install to RIB as is and propagate to all rtsock consumers, potentially triggering bugs in their code. Same goes for sin_port, sin_zero, etc.

The goal of this change is to make rtsock verify all sockaddr consistency and prefix consistency. Said differently, rib_action() or internals should NOT require to change any of the sockaddrs supplied by rt_addrinfo structure due to incorrectness.

To be more specific, this change implements the following:

  • sockaddr cleanup/validation check is added immediately after getting sockaddrs from rtm.
  • Per-family dst/netmask checks clears host bits in dst and zeros all dst/netmask "secondary" fields.
  • The same netmask checking code converts /32(/128) netmasks to "host" route case (NULL netmask, RTF_HOST), removing the dualism.
  • Instead of allowing ANY "known" sockaddr families (<AF_MAX), allow only actually supported ones (inet, inet6, link).
  • Automatically convert sockaddr_sdl (AF_LINK) to sockaddr_sdl_short

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 37039
Build 33928: arc lint + arc unit

Event Timeline

melifaro retitled this revision from Fix dst/netmask handling in routing socket code. Improve overall message validation. to Fix dst/netmask handling in routing socket code..Feb 14 2021, 9:24 PM
melifaro edited the summary of this revision. (Show Details)
melifaro added a reviewer: network.
donner added inline comments.
sys/net/rtsock.c
73–74

Duplicate

742–749

Why rtm? Below in the new functions, the rti_addrs flags are modified with &= ~RTA_NETMASK

1296

inline?

1326

#ifdef INET

1336

#ifdef INET6

tests/sys/net/routing/rtsock_common.h
829

#if 0 /* reason */
is easier to read and clearer even for the compiler

melifaro marked 2 inline comments as done.

Address comments.

This revision is now accepted and ready to land.Feb 16 2021, 8:12 AM