Invalid ADC commands sent via UDP will crash the app

Bug #1722364 reported by maksis on 2017-10-09
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
AirDC++
Undecided
Unassigned
ApexDC++
Undecided
Unassigned
DC++
Undecided
Unassigned
FlylinkDC++
Undecided
Unassigned
LinuxDC++
Undecided
Unassigned
StrongDC++
Undecided
Unassigned

Bug Description

The AdcCommand parsing function will throw ParseException on invalid commands: https://sourceforge.net/p/dcplusplus/code/ci/default/tree/dcpp/AdcCommand.cpp#l37

However, SearchManager (UDPServer in AirDC++) won't catch those exceptions at all: https://sourceforge.net/p/dcplusplus/code/ci/default/tree/dcpp/SearchManager.cpp#l286

As a result, you should be able to crash the app by sending the following raw text to the UDP port:

ARES T\n

(I used https://packetsender.com for testing)

maksis (maksis) wrote :

AirDC++ now uses the regular dispatch function for parsing ADC UDP commands that should fix the issue: https://github.com/airdcpp/airgit/blob/6a40613788e7ed8a7478f4f1789bbd142a98d231/airdcpp/airdcpp/UDPServer.cpp#L130-L175

Changed in airdcpp:
status: New → Fix Released
poy (poy) wrote :

<https://github.com/airdcpp/airgit/commit/6a40613788e7ed8a7478f4f1789bbd142a98d231> makes perfect sense. a similar patch for DC++ would be very welcome. :)

nitpick: I'd change "public CommandHandler<UDPServer>" to private.

maksis (maksis) wrote :

Yeah, I took the code from AdcHub where it was public for some reason. Here's a patch for DC++.

poy (poy) wrote :

what a nice patch! thanks, applied.
I especially liked the variadic template... ;)

- has anyting changed in the NMDC parsing part? hard to catch that from the patch.

- as I'm a bit rusty... could the "x[x.length() - 1] != 0x0a" check crash on empty data? it was after a "x.compare(1, 4, "RES ") == 0" check before your patch but that clause has been removed.

Changed in dcplusplus:
status: New → Fix Committed
maksis (maksis) wrote :

Ah right, the empty string check should have been added there as well (even though empty data seems to be currently discarded at https://sourceforge.net/p/dcplusplus/code/ci/f8540557bd5c28a9d0b3879b3e6b26f50ab670a6/tree/dcpp/SearchManager.cpp#l132). The old NMDC code was moved as it was as I don't even want to understand the logic in it.....

poy (poy) wrote :

Fixed in DC++ 0.867.

Changed in dcplusplus:
status: Fix Committed → Fix Released
eMTee (realprogger) on 2017-10-21
information type: Private Security → Public
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers