Client-side protection / Lua Injections

[Rory]

UPD: [b]Solved/Understood.[/b]

---

Trying to further understand computer programming and Lua, since I'm a student in CSCI and also like developing my own gamemodes, whether or not they'll ever actually be released. With that being said... What is the best way to prevent clients from messing with server networking, or is my understanding of this flawed?

In the current gamemode I'm writing, I use a mix of "SetNWInt" and ":NetworkVar("Int", "100", "Cash")" as explained [url=http://wiki.garrysmod.com/page/Networking_Entities]here[/url] (thanks to [url=https://facepunch.com/showthread.php?t=1548067&p=52407605&viewfull=1#post52407605]Welp's post[/url]) and [url=http://wiki.garrysmod.com/page/Entity/SetNWInt]here[/url]. How do I prevent a client from creating their own code to do "other_player:SetNWInt("cash", 0)" and wiping out the other player's cash? My current method consists of saving the player's information (cash, etc) to SetPData (sv.db) and loading it from there when they log in, but I use "ply:SetNWInt("cash", value)" for keeping the clients updated on what their cash levels are. Last but not least, I use "net.Start" and "net.Receive" for requesting permission for the clients to do something, such as buying something from the menu. Everything is done client side, but when the time comes to buy it, the message is sent to the server with the information what what the player wants to buy, and then the server determines if that's allowed. If it is allowed, the message is sent back to the client to dispatch a successful purchase message, the server spawns the item and deducts the money from SetPData and from SetNWInt("cash").

My actual gamemode doesn't use cash, but it's a great topic for an example.

What I get from Welp's post linked above, is that Networked Vars (:NetworkVar AND SetNWInt? or just NetworkVar?) have protection already, so the clients can't mess with the server. What about the rest that I mentioned? Welp has messaging turned off so making a topic about this is the best I could come up with.

TL;DR

How do you protect the server from malicious clients? What are some examples of Lua injections (don't give us actual code, keep the mingebags at bay) that you consider to protect for when writing client/server interactions?

[bigdogmat]

Just always assume the client can run whatever client side code they want. If you're receiving a net message server side, and you know said net message is only going to be received under certain conditions, check those conditions.

Shitty example but say the server wants the client to enter information that will be sent back to the server and be used to spawn an entity. You'll want to a check in the receiving net message to make sure the server ever sent out that request, because if it didn't and you're not checking if it did, the client could potentially spawn that entity any time it wanted to.

[editline]30th June 2017[/editline]

By the looks of it you've already answered your own question about the SetNW* and NetworkVar functions. If the client ever changed those they'd only be changed on said client, and eventually will be changed back to their real values.

[Rory]

[QUOTE=bigdogmat;52418318]
By the looks of it you've already answered your own question[/QUOTE]

That's why I wanted to post this. I don't feel like my question on this subject is quite answered, meaning I don't understand what's going on lol. Thanks for your reply, checking conditions is probably the way to go then for net.Send, but :NetworkVar is already protected it would seem. I just don't understand what stops a client for doing "ply:SetNWInt("cash", 0)" or doing "ply:SetCash(0)" and fucking up someone else's bank account.

[bigdogmat]

[QUOTE=Rory;52418331]I just don't understand what stops a client for doing "ply:SetNWInt("cash", 0)" or doing "ply:SetCash(0)" and fucking up someone else's bank account.[/QUOTE]

Because as I said in the first post, if a client does that it'll only be changed for them. So long as you don't have a backdoor or something installed on your server, the client can only run client side code. The SetNW* functions value is only changed for everyone if done server side.

[Rory]

[QUOTE=bigdogmat;52418340]...the client can only run client side code. The SetNW* functions value is only changed for everyone if done server side.[/QUOTE]

Missed that last part reading through your reply. Thank you, that solves that issue.

Just confirming as well that when people are talking about Lua injections, that's what you were recommending with the condition checks when accepting information via net messages? Is that where the vulnerability comes from Lua injections?

Thanks for the clarification on all of this.

[bigdogmat]

[QUOTE=Rory;52418375]
Just confirming as well that when people are talking about Lua injections, that's what you were recommending with the condition checks when accepting information via net messages? Is that where the vulnerability comes from Lua injections?
[/QUOTE]

Assuming Lua injections means running client side Lua at will, yes. Because the client has the ability to run client side Lua, they could send that net message at any time they wanted to. The server side check in the receiving net message prevents it from actually doing anything.

[TheEMP]

[QUOTE=Rory;52418375]Just confirming as well that when people are talking about Lua injections, that's what you were recommending with the condition checks when accepting information via net messages? Is that where the vulnerability comes from Lua injections?

Thanks for the clarification on all of this.[/QUOTE]
As a general rule never trust the client. Always perform validation checks on information passed from the client to the server. Just because you don't think you coded them a way to do what ever it is your checking from. Clients can always inject code (hack) and execute your netmessage in ways you never intended. Thats why if you don't code them right you can leave your self with huge vulnerabilities depeniding on what that data is used for.

[Rory]

[lua]
net.Receive("CreationSubmit", function(len, ply)

-- Receive Client's Information
local team_choice = net.ReadInt(8)
local plymodel = net.ReadString()

-- Set up consistencies
local valid_teams = {1, 2}
local valid_models = {
"models/humans/group03/male_01.mdl",
"models/humans/group03/male_02.mdl",
"models/humans/group03/male_03.mdl",
"models/humans/group03/male_04.mdl",
"models/humans/group03/male_05.mdl",
"models/humans/group03/male_06.mdl",
"models/humans/group03/male_07.mdl",
"models/humans/group03/male_08.mdl",
"models/humans/group03/male_09.mdl",
"models/player/combine__super_soldier.mdl",
"models/player/combine_soldier.mdl"
}

-- Check for consistency, that is, that the information the client sent us is valid
-- If the table does not have the client's request in it, set a default.
if !(table.HasValue(valid_teams, team_choice)) then
team_choice = math.random(1,2)
end

if !(table.HasValue(valid_models, plymodel)) then
if team_choice == 1 then
plymodel = "models/humans/group03/male_01.mdl"
else
plymodel = "models/player/combine_soldier.mdl"
end
end

ply:SetTeam(team_choice)

timer.Simple(0.25, function()
ply:Spawn()
end)

timer.Simple(1, function() SaveStats(ply) end)

end)
[/lua]

So this function, using net.Receive, would be protected from Lua injection because I did a consistency check?

[JasonMan]

Yes, but your table is built poorly, see [URL="http://wiki.garrysmod.com/page/Tables:_Bad_Habits"]Tables: Bad Habits[/URL]

[Rory]

[QUOTE=JasonMan34;52418479]Yes, but your table is built poorly[/QUOTE]

[lua]net.Receive("CreationSubmit", function(len, ply)

-- Receive Client's Information
local team_choice = net.ReadInt(8)
local plymodel = net.ReadString()

-- Set up consistencies
local valid_teams = {
1=true,
2=true
}
local valid_models = {
"models/humans/group03/male_01.mdl"=true,
"models/humans/group03/male_02.mdl"=true,
"models/humans/group03/male_03.mdl"=true,
"models/humans/group03/male_04.mdl"=true,
"models/humans/group03/male_05.mdl"=true,
"models/humans/group03/male_06.mdl"=true,
"models/humans/group03/male_07.mdl"=true,
"models/humans/group03/male_08.mdl"=true,
"models/humans/group03/male_09.mdl"=true,
"models/humans/group03/female_01.mdl"=true,
"models/humans/group03/female_02.mdl"=true,
"models/humans/group03/female_03.mdl"=true,
"models/humans/group03/female_04.mdl"=true,
"models/humans/group03/female_06.mdl"=true,
"models/humans/group03/female_07.mdl"=true,
"models/player/combine_soldier.mdl"=true,
"models/player/combine_super_soldier.mdl"=true
}

-- Check for consistency, that is, that the information the client sent us is valid
-- If the table does not have the client's request in it, set a default.
if !valid_teams[team_choice] then
team_choice = math.random(1,2)
end

if !valid_models[plymodel] then
if team_choice == 1 then
plymodel = "models/humans/group03/male_01.mdl"
else
plymodel = "models/player/combine_soldier.mdl"
end
end

ply:SetTeam(team_choice)

timer.Simple(0.25, function()
ply:Spawn()
end)
end)
[/lua]

Thanks for the help, hopefully that is more efficient.

[pervert]

One more thing.

"models/humans/group03/male_01.mdl"=true,

should be:

["models/humans/group03/male_01.mdl"] = true

[bigdogmat]

The table creation should be moved outside the function.

[Rory]

[QUOTE=bigdogmat;52419000]The table creation should be moved outside the function.[/QUOTE]

I put the tables inside of the function so that when the function is done running, the local variables are removed and thus not taking up any memory sitting in stasis waiting to be used for the next call.

[Kevlon]

[QUOTE=Rory;52419063]I put the tables inside of the function so that when the function is done running, the local variables are removed and thus not taking up any memory sitting in stasis waiting to be used for the next call.[/QUOTE]

Unless this function is called every frame these micro optimizations will do close to nothing, It doesn't really matter.

[PigeonTroll]

[QUOTE=JasonMan34;52418479]Yes, but your table is built poorly, see [URL="http://wiki.garrysmod.com/page/Tables:_Bad_Habits"]Tables: Bad Habits[/URL][/QUOTE]

But is it alright to use the table like he originally if you're not planning to do a table.HasValue check / loop?