Add missing sockaddr length and family validation to various protocols

[markj]

Summary

We have several protocol methods - pru_bind, pru_connect and pru_send -
which take a sockaddr as input. Protocols are required to validate the
sockaddr family and length, but in many cases this was not happening, or
was being done too late.

This diff adds requisite checks to various protocol entry points and in
some cases replaces existing checks with assertions. For now they are
open-coded but we should probably have a per-AF routine to do this.
Initially, though, let's make sure that we have sufficient checking.

Reported by: KASAN

[donner]

sizeof is a tricky operator, which may return a larger value than you would expect. This depends on the allocation scheme of the compiler, which knows how to pad a structure to the alignment and addressing requirements of the underlying hardware. So in for each of your cases it had to be checked if the comparison should be "==", "<", or ">=" to cope with remote data structures generated externally compiled software.

[markj]

In D29519#661804, @donner wrote:

sizeof is a tricky operator, which may return a larger value than you would expect. This depends on the allocation scheme of the compiler, which knows how to pad a structure to the alignment and addressing requirements of the underlying hardware. So in for each of your cases it had to be checked if the comparison should be "==", "<", or ">=" to cope with remote data structures generated externally compiled software.

I don't quite follow - the ABI assumes that different compilers will generate binary-compatible layouts of these structures. In some cases sizeof is indeed inappropriate, as in ng_socket.c for example. In many of these cases we were already performing checks of the form if (sin->sin_len != sizeof(struct sockaddr_in)), they were just happening too late.

[jtl]

Mark,

Thanks for doing this! It looks like a very positive change, and I'm sure there was a lot of effort put into finding the right way to clean up the code.

