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

asymcute: Fix null pointer dereference #12293

Merged
merged 1 commit into from Sep 24, 2019

Conversation

nmeum
Copy link
Member

@nmeum nmeum commented Sep 24, 2019

Contribution description

This PR fixes various null pointer dereferences in asymcute. All of
these are due to the assumption that req->arg is always non-NULL.
Since an attacker can spoof mqtt replies this is not neccessarly the
case.

This assumption is made at various places in the code. I only tested
this with MQTTSN_SUBACK messages (_on_suback function) but the same
issue should apply to any function accessing req->arg without checking
for NULL.

Testing procedure

My tap setup is as follows:

4: tap0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
    link/ether 72:f3:e4:fd:13:9c brd ff:ff:ff:ff:ff:ff
    inet6 fe80::70f3:e4ff:fefd:139c/64 scope link
       valid_lft forever preferred_lft forever

On native:

$ make -C `examples/asymcute_mqttsn` all-valgrind
$ make -C `examples/asymcute_mqttsn` term-valgrind
main(): This is RIOT! (Version: UNKNOWN (builddir: /root/RIOT))
Asymcute MQTT-SN example application

Type 'help' to get started and have a look at the README.md for more information.
> connect myclient [fe80::70f3:e4ff:fefd:139c]:2342

Before the connect request timesout spoof a reply using:

printf CBMAAAIAAAA= | base64 -d | \
	busybox nc -p 2342 -u 'fe80::70f3:e4ff:fefd:139d%tap0' 49152

49152 should be the default ephemeral port. The packet must have the
server address as source address and the server port as source port.

Expected result: The packet should be rejected.
Actual result: Segmentation fault + invalid read of size 4.

Impact

The null pointer dereference should result in a crash on most
platforms. Thereby allowing a denial of service. The attacker must be
able to spoof a MQTT response which is easy as the ephemeral port is not
picked at random. Additionally, the attacker needs to know a pending
MQTT MsgId, however, those aren't picked at random either and there are
only 2^8 possible values.

This fixes a denial of service where an attacker would be able to cause
a NULL pointer dereference by sending a spoofed packet. This attack only
requires knowledge about pending message ids.
@miri64 miri64 added Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Sep 24, 2019
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 24, 2019
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK. Tested as described. Checking if the values are NULL also makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants