Add a blacklist to block cheaters

[metal-crow]

Following Hillomunkki's work, I've implemented a blocking system in DSCM. Users can block up to 5 other players, and blocks are stored in the registry. Implementation wise, blocking is done via the same method as Europa's https://github.com/eur0pa/DSNA: Injecting code into the SendP2PPacket and ReadP2PPacket steamAPI functions.

This resolves issue #122

[Chronial]

Can you disable auto-format in your visual studio and undo all the changes that are only format changes? That would make the PR a bit easier to read.

It looks like this does not prevent DSCM from creating connections to blocked players or did I just miss that?

[metal-crow]

Ah, my bad! Fixed up and squashed into a single commit. Also, the connection prevention timer is autoDisconnectBlockList_Tick, line 298 in MainWindow.vb

[Chronial]

Thx for undoing the formatting

autoDisconnectBlockList_Tick only disconnects already connected nodes. But DSCM might still trigger connections to these nodes, which would result in a quick connect / disconnect cycle, which could be avoided.

[Chronial]

This PR breaks the UI. The UI is broken both in it's default size (the buttons at the bottom don't fit) and when not expanded (click the "Expand DSCM") checkbox).

[metal-crow]

Found a bug where blocks aren't removed from the registry when you press the remove block button

[metal-crow]

This PR breaks the UI. The UI is broken both in it's default size (the buttons at the bottom don't fit) and when not expanded (click the "Expand DSCM") checkbox).

Any suggestions as to fixing this? I can't make the buttons smaller without cutting off text, and i can't move the buttons/items to the left or right without overlapping other elements.

[Chronial]

Sry, I actually don't have a lot of time to look at this at the moment. I will try and have another look, when I'm less swamped for work. Or maybe wulf will find some free time first.

[metal-crow]

Sry, I actually don't have a lot of time to look at this at the moment. I will try and have another look, when I'm less swamped for work. Or maybe wulf will find some free time first.

Not a problem, no rush!

[Chronial]

Apparently this already exists: https://github.com/eur0pa/DSNA

[metal-crow]

Yeah, just saw myself. I had no idea about that when I made this, whoops 😌! So, do we want to have this feature built into dscm, or keep it separate? I personally think the former is better, since it's one less program to run, plus it fits in with dscm's purpose of network management anyway.

[Chronial]

I agree that having this in DSCM would be nice, but I do prefer euro0pa's implementation for the blocking (=actually blocking connections to blacklisted nodes). Your implementation can actually be quite easily circumvented by a cheater.

[metal-crow]

Easily circumvented? How so? The only way i can see is if they connect and invade in the 2 seconds between disconnections. But i'll take a look at euro0pa's implementation and try and port it over.

[metal-crow]

Ah wait, I see what you mean. The Disconnect function only sends a request to the other player to disconnect, which they can ignore.

[metal-crow]

Also, it appears the PatchCode method for JmpHook is broken, it appends the overwritten instructions to the new code instead of prepending it. Ill include a patch for this here as well i guess, since i'm fixing it

[metal-crow]

(still WIP, just pushing)

[Chronial]

The fact that PatchCode appends the original instructions is not a bug, but is completely intentional. This has the benefit of having more predictable code addresses then the other way around. Why would this be a bug?

[metal-crow]

Oh, sorry, my bad for assuming. I believed it's a bug because it breaks my current code and any other assembly with multiple returns. By appending the original instructions, if you have a second code section that jumps back to the original location, or just returns from the function (like mine), then those original instructions are not executed for that case.

[metal-crow]

However, since this is desired behaviour, I'll change my fix to make prepending it require an optional argument (if that's ok)

[Chronial]

I don't quite understand why the original behavior is a problem for your case. If you ret, you don't want the original instructions to run – you want to abort the function. Otherwise you just run to the end of your code, which you have to do anyways, since that's where the jump back is located.

[metal-crow]

Well, for my case i'd have to refactor my code to use esp instead of ebp, but the more important consideration is if the overwritten instructions write to memory or have some side effects.

[Chronial]

