gnrc_sock_udp: choose random ephemeral port #13253

nmeum · 2020-01-31T17:10:02Z

Contribution description

While writing a PoC for #12293 and #12382 I noticed that RIOT does not randomize its ephemeral currently. This circumstance also made it significantly easier to exploit #10739. The first port value is 49152 and the value is simply sequential incremented after that. I consider this a security issue as it eases spoofing replies why this is a problem is hopefully illustrated by the previously mentioned issues.

While working on this change, I noticed that gnrc_sock_udp already depends on the random module in Makefile.dep, even though it does not use it. This made me curious and I discovered that it was removed, without much further discussion, in #6212:

I was kind of proud to get rid of the random() …

I personally think that this is a bad idea and an unnecessary security risk. If there is any specific reason why I should not be randomized please let me know.

Testing procedure

I tested this change using examples/asymcute_mqttsn/ simply build the application, start it. Try to connect to a non-existing MQTT server and check using wireshark/tcpdump that it uses a random port as the UDP source port.

actually there was a discussion regarding random or not between @OlegHahm and me (@smlng), the reasoning was to not introduce the additional overhead of random by default. However, we also wanted to discuss this (or the implementation according to RFC 6056) separately of the PR #6212 - which sadly never happened.

Maybe this PR takes this up now -

smlng · 2020-02-03T07:16:07Z

on another note: (to me) in the past we (RIOT developers) were more into runtime and binary optimisation back when, but today I generally agree that features and security (should) come first. So I'm in favour of (re)introducing random here, but please review RFC 6056 too.

Implements a random ephemeral port selection as per the second algorithm from RFC 6056, see https://tools.ietf.org/html/rfc6056#section-3.3.2.

nmeum · 2020-02-03T16:01:37Z

actually there was a discussion regarding random or not between @OlegHahm and me (@smlng), the reasoning was to not introduce the additional overhead of random by default.

Thanks for pointing that out, I somehow missed that thread in the pull request, sorry.

[…] please review RFC 6056 too.

Didn't know about that RFC, thanks a lot for referencing it. In fact, the comment above _get_dyn_port function claims that the function implements the Simple Hash-Based Port Selection Algorithm from this RFC. My original intention was to implement Another Simple Port Randomization Algorithm, though my version still multiplies with GNRC_SOCK_DYN_PORTRANGE_OFF by accident.

RFC 6056 has an interesting section on Choosing an Ephemeral Port Selection Algorithm. According to this section, the algorithm proposed here "increase the chances of port number collisions, which could lead to the failure of a connection establishment attempt.". However, in my experience constrained devices establish far less connections than conventional ones. For this reason, I deem collisions to be less likely and have a personal preference towards this algorithm as it chooses a new truly random port for each socket.

I will update the PR to make sure it implements Another Simple Port Randomization Algorithm correctly. If you prefer a different algorithm let me know.

sys/net/gnrc/sock/udp/gnrc_sock_udp.c

Still check if the port is unused if no socket has been given.

nmeum · 2020-02-17T09:21:20Z

@smlng seems the assumption about sock never being null is incorrect for tests_gnrc_sock_udp:

main(): This is RIOT! (Version: buildtest)
Calling test_sock_udp_create__EADDRINUSE()
Calling test_sock_udp_create__EAFNOSUPPORT()
Calling test_sock_udp_create__EINVAL_addr()
Calling test_sock_udp_create__EINVAL_netif()
Calling test_sock_udp_create__no_endpoints()
Calling test_sock_udp_create__only_local()
Calling test_sock_udp_create__only_local_port0()
Calling test_sock_udp_create__only_local_reuse_ep()
Calling test_sock_udp_create__only_remote()
Calling test_sock_udp_create__full()
Calling test_sock_udp_recv__EADDRNOTAVAIL()
Calling test_sock_udp_recv__EAGAIN()
Calling test_sock_udp_recv__ENOBUFS()
Calling test_sock_udp_recv__EPROTO()
Calling test_sock_udp_recv__ETIMEDOUT()
 * Calling sock_udp_recv()
 * (timed out with timeout 1000000)
Calling test_sock_udp_recv__socketed()
Calling test_sock_udp_recv__socketed_with_remote()
Calling test_sock_udp_recv__socketed_with_port0()
Calling test_sock_udp_recv__unsocketed()
Calling test_sock_udp_recv__unsocketed_with_remote()
Calling test_sock_udp_recv__with_timeout()
Calling test_sock_udp_recv__non_blocking()
Calling test_sock_udp_send__EAFNOSUPPORT()
Calling test_sock_udp_send__EINVAL_addr()
Calling test_sock_udp_send__EINVAL_netif()
Calling test_sock_udp_send__EINVAL_port()
Calling test_sock_udp_send__ENOTCONN()
Calling test_sock_udp_send__socketed_no_local_no_netif()
Calling test_sock_udp_send__socketed_no_netif()
Calling test_sock_udp_send__socketed_no_local()
Calling test_sock_udp_send__socketed()
Calling test_sock_udp_send__socketed_other_remote()
Calling test_sock_udp_send__unsocketed_no_local_no_netif()
Calling test_sock_udp_send__unsocketed_no_netif()
Calling test_sock_udp_send__unsocketed_no_local()
Calling test_sock_udp_send__unsocketed()
Calling test_sock_udp_send__no_sock_no_netif()
sys/net/gnrc/sock/udp/gnrc_sock_udp.c:73 => 0x565eb1fb
*** RIOT kernel panic:
FAILED ASSERTION.

	pid | name                 | state    Q | pri | stack  ( used) | base addr  | current     
	  - | isr_stack            | -        - |   - |   8192 (   -1) | 0x56610580 | 0x56610580
	  1 | idle                 | pending  Q |  15 |   8192 (  568) | 0x5660e3a0 | 0x56610210 
	  2 | main                 | running  Q |   7 |  12288 ( 2992) | 0x5660b3a0 | 0x5660e210 
	  3 | ipv6                 | bl rx    _ |   4 |   8192 ( 1440) | 0x56619520 | 0x5661b390 
	  4 | udp                  | bl rx    _ |   5 |   8192 ( 1264) | 0x566174c0 | 0x56619330 
	    | SUM                  |            |     |  45056 ( 6264)

*** halted.

I changed the check to:

if ((sock != NULL && (sock->flags & SOCK_FLAGS_REUSE_EP)) ||
    !_dyn_port_used(port))

smlng · 2020-02-18T07:44:00Z

so there was a reason for the check after all 😉 didn't remember that.

smlng · 2020-02-18T09:08:21Z

sys/net/gnrc/sock/udp/gnrc_sock_udp.c

- if ((sock == NULL) || (sock->flags & SOCK_FLAGS_REUSE_EP) ||
- !_dyn_port_used(port)) {
+ (random_uint32() % GNRC_SOCK_DYN_PORTRANGE_NUM);
+ if ((sock && (sock->flags & SOCK_FLAGS_REUSE_EP)) || !_dyn_port_used(port)) {


&&? I'd suggest to leave as it was before, right?

I thought we agreed that the previous check was broken:

mhm, you're right the '||' doesn't make sense here.

To recap, the previous check would not check for local port collisions if sock is NULL, which seems very wrong to me.

sock == NULL || (sock->flags & SOCK_FLAGS_REUSE_EP) basically is sock → (sock->flags & SOCK_FLAGS_REUSE_EP) in boolean logic. The logical implication allows basically all fire and forget "socks" (i.e. sock == NULL) to be configured to use reusable ports.

sock reuse unused sock → reuse sock && reuse old new

0 0 0 1 0 1 0

0 0 1 1 0 1 1

0 1 0 1 0 1 0

0 1 1 1 0 1 1

1 0 0 0 0 0 0

1 0 1 0 0 1 1

1 1 0 1 1 1 1

1 1 1 1 1 1 1

While technically this would be in line with GNRC below, I think for the sock interface this is not desirable. So I agree with @nmeum here that the new change makes more sense.

miri64 · 2020-02-19T13:39:25Z

@smlng seems the assumption about sock never being null is incorrect for tests_gnrc_sock_udp:

Yepp, very useful for simple fire-and-forget solutions ;-)

nmeum · 2020-03-24T22:52:18Z

Any reason why this hasn't been merged yet? IMHO this change is straightforward. I would also like to point out that RFC 6056 explicitly discourages using Algorithm 3 (which is incorrectly implemented at the moment anyhow as _get_dyn_port uses a constant instead of a hash function) without a cryptographic hash function with the proper arguments and advertises using Algorithm 2 then. Maybe someone else can take a look at this since @smlng hasn't responded lately. @miri64 maybe?

miri64 · 2020-03-26T12:36:54Z

I suspect @smlng disappeared in children-assisted home office ;-) so let me have a look.

miri64

ACK. Tested with tests/gnrc_sock_udp on native and samr21-xpro. Both still succeed.

miri64 · 2020-03-26T12:44:16Z

sys/net/gnrc/sock/udp/gnrc_sock_udp.c

- if ((sock == NULL) || (sock->flags & SOCK_FLAGS_REUSE_EP) ||
- !_dyn_port_used(port)) {
+ (random_uint32() % GNRC_SOCK_DYN_PORTRANGE_NUM);
+ if ((sock && (sock->flags & SOCK_FLAGS_REUSE_EP)) || !_dyn_port_used(port)) {


sock == NULL || (sock->flags & SOCK_FLAGS_REUSE_EP) basically is sock → (sock->flags & SOCK_FLAGS_REUSE_EP) in boolean logic. The logical implication allows basically all fire and forget "socks" (i.e. sock == NULL) to be configured to use reusable ports.

sock reuse unused sock → reuse sock && reuse old new

0 0 0 1 0 1 0

0 0 1 1 0 1 1

0 1 0 1 0 1 0

0 1 1 1 0 1 1

1 0 0 0 0 0 0

1 0 1 0 0 1 1

1 1 0 1 1 1 1

1 1 1 1 1 1 1

While technically this would be in line with GNRC below, I think for the sock interface this is not desirable. So I agree with @nmeum here that the new change makes more sense.

miri64 · 2020-03-26T13:02:02Z

Last compile test was >1 month ago. Let's give that a rerun.

miri64 · 2020-03-26T13:31:55Z

Failing tests are unrelated and also fail in the current nightlies.

nmeum mentioned this pull request Jan 31, 2020

random: generate random seed at compile-time #13255

Closed

benpicco added Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jan 31, 2020

gnrc_sock_udp: choose random ephemeral port

b37bed8

Implements a random ephemeral port selection as per the second algorithm from RFC 6056, see https://tools.ietf.org/html/rfc6056#section-3.3.2.

nmeum force-pushed the pr/random_ephemeral_port branch from 79201d7 to b37bed8 Compare February 3, 2020 16:03

nmeum commented Feb 3, 2020

View reviewed changes

sys/net/gnrc/sock/udp/gnrc_sock_udp.c Outdated Show resolved Hide resolved

smlng self-assigned this Feb 5, 2020

gnrc_sock_udp: fix null check in port generation function

5cf4338

Still check if the port is unused if no socket has been given.

nmeum force-pushed the pr/random_ephemeral_port branch from 310a2a3 to 5cf4338 Compare February 17, 2020 09:21

nmeum requested review from cgundogan, haukepetersen, miri64 and PeterKietzmann as code owners February 17, 2020 09:21

smlng reviewed Feb 18, 2020

View reviewed changes

miri64 approved these changes Mar 26, 2020

View reviewed changes

miri64 removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Mar 26, 2020

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 26, 2020

miri64 merged commit f39cfc7 into RIOT-OS:master Mar 26, 2020

leandrolanzieri added this to the Release 2020.04 milestone Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gnrc_sock_udp: choose random ephemeral port #13253

gnrc_sock_udp: choose random ephemeral port #13253

nmeum commented Jan 31, 2020 •

edited

Loading

smlng commented Feb 3, 2020

smlng commented Feb 3, 2020

nmeum commented Feb 3, 2020

nmeum commented Feb 17, 2020

smlng commented Feb 18, 2020

smlng Feb 18, 2020

nmeum Feb 18, 2020 •

edited

Loading

nmeum Feb 24, 2020

miri64 Mar 26, 2020

miri64 commented Feb 19, 2020

nmeum commented Mar 24, 2020 •

edited

Loading

miri64 commented Mar 26, 2020

miri64 left a comment

miri64 Mar 26, 2020

miri64 commented Mar 26, 2020

miri64 commented Mar 26, 2020

gnrc_sock_udp: choose random ephemeral port #13253

gnrc_sock_udp: choose random ephemeral port #13253

Conversation

nmeum commented Jan 31, 2020 • edited Loading

Contribution description

Testing procedure

Related

smlng commented Feb 3, 2020

smlng commented Feb 3, 2020

nmeum commented Feb 3, 2020

nmeum commented Feb 17, 2020

smlng commented Feb 18, 2020

smlng Feb 18, 2020

Choose a reason for hiding this comment

nmeum Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

nmeum Feb 24, 2020

Choose a reason for hiding this comment

miri64 Mar 26, 2020

Choose a reason for hiding this comment

miri64 commented Feb 19, 2020

nmeum commented Mar 24, 2020 • edited Loading

miri64 commented Mar 26, 2020

miri64 left a comment

Choose a reason for hiding this comment

miri64 Mar 26, 2020

Choose a reason for hiding this comment

miri64 commented Mar 26, 2020

miri64 commented Mar 26, 2020

nmeum commented Jan 31, 2020 •

edited

Loading

nmeum Feb 18, 2020 •

edited

Loading

nmeum commented Mar 24, 2020 •

edited

Loading