Skip to content

emcute: never return from receive loop#12382

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
nmeum:pr/asymcute_continue
Oct 7, 2019
Merged

emcute: never return from receive loop#12382
miri64 merged 1 commit intoRIOT-OS:masterfrom
nmeum:pr/asymcute_continue

Conversation

@nmeum
Copy link
Copy Markdown
Member

@nmeum nmeum commented Oct 7, 2019 •
edited
Loading

Contribution description

The receive thread of the MQTT-SN implementation emcute returns from the packet receive loop if sock_udp_recv returns an error. This is, for instance, the case when a given packet doesn't fit into the supplied buffer. This condition can be triggered by anyone able to send large packets (> 512 byte) to the UDP socket. If so, this will stop the MQTT client from working until the device is restarted, i.e. emcute_run is invoked again.

RIOT/sys/net/application_layer/emcute/emcute.c

Lines 515 to 521 in 89afc37

while (1) {
ssize_t len = sock_udp_recv(&sock, rbuf, sizeof(rbuf), t_out, &remote);
if ((len < 0) && (len != -ETIMEDOUT)) {
LOG_ERROR("[emcute] error while receiving UDP packet\n");
return;
}

Testing procedure

My tap configuration is as follows:

4: tap0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
    link/ether 32:e2:ba:2b:07:d1 brd ff:ff:ff:ff:ff:ff
    inet6 fe80::30e2:baff:fe2b:7d1/64 scope link 
       valid_lft forever preferred_lft forever
  1. Enable debug in sys/net/application_layer/emcute/emcute.c.
  2. Compile and run examples/emcute_mqttsn.
  3. Connect to a bugos MQTT server address.
  4. Spoof a server reply with a packet exceeding the default buffer size (512 byte). This causes sock_udp_recv to return -ENOBUFS.
  5. Attempt to connect to a real working MQTT server (will fail).
MQTT-SN example application

Type 'help' to get started. Have a look at the README.md for more information.
> con fe80::30e2:baff:fe2b:7d1 2342

Spoof the reply, e.g. using:

head -c 525 < /dev/urandom  | busybox nc -p 2342 -u 'fe80::30e2:baff:fe2b:7d2%tap0' 1883

This should cause emcute_run to return and output the message [emcute] error while receiving UDP packet. Afterwards, it should no longer be possible to connect to a working MQTT-SN server since emcute will nerver receive the CONNACK reply from it since it doesn't read from the socket anymore.

Impact

Denial of service, a spoofed server message will cause the emcute client to stop working until emcute_run is invoked again which is usually the case when the device is restarted.

@nmeum
emcute: never return from receive loop
74e19d4 
Without this change an attacker would be able to stop the emcute server
by sending a crafted packet triggering this branch. The solution is
using `continue` instead of `return`.
@benpicco benpicco requested a review from haukepetersen October 7, 2019 14:00
@benpicco 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: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 7, 2019
benpicco
benpicco approved these changes Oct 7, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change makes sense, the emcute server thread should not exit on client error.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 7, 2019

Will try to remember to add a test case for that to #11823.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 7, 2019

Ok, wasn't that hard...: edae2c2. I was able to confirm this bug fix using that patch and cherry-picking 74e19d4 and 10a3f3e on top (and without of course).

@miri64 miri64 merged commit 999a771 into RIOT-OS:master Oct 7, 2019
@miri64 miri64 mentioned this pull request Oct 7, 2019
Merged
@kb2ma kb2ma added this to the Release 2019.10 milestone Oct 28, 2019
@nmeum nmeum mentioned this pull request Jan 17, 2020
Merged
@nmeum nmeum mentioned this pull request Jan 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

Assignees

No one assigned

Labels

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: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2019.10

Development

Successfully merging this pull request may close these issues.

4 participants

@nmeum @miri64 @benpicco @kb2ma