The idea is that you hook into the location of the instruction before which you want to run. If you want an instruction to run before your code, just hook later. Running the code you replaced before or after the injected code has exactly the same results – the only thing that changes is which hook-location you have to specify.

The reason why it makes more sense to run the replaced code at the end is that this allows you to hook into the beginning of a function which would otherwise be impossible. Since placing code after a ret (=end of a function) doesn't make sense, this issue does not exist the other way round.

And isn't that actually exactly what you want to do here, but since you can't hook into the begging of the function with your method, you have to undo what the beginning of the function does with these lines

+83 C4 04              - add esp,04 //drop the push [ebp+1C]
+5D                    - pop ebp

which you could just remove if you used the original hook method. But it's also late here and I might just be completely missing what's going on :)

But is there a reason why can't just move the hook location by 6 bytes, use the original hooking method and change nothing else?

Btw, in case you care: you can simplify the comparison loop like this:

looptop:
  cmp eax,[ecx+ebx]
  jne loopcontinue
  cmp edx,[ecx+ebx+04]
  je abortsend
loopcontinue:
  add ebx,08
  cmp ebx,size of block list memory allocated
  jl looptop
  jmp normalsend

[metal-crow]

Oh huh, yeah that makes a lot of sense. I've just always used Cheat Engine, which does it the prepend way, so i got used to that. Thank you for explaining this!

I don't think i can use the append method without modifying my code to use esp instead of ebp, but i'd be happy to fix that. But i see now that both are valid approaches, i'm just used to doing it this way :)

Also GAH, i knew i was overcomplicating that injection, thanks for the tip. I'll fix that up.

[metal-crow]

Right, there we go; all fixed up, improved to be non-bypassable, working, and squashed! Should be good to go!

[Chronial]

Could you make sure DSCM never selects a blocked node as a good candidate, please? For this you need to add the blocked nodes to the blackSet in the beginning of selectNetNodeForConnecting.

[metal-crow]

Added!

[Chronial]

thx

[Wulf2k]

Chronial, MC, do you both agree that the code's ready to merge?

[metal-crow]

Btw, i've received reports that the alpha release i shared with Reddit is already being bypassed (it used an insecure blocking method this one isn't susceptible too). So getting this merged and released with mainline DSCM reasonable soon would be appreciated, before that bypass spreads.

[metal-crow]

Also, I just realized by MC you meant me, my bad, i didn't even parse that as my name. Sorry! Yeah i think it's ready to merge.

[Chronial]

The UI still has various bugs:

imho it also feels more messy and overloaded now. I can't see any more bugs in the code at a quick glance, but there's some minor things I don't really like, like that fact that the DSProcess accesses the main Window now.

But I also don't have any time to look into any of this at the moment and I don't think it breaks DSCM.

But wulf, if you find time to do something: no pressure 🙂, but it might be more important to get some version of wulf2k.ca up (even if it's just temporary). If you need hosting, I think I can also provide something for you. It's not clear where to get DSCM from at the moment, most links link to your hp, so they are broken / people started linking to their own uploaded versions. The current version is not officially available for download anymore at all and the auto-updater doesn't work either.

[metal-crow]

With regards to the DSProcess accessing the main Window, if you think it'd be cleaner i can have Sync_MemoryBlockList take as an arg the array of blocked players, instead of having it read the main window's array of blocked players. That's a quick fix. Any other code design choices you don't like?

As for the gui bugs, the Add/Remove block buttons appearing under the Launch Dark Souls button is annoying, but i can't make them smaller without cutting off text, so the only alternative would be making some soft of dropdown or context menu for them. I can do that if i figure out how VB does gui stuff, but i don't think its worth blocking the pr right now.

Also bit confused about the second gui bug you show a screenshot for. Is it that the button and text field is hitting the window edge? Mine doesn't appear like that.

[metal-crow]

Oh wait, the gui bug does appear when Expand DSCM isn't checked, i see. I can see about fixing that

[metal-crow]

@Chronial Fixed the gui bug and DSProcess doesn't access MainWindow anymore.

[metal-crow]

since this version of DSCM isn't getting updated again according to the newest commit, i'll just close this PR. My fork with these changes will be available + kept updated.