Skip to content

Fix gmod_wire_textscreen console command injection through copied settings - #3738

Closed
James159758 wants to merge 1 commit into
wiremod:masterfrom
James159758:fix/textscreen-command-injection
Closed

James159758 wants to merge 1 commit into
wiremod:masterfrom
James159758:fix/textscreen-command-injection

Conversation

@James159758

Copy link
Copy Markdown

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 superadmin if 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.

@rahul05ranjan rahul05ranjan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 cheap if not cvar then continue end makes the receiver total.
  • The numeric convars (tsize, colors, alignment) now take whatever string arrives. Coming from the server that's fine, but SetString won't clamp — if an entity ever holds a bad value, the tool side reads it back as 0 via GetInt. 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.

@Astralcircle

Copy link
Copy Markdown
Member

Supreceded by #3742

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants