Normal users can issue CMDs

[Fredrik Ullner]

Any client may send a CMD (only B-type tested) to the hub, distributing it to any user. If done in a bot, you can effectively send tens or hundreds of these, and a receiving client will be forced to manage them, thus potentially causing a DoS scenario.

Generate the following user command in DC++ to test yourself;
Command type: Raw
Context: Hub menu
Name: RogueCommand
Command: BCMD %[mySID] Security\stest,\sbe\safraid TTHINF\sNIfoobar\n CT2
Hub address: adc://

(Above command should obviously be followed by a new line.)

The hub should ignore any CMD originating from a user. Potentially allow CMDs from trusted users.

[iceman50 (bdcdevel)]

Added DC++ to the report since DC++ doesn't overwrite the command but keeps adding it

[Pirre (pierreparys)]

temp fix commited a final fix will be based on userslevel/flag=bot or special permision

[poy (poy)]

Fixed in ADCH++ 2.11.

[Fredrik Ullner (ullner)]

The following patch does two things to address the issue here.
* Replace user commands (i.e., if the command 'test' is sent twice, the latter overwrites the previous command value)
* Restricts to a maximum of 100 (external) user commands. (Completely arbitrary number.)

While I did settings, I did not make them available in the UI. I wasn't sure whether the users should really be able to change them...

Additionally, the patch makes sure that the all external user commands are sorted after all internal (created by the user) user commands. This makes sure that it is easy to spot user/hub user commands.

[poy (poy)]

1) 100 is far too low: my test hub, for example, sends me 1176 user commands at login. in particular, a popular translation script sends 702 user commands; another popular script (FunScript) sends 251. i am willing to bet other scripts like Dixbot or Leviathan also send hundreds of user commands.
a better max value would be one beyond which opening a menu takes too long on a regular computer.

2) the current rule for settings in DC++ is to add them "fully" - with a UI, help, etc. for the max value, a hard-coded one would be fine by me if it were high enough. the replacing behavior should not be a setting but be standard (user commands are identified by their name).

3) the new loop that is executed on each user command addition has to be tested with hubs that send thousands of user commands. storing them in some sort of ordered map might help with performance....

4) i would remove the log message when the limit has been reached. note also that said log message could result in quite the spam in its current state. again, this depends on the value decided in #1; if it is high enough, the log message is futile.

5) in the following:
if(!lst.empty() && !lstExternal.empty())
shouldn't the AND be an OR?

6) doesn't UserCommand::FLAG_NOSAVE already accomplish the job of the new UserCommand::external? if it doesn't, add a new flag instead of a new bool.

[Fredrik Ullner (ullner)]

1) Set the limit to 7000 in this patch. I tested with 8000 and above, and DC++ hung when trying to open the view. But that is the same before and after a patch (the patch doesn't deal with displayment anyway). It took no noticable time difference adding 1k, 5k or 10k user commands. Perhaps we should set the limit to e.g. 5k to have a buffer zone?

2) Removed settings and use hard coded behaviour. (Always replace user commands.)

3) I could not test any difference in performance (didn't feel like it anyway...)

4) Removed.

5) No, it's correct, see the patch for a more descriptive check.

6) I wasn't too sure about it, but after some more investigation, all I can find is that FLAG_NOSAVE is only external. Though, I suppose if someone in the future adds behaviour for the ability (for the user) to create temporary user commands , then it not work as intended... Anyway, using FLAG_NOSAVE now.

To test the patch, add the following to the HubFrame.cpp (for instance): http://pastebin.com/H7GMtavt

[poy (poy)]

in rev 669febd69662, with the following tweaks:
- added a missing "if (isexternal)" (just a mistake i assume).
- set the max to 5000.
- added a call to std::move.

i couldn't notice any perf degradation on my test hub.

[poy (poy)]

Fixed in DC++ 0.840.