Repository navigation
Fix gmod_wire_textscreen console command injection through copied settings - #3738
James159758 wants to merge 1 commit into
Conversation
rahul05ranjan
left a comment
There was a problem hiding this comment.
This is the right fix. The old ply:ConCommand('wire_textscreen_text "'..ent.text..'"') is the classic injection hole: a crafted text value with " and a semicolon escapes the quoted argument and executes arbitrary client console commands, and that text can come from another player's screen. Moving the copy through a net message with WriteString/SetString transports the values as data instead of through command parsing, which closes the whole class of escape rather than trying to sanitize the string. Server-initiated only, no client handler on the server side, so no new attack surface.
The main weakness is the implicit lockstep between the two lists: the server builds values as a positional array while the client reads them in copyConVars order. They match today (I checked all 12 positions), but nothing enforces it — adding a convar to one side and forgetting the other silently writes each value into the wrong convar with no error. Building the server side as a keyed table ({ text = ..., model = ... }) and having the client iterate the same key list, or at least a comment on both sides stating they must stay in the same order, would make the corruption impossible to miss.
Two smaller notes on the client receiver:
GetConVar("wire_textscreen_" .. name):SetString(...)assumes all twelve convars exist when the message arrives. They do in the normal flow (copy only happens while the tool is in use), but a nil convar here errors inside a net receive hook — a cheapif not cvar then continue endmakes the receiver total.- The numeric convars (
tsize, colors, alignment) now take whatever string arrives. Coming from the server that's fine, butSetStringwon't clamp — if an entity ever holds a bad value, the tool side reads it back as 0 viaGetInt. Not new risk, just worth knowing the net path doesn't validate where the old path (through console command parsing) incidentally did.
Nice minimal diff — no behavior change beyond the transport.
|
Supreceded by #3742 |
Right-clicking another player's gmod_wire_textscreen copies its text and font into unescaped Player:ConCommand strings. A crafted screen can inject console commands that run as the player copying its settings, including privileged ULX commands such as
ulx adduser username superadminif that player has permission to run them.Send the copied settings as data and assign the fixed text screen ConVars directly on the receiving client. This removes command parsing from the copy path while preserving the settings, including quotes and multiline text. The fix is confined to the text screen tool.