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 2022/04/26 12:07:55 UTC
[GitHub] [incubator-nuttx] XinStellaris opened a new pull request, #6154: net/tcp:make initial tcp port more random
XinStellaris opened a new pull request, #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154
Signed-off-by: 田昕 <ti...@xiaomi.com>
## Summary
tcp_selectport choose initial port based on system time, this is not so random if a tcp connection is established just after system is booted. Especially when we have a relatively low freq cpu timer(in our case, 10ms per tick).
If tcp port remains the same after a reset, there is a great chance tcp connection fails.
## Impact
tcp port selection, tcp connection
## Testing
Vela tested
--
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] pkarashchenko commented on a diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r895503448
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
I think that kernel has it's own implementation of libc and we can use use it as well. However I've noticed another issue: `libs/libc/misc/lib_getrandom.c` use `read` instead of `_NX_READ`, `open` instead of `_NX_OPEN` and `close` instead of `_NX_CLOSE`. I think we need to examine libc code carefully and fix all similar places
--
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 diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r897988956
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
> > If your project use random_pool, but not /dev/[u]random, a possible solution is call arc4random_buf in getrandom too.
>
> This doesn't make sense, because getrandom can already use random_pool via `/dev/urandom` when `CONFIG_DEV_URANDOM_RANDOM_POOL` is defined. Also @XinStellaris said that `arc4random_buf` didn't work for them. I was saying that it _would_ work if `board_init_rngseed` was implemented and `CONFIG_BOARD_INITRNGSEED` enabled, and that it would be even better if that could happen automatically if `CONFIG_DEV_RANDOM` was defined. Calling `getrandom(..., GRND_RANDOM)` from `up_randompool_initialize` would be a bit weird (because of the same "calling libc functions from the kernel"), but at least `/dev/random` is registered before `/dev/urandom` so it would be possible.
>
If so the current implementation of getrandom is good.
--
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] pkarashchenko merged pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154
--
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] normanr commented on a diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
normanr commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r897684812
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
> If your project use random_pool, but not /dev/[u]random, a possible solution is call arc4random_buf in getrandom too.
This doesn't make sense, because getrandom can already use random_pool via `/dev/urandom` when `CONFIG_DEV_URANDOM_RANDOM_POOL` is defined. Also @XinStellaris said that `arc4random_buf` didn't work for them. I was saying that it _would_ work if `board_init_rngseed` was implemented and `CONFIG_BOARD_INITRNGSEED` enabled, and that it would be even better if that could happen automatically if `CONFIG_DEV_RANDOM` was defined. Calling `getrandom(..., GRND_RANDOM)` from `up_randompool_initialize` would be a bit weird (because of the same "calling libc functions from the kernel"), but at least `/dev/random` is registered before `/dev/urandom` so it would be possible.
> But do we really need the crypto strength random number generator here?
No, but it would be nice to have something that "just-works" with minimal configuration.
> It could be fixed by try /dev/random and then /dev/urandom.
We probably don't need the security of `random` for initial tcp port. I think reading using `urandom` first and falling back to `random` is fine - as long as `urandom` is properly seeded.
--
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 diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r895575691
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
Thanks
--
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 diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r896406323
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
> > I had tested arc4random_buf, it generated the exact same sequence when system is booted.
>
> `board_init_rngseed` needs to be implemented to fix this (and `CONFIG_BOARD_INITRNGSEED` enabled). Weirdly it doesn't look like that doesn't seem like random_pool will "self-init" if `ARCH_HAVE_RNG` is present.
>
If your project use random_pool, but not /dev/[u]random, a possible solution is call arc4random_buf in getrandom too.
> > This was the reason I didn't call arc4random_buf. This is unsafe, and will cause tcp connection to fail.
>
> Is getrandom any safer?
getranodm is just a wrapper function, whether is it safe depend on the real implementation.
> It just reads from `/dev/urandom` which (if it exists) probably isn't cyptographically secure.
But do we really need the crypto strength random number generator here?
> If that fails it falls back to reading from `/dev/random` (which requires `ARCH_HAVE_RNG`).
It could be fixed by try /dev/random and then /dev/urandom.
--
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] normanr commented on a diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
normanr commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r895359172
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
Isn't this a userspace library? Shouldn't kernel code just call `arc4random_buf` directly (conditional on `CONFIG_CRYPTO_RANDOM_POOL`)?
--
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] normanr commented on a diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
normanr commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r896375519
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
> I had tested arc4random_buf, it generated the exact same sequence when system is booted.
`board_init_rndseed` needs to be implemented to fix this (and `CONFIG_BOARD_INITRNGSEED` enabled). Weirdly it doesn't look like that doesn't seem like random_pool will "self-init" if `ARCH_HAVE_RNG` is present.
> This was the reason I didn't call arc4random_buf. This is unsafe, and will cause tcp connection to fail.
Is getrandom any safer? It just reads from /dev/urandom file which (if it exists) probably isn't cyptographically secure. If that fails it falls back to reading from /dev/random (which requires `ARCH_HAVE_RNG`).
--
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] normanr commented on a diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
normanr commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r897684812
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
> If your project use random_pool, but not /dev/[u]random, a possible solution is call arc4random_buf in getrandom too.
This doesn't make sense, because getrandom can already use random_pool via `/dev/urandom` when `CONFIG_DEV_URANDOM_RANDOM_POOL` is defined. Also @XinStellaris said that `arc4random_buf` didn't work for them. I was saying that it _would_ work if `board_init_rngseed` was implemented and `CONFIG_BOARD_INITRNGSEED` enabled, and that it would be even better if that could happen automatically if `CONFIG_DEV_RANDOM` was defined. Calling `getrandom(..., GRND_RANDOM)` from `up_randompool_initialize` would be a bit weird (because of the same "calling libc functions from the kernel"), but at least `/dev/random` is registered before `/dev/urandom` so it would be possible.
> But do we really need the crypto strength random number generator here?
No, but it would be nice to have something that "just-works" with minimal configuration.
> It could be fixed by try /dev/random and then /dev/urandom.
We probably don't need the security of `random` for initial tcp port. I think reading using `urandom` first and falling back to `random` is fine - as long as `urandom`
--
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 #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#issuecomment-1127009189
Fixed by https://github.com/apache/incubator-nuttx/pull/6154
--
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] normanr commented on a diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
normanr commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r896375519
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
> I had tested arc4random_buf, it generated the exact same sequence when system is booted.
`board_init_rndseed` needs to be implemented to fix this (and `CONFIG_BOARD_INITRNGSEED` enabled). Weirdly it doesn't look like that doesn't seem like random_pool will "self-init" if `ARCH_HAVE_RNG` is present.
> This was the reason I didn't call arc4random_buf. This is unsafe, and will cause tcp connection to fail.
Is getrandom any safer? It just reads from `/dev/urandom which (if it exists) probably isn't cyptographically secure. If that fails it falls back to reading from `/dev/random` (which requires `ARCH_HAVE_RNG`).
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
> I had tested arc4random_buf, it generated the exact same sequence when system is booted.
`board_init_rndseed` needs to be implemented to fix this (and `CONFIG_BOARD_INITRNGSEED` enabled). Weirdly it doesn't look like that doesn't seem like random_pool will "self-init" if `ARCH_HAVE_RNG` is present.
> This was the reason I didn't call arc4random_buf. This is unsafe, and will cause tcp connection to fail.
Is getrandom any safer? It just reads from `/dev/urandom` which (if it exists) probably isn't cyptographically secure. If that fails it falls back to reading from `/dev/random` (which requires `ARCH_HAVE_RNG`).
--
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] pkarashchenko commented on a diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r895553280
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
I created https://github.com/apache/incubator-nuttx/pull/6421 to address this.
--
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] XinStellaris commented on a diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
XinStellaris commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r895374200
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
I had tested arc4random_buf, it generated the exact same sequence when system is booted. This was the reason I didn't call arc4random_buf.
This is unsafe, and will cause tcp connection to fail.
--
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] normanr commented on a diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
normanr commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r896375519
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
> I had tested arc4random_buf, it generated the exact same sequence when system is booted.
`board_init_rngseed` needs to be implemented to fix this (and `CONFIG_BOARD_INITRNGSEED` enabled). Weirdly it doesn't look like that doesn't seem like random_pool will "self-init" if `ARCH_HAVE_RNG` is present.
> This was the reason I didn't call arc4random_buf. This is unsafe, and will cause tcp connection to fail.
Is getrandom any safer? It just reads from `/dev/urandom` which (if it exists) probably isn't cyptographically secure. If that fails it falls back to reading from `/dev/random` (which requires `ARCH_HAVE_RNG`).
--
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] normanr commented on a diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
normanr commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r896375519
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
> I had tested arc4random_buf, it generated the exact same sequence when system is booted.
`board_init_rndseed` needs to be implemented to fix this (and `CONFIG_BOARD_INITRNGSEED` enabled). Weirdly it doesn't look like that doesn't seem like random_pool will "self-init" if `ARCH_HAVE_RNG` is present.
> This was the reason I didn't call arc4random_buf. This is unsafe, and will cause tcp connection to fail.
Is getrandom any safer? It just reads from /dev/urandom file which (if it exists) probably isn't cyptographically secure. If that fails it falls back to reading from /dev/random (which requires `ARCH_HAVE_RNG`).
--
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] normanr commented on a diff in pull request #6154: net/tcp:make initial tcp port more random
Posted by GitBox <gi...@apache.org>.
normanr commented on code in PR #6154:
URL: https://github.com/apache/incubator-nuttx/pull/6154#discussion_r906870581
##########
net/tcp/tcp_conn.c:
##########
@@ -49,6 +49,7 @@
#include <assert.h>
#include <errno.h>
#include <debug.h>
+#include <sys/random.h>
Review Comment:
Yea, I was just surprised that kernel code was calling a libc function that was opening a file. I assume #6421 makes this work properly from the kernel context.
My other concern (of having to open a file descriptor) was partly fixed by #6438. Correcting the comment is easy compared to refactoring the code to avoid using the file descriptor.
--
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