net: socket: Introduce net_clear_sinzero()

[masayuki2009]

Summary

• In Fix usrsocktest errors nuttx-apps#85, we finally found that sin_zero field in sockaddr_in should be set to zero in the kernel space. This PR introduces net_clear_sinzero() for it.

Impact

• Some socket APIs such as accept(), getpeername(), getsockname() and recvfrom() are affected, if they are used with IPv4 address.

Testing

• I tested this PR with spresense:wifi with usrsocktest application which I modified to check all address field. Also, I confirmed spresense:rndis which does not use usrsock also works.

[xiaoxiang781216]

Can we zero out the whole addr before call si_getpeername/si_getsockename/si_accept/si_recvfrom?

[masayuki2009]

@xiaoxiang781216 Do you mean that if (add != NULL && addrlen > 0), then memset(addr, 0, addrlen) ?

[xiaoxiang781216]

Yes, something like this, but we need check each implementation don't overwrite the unused field with the random bit again.

[xiaoxiang781216]

It doesn't make sense to just clear IPv4 address, we should clear all unused field to zero for all address family, otherwise the similiar error will report again and again.

[masayuki2009]

@xiaoxiang781216 In my understanding, padding field only exists in sockaddr_in (i.e. IPv4 family). So that's why I added ifdef CONFIG_NET_IPv4.

[xiaoxiang781216]

BSD socket is a general interface, many protocol build on top of it: IPv4, IPv6, Unix domain, Bluetooth etc. Each protocol define his own address repsentation, and then may add the padding field like IPv4. We have to find a general solution.

[masayuki2009]

Isn't it the application's responsibility to clear sin_zero,not the OS. Googling for examples suggest it is the application responsibility. If that is there case, there should be no OS change.

@patacongo I thought it's the application's responsibility to clear sin_zero. But as long as I checked Linux kernel, it clears in the network stack. (perhaps to accept network applications having an wrong manner?)

[patacongo]

I thought it's the application's responsibility to clear sin_zero

For input addresses, the form of the address parameter will be "FAR const struct sockaddr *addr" and the application must set the sin_zero field. These are read-only for the OS and must never be modified by the OS. For example when a struct sockaddr_in is passed to sendto(). These may not be written to under any circumstances (and may even lie in ROM).

If Linux is modifying read-only values, it is screwed up. We do not want duplicate such errors in the this OS.

For output addresses, the form form of the address parameter will be "FAR struct sockaddr *addr). In that case, the OS must also set the sin_zero field before returning,

I really dislike this solution. I think that the sin_zero field should always be set where ever sin_port and sin_addr is set. You chould not have to check anyhting in those cases just memset() the field. There should not be any centralizing logic to retroactively sets sin_zero.

I am closing this PR because I think it is not correct. It is difficult to understand the logic completely form just looking at the differences, and it may be correct if it is applied ONLY to output addresses. However, even in that case. I do not believe that it is the correct way to implement the zeroing. That should be done when sin_port and sin_addr are set.

[patacongo]

Let me be clearer. Whenever logic within the OS inializes a sockaddr_in structure, the code sequence should always be modified to set sin_zero() like:

addr->sin_port = AF_INET;
addr->sin_addr.s_addr = addr;
memset(addr->sin_zero, 0, sizeof(addr->sin_zero));

There should be no centralized logic to try to set sin_zero in some other place.

If the sockaddr_in structure is not created by the OS, it must never modify it.

[xiaoxiang781216]

@patacongo does it make sense that we call memset on the whole addr before call si_getpeername/si_getsockename/si_accept/si_recvfrom?
It's simple and flexible for any address family which may define the reserved field too.

[patacongo]

does it make sense that we call memset on the whole addr before call si_getpeername/si_getsockename/si_accept/si_recvfrom?

I dislike that solution too. It is a ugly hack. We should do it right. sin_zerio is not different then any other field in sockadd_in. The clean, easily unstandable way is:

addr->sin_port = AF_INET;
addr->sin_addr.s_addr = addr;
memset(addr->sin_zero, 0, sizeof(addr->sin_zero));

Let's not take ugly shortcuts.

But having said that, it is not as ugly as the original proposal. It is just not as straightforward.

[patacongo]

People on the internet are saying that leaving the sin_zero field is causing some kinds of issues. I don't know how that could be unless the sin_zero actually contains values in some cases. Is there there some use case where sin_zero would not be zero (for example, some protocol-specific usage).

[xiaoxiang781216]

The problem is that:
accept/getsockname/getpeername/recvfrom are general functions, shouldn't assume addr must be IPv4 and then contain sin_zero. If we want to just zero sin_zero, it's better to move the code into inet/.

[patacongo]

But the memset of the whole structure would be wasteful for IPv6 addresses.

accept/getsockname/getpeername/recvfrom are general functions, shouldn't assume addr must be IPv4 and then contain sin_zero.

The distinction between IPv4 and IPv6 is made at a lower level than accept/getsockname/getpeername/recvfrom. At that lower level, the IPv4 specific code and always assume that sin_zero is present. If sin_port and sin_addr are set then you know that sin_zero can be set too.. always.

For example:

accept->psock_accept->inet_accept->tcp_psock_accept

then the address is initialized like:

106 #ifdef CONFIG_NET_IPv4
114         {
115           FAR struct sockaddr_in *inaddr = (FAR struct sockaddr_in *)addr;
116
117           inaddr->sin_family = AF_INET;
118           inaddr->sin_port   = conn->rport;
119           net_ipv4addr_copy(inaddr->sin_addr.s_addr, conn->u.ipv4.raddr);
120
121           *addrlen = sizeof(struct sockaddr_in);
122         }
123 #endif /* CONFIG_NET_IPv4 */

The memset of sin_zero belongs between lines 119 and 120. Easy and perfect!

sin_zero should be initialized at the very, very lowest level at the same time that sin_port and sin_addr are initialize. That is the simplest and can never fail. I have not looked at every case, but I believe that all reduce to this simplest, most straight-forward, monst maintainable, most understanable solution.

[xiaoxiang781216]

Yes, this is what I want to say:
1.put to IPv4 case if we just zero sin_zero field or
2.put to common place but we set the whole structure
The mix(set sin_zero in common place) isn't a good implementation.

[patacongo]

Okay so I did the work and checked out where all IPv4 sockadd_in structures are initialed. There are many, many more than just accept/getsockname/getpeername/recvfrom .

I added memset(sin_zero, 0, sizeof(sin_zero)); to each of them and create pr377.

[patacongo]

But as long as I checked Linux kernel, it clears in the network stack.

Ignorinig the read-only 'const' would be a violation of C principles. We should not do that.

I think it would be legal to test if sin_zero is all zero and, if not, return an error. That is correct, but would probably annoy a lot of people.

[patacongo]

One thing we could do it if matters enough to you is to make a copy of socketadd_in input then clear the sin_zero field in the copy. I don't recommend this for embedded systems but this is most likely what Linux is doing. Linux never accesses any user pointers without going though severl security layers so, for Linux, is it simpler to copy input argument (if they are not large) rather than go through the security measures on every access.

If there are no security macros in the code you are looking at, you can be assured that linux is modifying a copy of the user input, not the actual user input itself. That should never be done for a const input.

[patacongo]

AFAIK nothing in the NuttX network stack cares about the content of sin_zero. There would be nothing gained for NuttX by clearing that field for input sockaddr_in values.