Vincent Gross <vgr...@openbsd.org> writes:
> Le Mon, 13 Jun 2016 07:35:16 +0200,
> j...@wxcvbn.org (Jeremie Courreges-Anglas) a écrit :
>
>> j...@wxcvbn.org (Jeremie Courreges-Anglas) writes:
>>
>> > cc'ing sthen since he also has interest in IP_SENDSRCADDR
>> >
>> > Jeremie Courreges-Anglas <j...@wxcvbn.org> writes:
>> >
>> >> Vincent Gross <vgr...@openbsd.org> writes:
>> >>
>> >>> This diff adds support for IP_SENDSRCADDR cmsg on UDP sockets. As
>> >>> for udp6_output(), we check that the source address+port is
>> >>> available only if inp_laddr != *
>> >>
>> >> Your last IP_SENDSRCADDR diff didn't have that check, I think it is
>> >> harmful. If the socket is not bound then there is effectively no
>> >> check performed by in_pcbaddrisavail(), thus I can use any random
>> >> address. Other than this additional bypass check, your diff looks
>> >> good to me.
>> >>
> [...]
>> >>
>> >> I haven't checked yet whether udp6_output is also affected. If you
>> >> folks already know that it isn't, please let me know.
>>
>> The answer is "no", a few tests can't trigger the same problem. IIUC
>> in6_selectsrc is responsible for rejection of non-local systems.
>> Maybe we should take the same approach in netinet/, and extend
>> in_selectsrc()?
>>
>> --
>
> While validating source address inside selection functions is the right
> direction, I don't think it would be a good thing to extend further
> in_selectsrc() prototype.
I find it nice to have all the source address selection in one place.
Or do you have another refactoring in mind?
> However it is easy to add a check while
> processing cmsg.
>
> rev2 below. Ok ?
Nits below, looks fine otherwise. The checks do detect addresses not
configured on the system and overlaps of bound sockets.
>
> diff --git a/share/man/man4/ip.4 b/share/man/man4/ip.4
> index 111432b..154b0d1 100644
> --- a/share/man/man4/ip.4
> +++ b/share/man/man4/ip.4
> @@ -290,6 +290,27 @@ cmsg_len = CMSG_LEN(sizeof(u_int))
> cmsg_level = IPPROTO_IP
> cmsg_type = IP_RECVRTABLE
> .Ed
> +.Pp
> +If the
> +.Dv IP_SENDSRCADDR
> +option is passed to a
> +.Xr sendmsg 2
> +call on a
> +.Dv SOCK_DGRAM
> +socket, the address passed along the
> +.Vt cmsghdr
> +structure will be used as the source of the outgoing
> +.Tn UDP
> +datagram. The
> +.Vt cmsghdr
> +fields for
> +.Xr sendmsg 2
> +have the following values:
I would have worded it "should have" here, since these are the values
that the developer is supposed to pass.
> +.Bd -literal -offset indent
> +cmsg_len = CMSG_LEN(sizeof(struct in_addr))
> +cmsg_level = IPPROTO_IP
> +cmsg_type = IP_SENDSRCADDR
> +.Ed
> .Ss "Multicast Options"
> .Tn IP
> multicasting is supported only on
> diff --git a/sys/netinet/in.h b/sys/netinet/in.h
> index adb1b30..bf8c95d 100644
> --- a/sys/netinet/in.h
> +++ b/sys/netinet/in.h
> @@ -307,6 +307,7 @@ struct ip_opts {
> #define IP_RECVRTABLE 35 /* bool; receive rdomain w/dgram */
> #define IP_IPSECFLOWINFO 36 /* bool; IPsec flow info for dgram */
> #define IP_IPDEFTTL 37 /* int; IP TTL system default */
> +#define IP_SENDSRCADDR 38 /* struct in_addr; source address
> to use */
>
> #define IP_RTABLE 0x1021 /* int; routing table, see SO_RTABLE */
> #define IP_DIVERTFL 0x1022 /* int; divert direction flag opt */
> diff --git a/sys/netinet/udp_usrreq.c b/sys/netinet/udp_usrreq.c
> index 1feea11..401ed7a 100644
> --- a/sys/netinet/udp_usrreq.c
> +++ b/sys/netinet/udp_usrreq.c
> @@ -888,6 +888,7 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct mbuf
> *addr,
> struct sockaddr_in *sin = NULL;
> struct udpiphdr *ui;
> u_int32_t ipsecflowinfo = 0;
> + struct sockaddr_in src_sin;
> int len = m->m_pkthdr.len;
> struct in_addr *laddr;
> int error = 0;
> @@ -906,6 +907,8 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct mbuf
> *addr,
> goto release;
> }
>
> + memset(&src_sin, 0, sizeof(src_sin));
> +
> if (control) {
> u_int clen;
> struct cmsghdr *cm;
> @@ -939,9 +942,20 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct
> mbuf *addr,
> cm->cmsg_level == IPPROTO_IP &&
> cm->cmsg_type == IP_IPSECFLOWINFO) {
> ipsecflowinfo = *(u_int32_t *)CMSG_DATA(cm);
> - break;
> - }
> + } else
> #endif
> + if (cm->cmsg_len == CMSG_LEN(sizeof(struct in_addr)) &&
> + cm->cmsg_level == IPPROTO_IP &&
> + cm->cmsg_type == IP_SENDSRCADDR) {
> + bzero(&src_sin, sizeof(src_sin));
No need for bzero here since you unconditionally call memset above.
> + memcpy(&src_sin.sin_addr, CMSG_DATA(cm),
> + sizeof(struct in_addr));
> + src_sin.sin_family = AF_INET;
> + src_sin.sin_len = sizeof(src_sin);
> + /* no check on reuse done when sin->sin_port ==
> 0 */
> + if ((error = in_pcbaddrisavail(inp, &src_sin,
> 0, curproc)))
Lines > 80 chars.
> + goto release;
> + }
> clen -= CMSG_ALIGN(cm->cmsg_len);
> cmsgs += CMSG_ALIGN(cm->cmsg_len);
> } while (clen);
> @@ -980,6 +994,17 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct
> mbuf *addr,
> if (error)
> goto release;
> }
> +
> + if (src_sin.sin_len > 0 &&
> + src_sin.sin_addr.s_addr != INADDR_ANY &&
> + src_sin.sin_addr.s_addr != inp->inp_laddr.s_addr) {
> + src_sin.sin_port = inp->inp_lport;
> + if (inp->inp_laddr.s_addr != INADDR_ANY &&
> + (error =
> + in_pcbaddrisavail(inp, &src_sin, 0, curproc)))
> + goto release;
> + laddr = &src_sin.sin_addr;
> + }
> } else {
> if (inp->inp_faddr.s_addr == INADDR_ANY) {
> error = ENOTCONN;
>
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
