search

Wulf2k / DaS-PC-MPChan

77 stars 30 forks source link

Prevent DSCM-Users from crashing others #108

Open Chronial opened 8 years ago

Chronial commented 8 years ago

There are two ways to do this:

  1. Change the name in memory
  2. When we detect an invalid name, close DSCM and display a popup that explains the situation and how to fix it.

We should implement (2) soon and change over to (1) if somebody has time and energy.

Chronial commented 8 years ago

@Jellybaby34 Do you have knowledge of the problem? Is it enough to check that the name is < 16 chars and does not contain #?

Wulf2k commented 8 years ago

Do you have any evidence that this is happening at any time other than summoning/invasions?

The post would suggest that it is, but if that were the case then I'd expect far more reports of DSCM crashing people. One person with a bad name would poison the network and nuke everybody in that level range / area.

I was actually going to tackle (1) this weekend.

Wulf2k commented 8 years ago

Minor note, I believe it's actually a "#c" that can't exist, as that should indicate a colour code. I haven't tested myself but I believe a lone "#" is fine.

Edit: This is reversed. a lone # will crash it, but at least when examining your own sign a #c is silently removed.

Chronial commented 8 years ago

I linked the wrong thread. I meant this one: http://steamcommunity.com/app/211420/discussions/0/358415103477070691/

Wulf2k commented 8 years ago

Ah, k. That one makes sense.

Yes, no surprises there but we should definitely fix that one for people.

Jellybaby34 commented 8 years ago

The best approach is shortening names to 16 characters maximum and replacing any #'s with another character e.g. "!".

Shortening the name to 16 characters fixes one of the crashes that occur when touching summon signs which was due to a buffer overflow. It also fixes an issue where if the invaders name was longer than 16 characters, rather than just copy the first 16 bytes it would just leave the copy function and the invader would be using whatever name the previous invader had.

Stripping any instance of the # in player names fixes the other type of crashing due to incorrectly formatted text tagging. The game enters a routine when it reads a # in the string its about to display and if it isn't formatted like it expects i.e. #c[FFFFFF]txthere#c it just dies.

Furthermore the name used when you touch your own summon sign is queried directly through the steam api so even if you purged any #'s in a persons name they would still crash if they touched their own sign.

TL;DR Best fix is to replace any #'s in the persons name with another character and limit it to 16 characters. Only issue then is crashing when touching your own summon sign due to it getting your steam name through the steam api

Chronial commented 8 years ago

@Wulf2k Did you test that your fix actually prevents crashing of other players in the various scenarios?

Jellybaby34 commented 8 years ago

Had a quick look and it works at removing #'s and shortening names so that should prevent people inadvertently crashing those who aren't running the appropriate fixes. Only nitpick is that it shortens names to 15 characters rather than 16 which is the maximum that can be used before it crashes others.

Wulf2k commented 8 years ago

Doesn't the 16th character have to be a 0x00, or else it will just blindly keep reading whatever's in the 17th slot and beyond?

I had nobody on to test with last night. Still, easy test and fix at some point. On Jun 18, 2016 16:05, "Jellybaby34" notifications@github.com wrote:

Had a quick look and it works at removing #'s and shortening names so that should prevent people inadvertently crashing those who aren't running the appropriate fixes. Only nitpick is that it shortens names to 15 characters rather than 16 which is the maximum that can be used before it crashes others.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Wulf2k/DaS-PC-MPChan/issues/108#issuecomment-226964799, or mute the thread https://github.com/notifications/unsubscribe/AOMBrjV4-ziCvaCpuQkhNSEmsWFnpb_tks5qNF2LgaJpZM4I2dS1 .

Wulf2k commented 8 years ago

Chronial, I tested as much as I could by checking my own summoning sign but that's a different function than reading others, so not perfect. On Jun 18, 2016 16:52, "Lane Hatland" wulf2k@gmail.com wrote:

Doesn't the 16th character have to be a 0x00, or else it will just blindly keep reading whatever's in the 17th slot and beyond?

I had nobody on to test with last night. Still, easy test and fix at some point. On Jun 18, 2016 16:05, "Jellybaby34" notifications@github.com wrote:

Had a quick look and it works at removing #'s and shortening names so that should prevent people inadvertently crashing those who aren't running the appropriate fixes. Only nitpick is that it shortens names to 15 characters rather than 16 which is the maximum that can be used before it crashes others.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Wulf2k/DaS-PC-MPChan/issues/108#issuecomment-226964799, or mute the thread https://github.com/notifications/unsubscribe/AOMBrjV4-ziCvaCpuQkhNSEmsWFnpb_tks5qNF2LgaJpZM4I2dS1 .

Jellybaby34 commented 8 years ago

You're right. Its 15 then the null byte or else it overruns the stack buffer. Sorry for the wrong information, I thought it was 16 characters.

Wulf2k commented 8 years ago

Doh, i just took your word for it and upped it to 16 last night. I'll put it back later.

Got a code for another copy of Dark Souls last night so these things should be easier to test now.

You're right. Its 15 then the null byte or else it overruns the stack buffer. Sorry for the wrong information, I thought it was 16 characters.

Chronial commented 6 years ago

@Wulf2k I had a look at what the game does, and a big portion of the locations the namecrash fix patches are places in which the name was just directly retrieved from the steam api. So it doesn't matter what's reported via the game's mechanic.

Given that information I think we should check for the player's name and if it violates the name rules, DSCM should refuse to work and display a big message instead that describes the problem and tells the user how to fix it.

If we allow bad-name users to use DSCM, we help them crash as many machines as possible, which I don't think we should do ^^.

Are you fine with my proposal or do you disagree?

Wulf2k commented 6 years ago

I'm ok with it in theory, but how many calls are there?

Would it be reasonable to just patch each call so it does an extra "jmp 0xOURCODE, mov al, 00, mov [edi+15], al, jmp 0xORIGCODE"?

Edit: Er, that fixes the local crash but not the outbound crash.

Yeah, we should be forceful.