amxmodx
amxmodx copied to clipboard
alliedmodders
Filter evil characters in name and message (bug 5912, bug 6301)
This fixes a known exploit around localized string and var arg, like #Spec_Help_Text, +forward or %s , which can cause bugs with multiple occurrences, at worst crashing a client.
While it should not crash on the client (typically an overflow with localized strings), unfortunately, this is happening because AMXX sends a message as it is directly and people are enough cunning to use such string in their name as well.
The concerned messages are: TextMsg, SayText and ShowMenu.
This means that any plugin which displays a menu with player's name (like slap menu) or rewrites the chat output (like adminchat with say_team @), at best you get an unwanted translated message, at worst it crashes the client who receives the message.
Ideally, the name should be filtered in the engine, and anything typed in say/_team in the mod, but since AMXX is equally responsible and goldsrc is not a priority for valve, it makes sense we do this filtering.
This patch filters the following characters in those specific events:
- When at player changes name
- By filtering name, this should cover most of the situations since name is often inserted in message
- When a player says something via
sayandsay_team- By filtering when a command is issued, it should cover
read_argswhich is used by the plugin to retrieve the whole input buffer from chat before being passed in natives usingTextMsg,SayTextsuch asclient_print/_coloror manually viawrite_string.
- By filtering when a command is issued, it should cover
| Character | Description | Message |
|---|---|---|
+ <key> |
Key binds. Translated to [key]. |
ShowMenu |
& <value> |
Hotkeys in VGUI. Unsure. | ShowMenu ? |
# <value> |
Localized string | ShowMenu, SayText, TextMsg |
% s |
Format specifier | SayText, ``TextMsg` |
Some notes ^:
- I'm not totally sure about
&. It seems engine already filters it by replacing by spaces, but for safety I keep it %should be filtered by the mod if you opt for the beta, but for safety I keep it.- Only
#and%are concerned in messages
I would like some feedbacks.
- Characters are replaced with similar Unicode (code written by @WPMGPRoSToTeMa ). What do you think about it? Is a config should be added allow admin to choose whether they want to replace evil characters with a specific one instead of Unicode?
- Any usage I could have forgotten?
- Is a native to filter such characters would be useful?
What do you mean? I know it has been around since some time, and you have some metamod module (like localizebugfix or SafeChatAndMessage) for that, even reHLDS partially fixed, but it should be anyway available by default in AMXX. Better late than never, right?
I would prefer more having helpful answers really.
I'm not sure if I understand correctly. Will the character + be replaced unconditionally every time you show a menu?
Not the +, but the next character by an unicode character. http://unicode-table.com/blocks/halfwidth-and-fullwidth-forms/
Close enough. On HUD/Menus I'm sure it will look flawless (since it's font much better with unicode). A cvar to disable this feature entirely would be nice.
Why would you want to disable a protection? That's the purpose of this PR, giving feedback, so please elaborate, maybe it makes sense I don't know.
Because this protection isn't seamless (it replaces user names, and strings in a way that is invisible (but different) to plugins) and you may want to handle this at a plugin level or whatever. Options are always a good idea when you have new, different behavior
with this feature, do we still need client_printex() stock ? https://www.amxmodx.org/api/message_stocks/client_printex
@luxxxoor We have no natives to send predefined text with arguments like does the game. You can do that manually via message_begin and this is what the stock is for.
@IgnacioFDM
Well, the whole point of using Unicode is because the user is more important than coder here. Change is done before everything else, it should not make much difference, the plugin still receives an original input, right?
Actually, name is already partially filtered by the engine before pfnClientUserInfoChanged is called. So for the name it's not going to be a surprise.
But I think you're saying that plugin should get an unmodified input and filtering should happen only in case message is going to be sent after plugin did their stuff on it. The purpose would be then to keep current engine/mod/amxx behavior but filtering at the very end for displaying. For a fix inside AMXX, this would be actually more welcomed I guess. Is it what you mean?
About filtering a plugin, it's probably a bad idea. You can't filter properly and fix an exploit should be anyway in core. About the option, It could make sense but not for the reason you said. The only valid I see is if you're using reHLDS/GameDLL and because they have a similar protection, that would be redundant.
Filtering happening after sending the message (so plugins get raw input) is a much better way to do it. Imagine you have a plugin where you have (similar to admin say) hook"say" clcmd, and you print text in a special way when the message begins with + (or #) (similarly to how @ is treated for admin say). Now not only you break compatibility with older plugins without support, but you also require plugins to detect some silly longer than 1 byte unicode character instead of simply text[pos] == '+', when users are simply pressing the + button (this is really unintuitive to some developer who hasn't read this PR).
Besides, the cvar for future regamedll/hlds changes as you said is very important. Furthermore, implementing a cvar is simply less effort and would have taken less time than this discussion :laughing:
In general, I've been frustrated many times (in other games) for having "protections" (such as anti speed hack, anti DoS or whatever) that end up backfiring, so having an option to toggle is always a good idea unless we are talking of something 100% innocuous such as maybe fixing some buffer overflow or whatever.
By the way, is the adminsay.sma (or whatever it's called) exploit fixed, where you could say_team @%s0 hehe resulting in admin's game crashing? There are many exploits possible (and working on approved plugins) not related to client side parsing of %s (and similar % keywords or whatever they are called), but actually related to parsing them with formatex
I agree to filter say/_teamafter plugins are done is a more backward compatible solution, so I guess I will commit later. But the change in name is probably fine as it is (because engine already filters things, and because it will be tricky to filter ShowMenu).
About cvar, if the patch doesn't mess with the original input, you have no reason to have it disabled, but because of reHLDS/GameDLL and maybe other tools/contexts, it's okay to add one I guess.
About %s, it's filtered in the client if you opt for the beta If I remember. It will replace s by space.
But I'm not aware of any others exploits with %, so if you know something, it's time to say it.
Thanks for your feedback.
Did you try
say_team @%s
and
say_team @%s0
(admins' client should crash)
I'm not home and can't test now, but that's a confirmed exploit on amx <= 1.8.2
] say_team @%s
(ADMIN) Arkshine : s
] say_team @%s0
(ADMIN) Arkshine : s0
When I said beta, I mean the client CS. Right now, I have opted for the beta and it doesn't crash. I remember the message from Alfred saying %s is filtered. It will crash for sure if you don't opt for the beta.
Yes, you're right about client_t, but probability struct is updated again is near zero. Could be fixed through a gamedata file. It's anyway not a good example since it can be done differently.
I know about svencoop, but they do what they can to not break compatibility.Though they are trying to get rid of AMXX with their AngelScript, so sooner or later, things might be complicated. Considering the low number of servers under AMXX and because I'm kind of alone, dealing with svencoop specificities is not really a priority. But this should be fine for some time, this is not in their interest.
About your last question, functionalities relying on gamedata are optional. Only associated stuff are concerned. (Btw, AMXX is old, you can't expect everything being well organized in a way it makes sense. Cvar stuffs are originally made in core, so hooking cvars have to be there as well. AMXX code source is horrible in many ways).
Some feedback after having used something similar to this over a year:
I've been using the UTIL_FilterEvilChars reimplemented in pawn with great success (I needed having access to the "raw data" and a bit more fine-grained control than this PR). To merge this PR I'd say we need to
- Check interactions with ReHLDS since it does this with names by default
- Native/stock is definitely needed, so developers can filter their stuff (think menus, data coming externally (databases, userinfo, etc) and so on)
- I really don't like the "feeding fake data to read_args". I get the PHP Magic Quotes vibe from it. Maybe a better one size fits all approach would be filtering client_print? I don't know. I haven't really thought much about other solutions that can "backport" this fix to older plugins. Not too fussed about this as long as there is a cvar to disable it, since it can unleash chaos in some scenarios (it did to me, think duplicate SQL entries for names or other user input with the both the older normal characters and the weird new ones, or admin commands needing correct characters to work such as checking for hardcoded strings like "#TTs")
- As mentioned above, cvars to disable this
That's my personal opinion of course.
~Chat doesn't need fixes since last update.~ I was wrong, Valve fixed only % in chat.
Totally forgot about this. I'm kind of lost about this.
@WPMGPRoSToTeMa, do you know exactly what it still needs to be filtered in HLDS and what to skip with ReHLDS?
@Arkshine nothing with ReHLDS and only nicknames with HLDS. &, % are replaced by default in HLDS, # is replaced too, but only at the beginning of name. We can also add gamedata files for + commands and localizable strings to avoid unnecessary replacements and save compatibility with safe nicknames. And also we can add functions that can print % and # symbols in chat messages (#Spec_PlayerItem that is used in my ChatPrint can help).
@WPMGPRoSToTeMa Looks like ReHLDS doesn't do that. I'm not sure if it's a good idea that both don't behave the same. Any thoughts on it? Is it really worth? Actually, what are the compatibility issues, I see tag checking I guess, are there others?
About localized strings, if I remember, only the keys in /resources/$mod_english.txt are concerned, right?
Looks like ReHLDS doesn't do that.
I didn't think about that when I commited in ReHLDS. There is still an open issue https://github.com/dreamstalker/rehlds/issues/198, so I will resolve it in ReHLDS in future. Some people also said that these symbols aren't displayed correctly for them (empty squares).
About localized strings, if I remember, only the keys in /resources/$mod_english.txt are concerned, right?
In ShowMenu only titles.txt is considered. This file is present on the server-side, so looks like we can easily use it without any additional gamedata file.
Hm, config.cfg is also present on the server-side. We can retrieve a + commands list from it. But looks like it doesn't contain a full list. Manual collecting of + commands isn't so hard, we can just use cmdlist log command on the client and leave strings starting with + from the cmdlist.txt file. Basically if the gamedata file isn't present we can use config.cfg, then if it is also not present we can disable this option or behave as ReHLDS. (same for localized strings)
We can also filter # and + at the show_menu/menu_display side. But there are some problems:
- What if someone needs a localized string or a + feature in his menu text?
＃Buyand#Buynicknames will look fully the same.
The second problem gives me a new idea for another PR: enhanced detection of duplicate names, so #abcdef123 == ＃ＡＢＣＤＥＦ１２３ == #abc̲d̲e̲f123.
Also instead of removing "invisible" symbols we can try to replace them with visible alternatives. (I'm talking about ReHLDS and SNAC invisible symbols removing)
