You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/09/23 14:57:20 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #4603: [SECURITY]net/tcp: sanity check for the listen address
anchao opened a new pull request #4603:
URL: https://github.com/apache/incubator-nuttx/pull/4603
## Summary
net/tcp: sanity check for the listen address
TCP stack only checks the visitor's port number on listening port,
which will cause external links to arbitrarily access local resources in the nuttx system:
```
client NUTTX server (192.168.1.101)
listen (127.0.0.1):8888
connect (192.168.1.101:8888) -> accept (127.0.0.1):8888 (success) <-- here is the issue.
listen (0.0.0.0):8889
connect (192.168.1.101:8889) -> accept (0.0.0.0):8889 (success)
listen (192.168.1.101):8890
connect (192.168.1.101:8890) -> accept (192.168.1.101):8890 (success)
```
In this PR we added an address check on listener port and reject malicious connections if the address check failure
```
client NUTTX server (192.168.1.101)
listen (127.0.0.1):8888
connect (192.168.1.101:8888) -> accept (127.0.0.1):8888 (Reject)
```
## Impact
tcp stack accept
## Testing
tcp test connect to the nuttx
```
client NUTTX server (192.168.1.101)
listen (127.0.0.1):8888
connect (192.168.1.101:8888) -> accept (127.0.0.1):8888 (Reject)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4603: [SECURITY]net/tcp: sanity check for the listen address
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4603:
URL: https://github.com/apache/incubator-nuttx/pull/4603#discussion_r715268741
##########
File path: net/tcp/tcp_listen.c
##########
@@ -100,9 +104,35 @@ FAR struct tcp_conn_s *tcp_findlistener(uint16_t portno)
if (conn && conn->lport == portno)
#endif
{
- /* Yes.. we found a listener on this port */
+#ifdef CONFIG_NET_IPv6
+# ifdef CONFIG_NET_IPv4
+ if (domain == PF_INET6)
+# endif
+ {
+ if (net_ipv6addr_cmp(conn->u.ipv6.laddr, uaddr->ipv6.laddr) ||
Review comment:
Yes, the header file which provided net_ipv6addr_cmp macro should include string.h to make self contained.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4603: [SECURITY]net/tcp: sanity check for the listen address
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4603:
URL: https://github.com/apache/incubator-nuttx/pull/4603#issuecomment-926293130
> > The changes seem fine, but I may lack the knowledge on the TCP stack for properly evaluating them.
> > Just a question: although it may be a security concern, isn't it valid to listen just to a port and accept connections from any address?
>
> yes, that is why the code pass the check if anyone of two addresses match. The hardcode one represent the any address. Caller can specify the netdev ip to accept the connection from only that device, or all zero ip to accept the connection from any netdev. Actually, this behaivour specify in the spec.
>
> > If I understood correctly, this won't be possible anymore, right?
>
> See the above comment.
Oh, right. I had misunderstood that part of the code. Thanks for the clarification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4603: [SECURITY]net/tcp: sanity check for the listen address
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4603:
URL: https://github.com/apache/incubator-nuttx/pull/4603#issuecomment-926290037
> The changes seem fine, but I may lack the knowledge on the TCP stack for properly evaluating them.
> Just a question: although it may be a security concern, isn't it valid to listen just to a port and accept connections from any address?
yes, that is why the code pass the check if anyone of two addresses match. The hardcode one represent the any address. Caller can specify the netdev ip to accept the connection from only that device, or all zero ip to accept the connection from any netdev. Actually, this behaivour specify in the spec.
> If I understood correctly, this won't be possible anymore, right?
See the above comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4603: [SECURITY]net/tcp: sanity check for the listen address
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4603:
URL: https://github.com/apache/incubator-nuttx/pull/4603#issuecomment-926120995
The changes seem fine, but I may lack the knowledge on the TCP stack for properly evaluating them.
Just a question: although it may be a security concern, isn't it valid to listen just to a port and accept connections from any address?
If I understood correctly, this won't be possible anymore, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #4603: [SECURITY]net/tcp: sanity check for the listen address
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4603:
URL: https://github.com/apache/incubator-nuttx/pull/4603
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #4603: [SECURITY]net/tcp: sanity check for the listen address
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #4603:
URL: https://github.com/apache/incubator-nuttx/pull/4603#discussion_r715287743
##########
File path: net/tcp/tcp_listen.c
##########
@@ -100,9 +104,35 @@ FAR struct tcp_conn_s *tcp_findlistener(uint16_t portno)
if (conn && conn->lport == portno)
#endif
{
- /* Yes.. we found a listener on this port */
+#ifdef CONFIG_NET_IPv6
+# ifdef CONFIG_NET_IPv4
+ if (domain == PF_INET6)
+# endif
+ {
+ if (net_ipv6addr_cmp(conn->u.ipv6.laddr, uaddr->ipv6.laddr) ||
Review comment:
Thanks, let me fix it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4603: [SECURITY]net/tcp: sanity check for the listen address
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4603:
URL: https://github.com/apache/incubator-nuttx/pull/4603#discussion_r715110875
##########
File path: net/tcp/tcp_listen.c
##########
@@ -100,9 +104,35 @@ FAR struct tcp_conn_s *tcp_findlistener(uint16_t portno)
if (conn && conn->lport == portno)
#endif
{
- /* Yes.. we found a listener on this port */
+#ifdef CONFIG_NET_IPv6
+# ifdef CONFIG_NET_IPv4
+ if (domain == PF_INET6)
+# endif
+ {
+ if (net_ipv6addr_cmp(conn->u.ipv6.laddr, uaddr->ipv6.laddr) ||
Review comment:
CI is broken due to missing `string.h` include
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org