I have a general question about IPv4 addresses on IPv6 sockets which is really larger than your change. However, I think it may be worth resolving it prior to committing this patch. (Or, alternatively, it is probably worth at least making sure that this patch doesn't make an unintended change in the way IPv4 addresses work when used on IPv6 sockets.)

Jonathan

sys/netinet6/sctp6_usrreq.c

711

I'm just picking this as a good spot to insert a general question/concern/comment. (And, even though your code is a trigger for my comment, my comment is really largely independent of your proposed change and certainly not any sort of criticism of it.)

I personally remain a tiny bit confused about when we allow AF_INET/(struct sockaddr_in) on IPv6 sockets. My general understanding is that this is generally a legacy functionality related to IPV4 mapped addresses and should no longer occur. However, our code still seems to include places which allow IPv4 addresses to be used with IPv6 sockets.

It seems to me like this change may break that functionality in one or more places, if it is something we want to support. OTOH, if we no longer want to support this, we should probably intentionally clean that up as a separate change to the one which you are proposing.

So, can you (or anyone else on this review) give a summary of our position with respect to allowing AF_INET/(struct sockaddr_in) to be used on IPv6 sockets? Related, can anyone summarize our expected support for IPv4 mapped addresses?

It is possible that I'm completely confused and my questions are non-sensical. If so, I apologize in advance...

[markj]

sys/netinet6/sctp6_usrreq.c

711

I believe that we in fact do not allow AF_INET sockaddrs to be used with AF_INET6 sockets at all. Consider the !SCTP_IPV6_V6ONLY(inp) case here. We check IN6_IS_ADDR_V4MAPPED(&sin6->sin6_addr), i.e., we assume that the sockaddr is a sockaddr_in6. This assumption exists in the TCP and UDP code as well. In all cases we check IN6_IS_ADDR_V4MAPPED() before converting the sockaddr to a sockaddr_in with in6_sin6_2_sin().

[markj]

Ping? Any comments on the overall approach, or on the details of the change?

[tuexen]

In D29519#666585, @markj wrote:

Ping? Any comments on the overall approach, or on the details of the change?

Hi Mark,

we discussed this at the last transport call. We agreed that that handling should be consistent but wanted to check until the next transport call (about two weeks from now) what the consistent way would be. My understanding is:

(1) On an AF_INET socket, only sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.
(2) On an AF_INET6 socket, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a non-mapped address can be used.
(3) On an AF_INET6 socket which is V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can not be used.
(4) On an AF_INET6 socket which is not V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can be used.
(5) It is open, if on an AF_INET6 socket which is not V6_ONLY, sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.

I think the people in the transport call agreed (1)-(4). The discussion point was (5). We wanted to look at the code.

Are the corresponding checks OK for the various socket options which pass in IP-addresses? It looks like the diff covers only bind()/connect()/sendto()?

[jtl]

In D29519#666595, @tuexen wrote:

In D29519#666585, @markj wrote:

Ping? Any comments on the overall approach, or on the details of the change?

Hi Mark,

we discussed this at the last transport call. We agreed that that handling should be consistent but wanted to check until the next transport call (about two weeks from now) what the consistent way would be. My understanding is:

I believe @tuexen summarized it well. I appreciate him following up; I was supposed to add a note from the last meeting to this review, but hadn't yet done so.

Mark, you are more than welcome to attend the next transport call (April 22 at 1500 UTC) if you would like to discuss this in real time.

Jonathan

[markj]

In D29519#666595, @tuexen wrote:

In D29519#666585, @markj wrote:

Ping? Any comments on the overall approach, or on the details of the change?

Hi Mark,

we discussed this at the last transport call. We agreed that that handling should be consistent but wanted to check until the next transport call (about two weeks from now) what the consistent way would be. My understanding is:

(1) On an AF_INET socket, only sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.
(2) On an AF_INET6 socket, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a non-mapped address can be used.
(3) On an AF_INET6 socket which is V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can not be used.
(4) On an AF_INET6 socket which is not V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can be used.
(5) It is open, if on an AF_INET6 socket which is not V6_ONLY, sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.

I think the people in the transport call agreed (1)-(4). The discussion point was (5). We wanted to look at the code.

(5) may be an aspiration, but it is not true today. Take a look at, for example, udp6_bind() or udp6_connect(). They assume outright that the passed sockaddr is a sockaddr_in6. Is there some other layer that handles this that I'm missing?

Are the corresponding checks OK for the various socket options which pass in IP-addresses? It looks like the diff covers only bind()/connect()/sendto()?

That's a good question. in6_control() (an ioctl handler) does not validate the length, but that is less concerning since struct in6_ifreq and struct in6_aliasreq embed a struct sockaddr_in6.

The IPV6_2292NEXTHOP and IPV6_NEXTHOP sockopt handlers do indeed check specifically for AF_INET6. I do not see any other options which operate on sockaddrs. I could easily have missed something.

In D29519#666613, @jtl wrote:

Mark, you are more than welcome to attend the next transport call (April 22 at 1500 UTC) if you would like to discuss this in real time.

I will try to make that one.

[tuexen]

In D29519#666666, @markj wrote:

In D29519#666595, @tuexen wrote:

In D29519#666585, @markj wrote:

Ping? Any comments on the overall approach, or on the details of the change?

Hi Mark,

we discussed this at the last transport call. We agreed that that handling should be consistent but wanted to check until the next transport call (about two weeks from now) what the consistent way would be. My understanding is:

(1) On an AF_INET socket, only sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.
(2) On an AF_INET6 socket, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a non-mapped address can be used.
(3) On an AF_INET6 socket which is V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can not be used.
(4) On an AF_INET6 socket which is not V6_ONLY, sockaddr_in6 structures with sin6.family == AF_INET6 and sin6.len == sizeof(struct sockaddr_in6) and a mapped address can be used.
(5) It is open, if on an AF_INET6 socket which is not V6_ONLY, sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.

I think the people in the transport call agreed (1)-(4). The discussion point was (5). We wanted to look at the code.

(5) may be an aspiration, but it is not true today. Take a look at, for example, udp6_bind() or udp6_connect(). They assume outright that the passed sockaddr is a sockaddr_in6. Is there some other layer that handles this that I'm missing?

Maybe the IPPROTO_SCTP level socket options? There is at least a IPPROTO_SCTP level socket option SCTP_I_WANT_MAPPED_V4_ADDR (see https://tools.ietf.org/html/rfc6458#section-8.1.15 for the specification), but this handles to kernel -> userland direction. I think (that is what I need to double check) we support a mixture of sockaddr_in and sockaddr_in6 in calls like sctp_bindx() and sctp_connectx(), but I'm not sure.

Are the corresponding checks OK for the various socket options which pass in IP-addresses? It looks like the diff covers only bind()/connect()/sendto()?

That's a good question. in6_control() (an ioctl handler) does not validate the length, but that is less concerning since struct in6_ifreq and struct in6_aliasreq embed a struct sockaddr_in6.

The IPV6_2292NEXTHOP and IPV6_NEXTHOP sockopt handlers do indeed check specifically for AF_INET6. I do not see any other options which operate on sockaddrs. I could easily have missed something.

In D29519#666613, @jtl wrote:

Mark, you are more than welcome to attend the next transport call (April 22 at 1500 UTC) if you would like to discuss this in real time.

I will try to make that one.

[markj]

In D29519#666669, @tuexen wrote:

In D29519#666666, @markj wrote:

In D29519#666595, @tuexen wrote:

(5) It is open, if on an AF_INET6 socket which is not V6_ONLY, sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.

I think the people in the transport call agreed (1)-(4). The discussion point was (5). We wanted to look at the code.

(5) may be an aspiration, but it is not true today. Take a look at, for example, udp6_bind() or udp6_connect(). They assume outright that the passed sockaddr is a sockaddr_in6. Is there some other layer that handles this that I'm missing?

Maybe the IPPROTO_SCTP level socket options? There is at least a IPPROTO_SCTP level socket option SCTP_I_WANT_MAPPED_V4_ADDR (see https://tools.ietf.org/html/rfc6458#section-8.1.15 for the specification), but this handles to kernel -> userland direction. I think (that is what I need to double check) we support a mixture of sockaddr_in and sockaddr_in6 in calls like sctp_bindx() and sctp_connectx(), but I'm not sure.

I see, thanks. Indeed, those SCTP-specific entry points seem to handle both address families (and include corresponding checks on the sockaddrs). As far as I can tell, SCTP is unique in this regard. I believe this diff won't change anything with respect to sctp_bindx() and _connectx().

[tuexen]

In D29519#666731, @markj wrote:

In D29519#666669, @tuexen wrote:

In D29519#666666, @markj wrote:

In D29519#666595, @tuexen wrote:

(5) It is open, if on an AF_INET6 socket which is not V6_ONLY, sockaddr_in structures with sin.family == AF_INET and sin.len == sizeof(struct sockaddr_in) can be used.

I think the people in the transport call agreed (1)-(4). The discussion point was (5). We wanted to look at the code.

(5) may be an aspiration, but it is not true today. Take a look at, for example, udp6_bind() or udp6_connect(). They assume outright that the passed sockaddr is a sockaddr_in6. Is there some other layer that handles this that I'm missing?

Maybe the IPPROTO_SCTP level socket options? There is at least a IPPROTO_SCTP level socket option SCTP_I_WANT_MAPPED_V4_ADDR (see https://tools.ietf.org/html/rfc6458#section-8.1.15 for the specification), but this handles to kernel -> userland direction. I think (that is what I need to double check) we support a mixture of sockaddr_in and sockaddr_in6 in calls like sctp_bindx() and sctp_connectx(), but I'm not sure.

I see, thanks. Indeed, those SCTP-specific entry points seem to handle both address families (and include corresponding checks on the sockaddrs). As far as I can tell, SCTP is unique in this regard. I believe this diff won't change anything with respect to sctp_bindx() and _connectx().

I don't think either. But the point of the discussion was to get some consistent behaviour... That is why we wanted to look at the code before coming to a conclusion.

[markj]

Rework updates to sctp6_send(): permit both address families, and validate the
sockaddr length as sctp6_connect() and sctp6_bind() do.

[markj]

sys/netinet6/sctp6_usrreq.c

713

We are leaking mbufs here. Some of the existing checks in this function do as well, as do other protocols in some cases. I'm working on a separate patch for that.

[melifaro]

I have a general question about compatibility.
Recently I landed a similar change (D28668) in the rtsock space. It turned out that (older) applications do a lot of weird things: some send the length less than expected (recall sockaddr_in has a weird sin_zero field), some send zero-length, etc.
I'm pretty sure that some applications will break in some weird ways - just because we didn't have these checks in place before.
What's our position here? Do we want to let the apps break and insisting on fixing the code, or we consider guarding these checks under, for example #ifndef COMPAT_FREEBSD12?

sys/netgraph/ng_socket.c

243

Nit: + offsetof(struct sockaddr_ng, sg_data) ?

433

Same here

[markj]

In D29519#672110, @melifaro wrote:

I have a general question about compatibility.
Recently I landed a similar change (D28668) in the rtsock space. It turned out that (older) applications do a lot of weird things: some send the length less than expected (recall sockaddr_in has a weird sin_zero field), some send zero-length, etc.
I'm pretty sure that some applications will break in some weird ways - just because we didn't have these checks in place before.
What's our position here? Do we want to let the apps break and insisting on fixing the code, or we consider guarding these checks under, for example #ifndef COMPAT_FREEBSD12?

I think the situation here is not as bad as the routing socket code:

1. In common cases we were already validating sockaddr lengths, just not early enough.
2. getsockaddr() is responsible for allocating all of the sockaddrs covered by this diff. It does some basic validation (really just guards against zero-length sockaddrs). Note that when passing a sockaddr via bind(2), connect(2), etc.., it ignores the sa_len in the user-provided sockaddr.
3. These are standard APIs, and other operating systems perform this validation as well. Linux appears to be more relaxed at a cursory glance, it just requires that addrlen >= sizeof(struct sockaddr_foo). OpenBSD is strict.

I can see an argument for relaxing these checks to follow Linux's example, but in general I'd expect application behaviour here to be more normalized than it is for routing sockets. My plan is to give a reasonable MFC window to catch fallout, but I don't expect to have to provide compatibility ifdefs.

sys/netgraph/ng_socket.c

243

Ok, I will convert the - 2 below as well then.

[markj]

Use offsetof instead of hard-coded constants.