Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SECURITY]net/tcp: sanity check for the listen address #4603

Merged
merged 1 commit into from
Sep 24, 2021

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Sep 23, 2021

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)

@gustavonihei
Copy link
Contributor

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?

@xiaoxiang781216
Copy link
Contributor

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.

@gustavonihei
Copy link
Contributor

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.

Signed-off-by: chao.an <anchao@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit c132e5b into apache:master Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants