Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setPedArmor and setElementHealth synchronization problems from Client to Server #3791

Open
1 task done
bastianmarin opened this issue Oct 12, 2024 · 15 comments
Open
1 task done
Labels
bug Something isn't working

Comments

@bastianmarin
Copy link

Describe the bug

Creating anti-cheat checks for a server, we realized something curious.

Although the MTA wiki indicates that the vest that can be placed on a player is from 0-100, this is not real, you can set the number you want (example 1000) from the clientside, from the serverside this is not possible.

Testing this problem, I reached several conclusions.

  • The clientside part is currently vulnerable to this problem, you can set setPedArmor values ​​outside the range indicated by the wiki. Therefore, a cheater can use this function to set values ​​higher than those allowed.
  • The Server and the Clientside will be out of sync. In the sense that if on the Clientside my armor is 1000 on the server it will be reflected as 127.5 with the getPedArmor function. This also happens with the setElementHealth but it reaches a maximum of 255.
  • Armor and life are actually usable, when a person is hurt, the damage of the armor placed on the client side will be reduced. If you shoot him with 1000 armor from a distance, it will leave him with 9990.

Note:

  • MTA does a check only on life every 30-60 seconds and this returns to the real maximum. This does not happen with armor
  • It is not possible to do this in Health, however when a cheater uses the menu he can edit the Health.

Steps to reproduce

  1. In clientside use setPedArmor

Version

Client: Multi Theft Auto v1.6-release-22763

Additional context

Image

Relevant log output

No response

Security Policy

  • I have read and understood the Security Policy and this issue is not security related.
@bastianmarin bastianmarin added the bug Something isn't working label Oct 12, 2024
@PlatinMTA
Copy link
Contributor

Health seems to be capped but yeah you can set any amount you want for armor clientside. So I can confirm this for the Armor.

To be honest I think that via script you should be able to set any value for health and armor, it shouldn't break anything. We already have this for vehicles, why not peds?

@TracerDS
Copy link
Contributor

Health seems to be capped but yeah you can set any amount you want for armor clientside. So I can confirm this for the Armor.

To be honest I think that via script you should be able to set any value for health and armor, it shouldn't break anything. We already have this for vehicles, why not peds?

Tbh it should be fixed for all elements. You shouldnt have more max health just because.

@Fernando-A-Rocha
Copy link
Contributor

Yeh max health exists for a reason, it shouldn't be allowed to exceed it.

@PlatinMTA
Copy link
Contributor

Ok so, what's the actual reason? It works without issue with vehicles.

@bastianmarin
Copy link
Author

bastianmarin commented Oct 13, 2024

My real problem is with server security, meaning preventing cheaters from adding more life than possible. It's 100 times safer to verify on serverside that the player has +100 armor* than on client and prevent it from being intercepted and cancelled.

@PlatinMTA
Copy link
Contributor

PlatinMTA commented Oct 13, 2024

My real problem is with server security, meaning preventing cheaters from adding more life than possible. It's 100 times safer to verify on serverside that the player has +100 armor* than on client and prevent it from being intercepted and cancelled.

addEventHandler("onClientPlayerDamage", localPlayer, function()
    cancelEvent()
end)

You should really be doing serversided checks for this stuff by now (never trust the client, even if it has capped values). Also, what I am asking is to be able to set any health serverside, not even just clientside. Right now this is a bug, but it could be a feature and it would make it easier for scripters so they don't need to make their own health system when there is already one. Then again is up to the rest of us to decide if its worth it or not.

@bastianmarin
Copy link
Author

Mi verdadero problema es la seguridad del servidor, es decir, evitar que los tramposos agreguen más vida de la posible. Es 100 veces más seguro verificar en el servidor que el jugador tiene +100 de armadura* que en el cliente y evitar que sea interceptado y cancelado.

addEventHandler("onClientPlayerDamage", localPlayer, function()
cancelEvent()
end)

A estas alturas ya deberías estar haciendo comprobaciones en el servidor para estas cosas. Además, lo que estoy pidiendo es poder configurar cualquier lado del servidor en estado de salud, ni siquiera _ solo _ el lado del cliente. En este momento esto es un error, pero podría ser una característica y facilitaría que los programadores no tengan que crear su propio sistema de salud cuando ya existe uno. Por otra parte, depende del resto de nosotros decidir si vale la pena o no.

It seems that he really doesn't know how to read what I wrote.
The problem is the desynchronization between the serverside and clientside values.
Of course I want to check the value on Serverside but this is not possible because the values ​​do not match.
If you want to make an improvement or a change to the health of the elements in MTA. Open a pull request and follow your idea.
This post is to fix a desynchronization problem between items.
He also seems to not know how the game works internally. Changing this may bring problems for the game's performance and the current AC of MTA.
The game is not practically designed to exceed these values ​​and probably has a problem with it.
MTA will automatically correct the health if it is exceeded to unrealistic values ​​in memory.

@PlatinMTA
Copy link
Contributor

PlatinMTA commented Oct 14, 2024

Kinda rude but sure, since we are talking about capping values I think it's related. I will not take into account the unnecessary comments you made and I will just ask you what's the request? To cap the values clientside? If that's the case, then of course I would argue that maybe we should consider uncapping those values or making the cap higher. Why? So we don't have to rely on custom resources to handle another health system.


Anyways the issue here is related to puresync most likely. What I think its happening for the server to correct their own values is that when the puresync packet arrives it already arrives with the values shorten down due to its maximum size, thats why the values are capped at 127.5 and 2047.5. So that's were the fix should be put in place, plus a cap in setArmor clientside (which is non-existent right now). Since we are already doing that we might as well add a cap for the other elemens in setElementHealth too.


Now my argument,

runcode
run local veh = getPedOccupiedVehicle(me) setElementHealth(veh, 9995) iprint("new:", getElementHealth(veh)) setTimer(function() iprint(getTickCount(), getElementHealth(veh)) end, 0, 10)
This will return for a tiny bit the set up value (9995) and then it will go back to 2047.5, but only serverside. All clients (or at least the syncer, since i tried this only in a local server) will still have the health value at 9995.

So, if the server can set values higher than 2047.5 and it can also sync those values without much issue, why should we cap them low?

Changing this may bring problems for the game's performance and the current AC of MTA.

Could be, but why? The cap is not being done internally (there is a clamp in setPedArmor serverside but not clientside, but there is no clamp with setElementHealth in either client nor server except for ped/players). This is a side effect of puresync. If that's the case (game performing poorly) of course we don't want it, but if it isn't this could be a nice opportunity to fix this issue + raise the current limits of MTA.

Or we can make our own implementation with lua and deal with it like we have been doing for years at this point...

@TracerDS
Copy link
Contributor

Cancelling event on the client is dangerous as the cheater can just not execute that code

@derxgbb
Copy link

derxgbb commented Oct 15, 2024

I'd like to point out something here because it has some connections with this issue. (kind of)

A few years ago I was testing someting with clientside setElementData and things of like that and I also noticed this desynchronization thing. I wanted to make a custom hp+armor script and firstly I wanted to make it with setElementData (I dropped the whole idea after some days)
I just mentioned this that on clientside there will always be some desynchronization.

What I want to ask is, why the maximum hp is 200 and not 150?

In the singleplayer you can have a maximum of 150 hp. To get them you either need to do the paramedic side mission, or you'd need to exercise CJ's body to have more stamina+muslces to earn that slowly.

This is a problem because the game HUD is designed for 150 hp and I'm surprised that no one caught this yet in these years.
I'm not sure if I can desrcibe it well, so I encourage everyone to set up a local server then try the following:

setPedStat(ped, 24, 1000)

This makes your charachter to have MAX_HEALTH of 200.
After this set your current HP to 100.

setElementHealth(ped, 100)

Watch the HPBAR on the HUD. It'll look like its in the middle (almost) as it should.
Now set it to 150, then check the HPBAR again.
It'll look like its almost full, meanwhile there is still 50 hp left till 200. As i tested the HPBAR on the HUD visually maximises at 172 hp and all HP after that won't be seen.
This can mislead players.

The other problem which also applies to this is the setPedStat itself which mislead the scripters.

setPedStat(ped, 24, 1000) -- maxhp: 200  (so if 1000points means 200maxhp)
setPedStat(ped, 24, 500) -- maxhp: 84 (why 500points is not 100maxhp?)
setPedStat(ped, 24, 565) -- maxhp: 100 (in fact 565points is 100maxhp...) -- getPedStat returns 569 by default

The best would be:

setPedStat(ped, 24, 100) -- maxhp: 100 -- making this as default
setPedStat(ped, 24, 150) -- maxhp: 150
setPedStat(ped, 24, 200) -- error: maxhp can only be between 1-150

I'm only guessing but the reason that we can have 200max hp is because of the armor can not be set to 150.

setPedStat(ped, 164, 1000) -- maxarmor: makes nothing, setting this stat doesn't do anything.

In singleplayer we have max armor at 100, but if we do the vigilante mission then it will be increased to 150.
In PS2 AFAIK it works, but the hud is misleading in all ports (ps2 and on pc as well) because the same width of amorbar applies to both 100armor and 150armor. (AFAIK in definitive edition they fixed these, if you do vigilante the armorbar will be wider)

In PC if I recall this feature is bugged and never worked in singleplayer as well. You couldn't have 150 armor even if you did the vigilante mission. (maybe a silentpatch could fix this but MTA doesn't (use/let use players to use) silentpatch.
The same goes for fast sprint with cj skin if you tap space and the fireproof cj as well.

I understand that we couldn't have these unless we can use silentpatch, so at least the HUD bugs on hpbar should be fixed in the future and also the setPedStat should be reworked in my opinion.

@bastianmarin
Copy link
Author

I'd like to point out something here because it has some connections with this issue. (kind of)

A few years ago I was testing someting with clientside setElementData and things of like that and I also noticed this desynchronization thing. I wanted to make a custom hp+armor script and firstly I wanted to make it with setElementData (I dropped the whole idea after some days) I just mentioned this that on clientside there will always be some desynchronization.

What I want to ask is, why the maximum hp is 200 and not 150?

In the singleplayer you can have a maximum of 150 hp. To get them you either need to do the paramedic side mission, or you'd need to exercise CJ's body to have more stamina+muslces to earn that slowly.

This is a problem because the game HUD is designed for 150 hp and I'm surprised that no one caught this yet in these years. I'm not sure if I can desrcibe it well, so I encourage everyone to set up a local server then try the following:

setPedStat(ped, 24, 1000)

This makes your charachter to have MAX_HEALTH of 200. After this set your current HP to 100.

setElementHealth(ped, 100)

Watch the HPBAR on the HUD. It'll look like its in the middle (almost) as it should. Now set it to 150, then check the HPBAR again. It'll look like its almost full, meanwhile there is still 50 hp left till 200. As i tested the HPBAR on the HUD visually maximises at 172 hp and all HP after that won't be seen. This can mislead players.

The other problem which also applies to this is the setPedStat itself which mislead the scripters.

setPedStat(ped, 24, 1000) -- maxhp: 200 (so if 1000points means 200maxhp)
setPedStat(ped, 24, 500) -- maxhp: 84 (why 500points is not 100maxhp?)
setPedStat(ped, 24, 565) -- maxhp: 100 (in fact 565points is 100maxhp...) -- getPedStat returns 569 by default

The best would be:

setPedStat(ped, 24, 100) -- maxhp: 100 -- making this as default
setPedStat(ped, 24, 150) -- maxhp: 150
setPedStat(ped, 24, 200) -- error: maxhp can only be between 1-150

I'm only guessing but the reason that we can have 200max hp is because of the armor can not be set to 150.

setPedStat(ped, 164, 1000) -- maxarmor: makes nothing, setting this stat doesn't do anything.

In singleplayer we have max armor at 100, but if we do the vigilante mission then it will be increased to 150. In PS2 AFAIK it works, but the hud is misleading in all ports (ps2 and on pc as well) because the same width of amorbar applies to both 100armor and 150armor. (AFAIK in definitive edition they fixed these, if you do vigilante the armorbar will be wider)

In PC if I recall this feature is bugged and never worked in singleplayer as well. You couldn't have 150 armor even if you did the vigilante mission. (maybe a silentpatch could fix this but MTA doesn't (use/let use players to use) silentpatch. The same goes for fast sprint with cj skin if you tap space and the fireproof cj as well.

I understand that we couldn't have these unless we can use silentpatch, so at least the HUD bugs on hpbar should be fixed in the future and also the setPedStat should be reworked in my opinion.

Who would have thought that after so many years I read something new about the difference in the GTA:SA ports. Good research. I guess the 84 life is done on purpose for when you don't take care of your character in the game. So that probably has to do with the 500 being 84 min health. Remember that in the game you can get "malnutrition" for not eating. I think that has something to do with this value.

@derxgbb
Copy link

derxgbb commented Oct 18, 2024

I think you misunderstood, setPedStat 24 is for setting MAX_HEALTH, that has nothing to do with starvation in the game. It's just the maximum health you can have, your current health is getting lowered by starvation and from damage.
A fresh game starts with a maxhp of 100 and it slowly increases to 150 by running, swimming, exercising or by doing the paramedic side mission. (I opened an issue about it, check it)

if you use setPedStat 24 with 500 value then your maxhp will be 84 which is lower than the default. It is a good thing if you want to make players to have way less maxhp by default.
My problem is (thinking of opening an issue for it) that its misleading scripters.

setPedStat(ped, 24, 1000) -- maxhp: 200 -- if 1000 for 200 then
setPedStat(ped, 24, 500) -- maxhp: 84 -- 500 should be for 100 not for 84

setPedStat(ped, 24, 569) -- maxhp: 100 -- this is the default now.

@FileEX
Copy link
Contributor

FileEX commented Oct 22, 2024

Personally, I don't see any reason for functions like setPedArmor and setElementHealth to be available on the client side at all for non-local elements. What's the purpose? Why is it easier to use setElementHealth on the client side rather than on the server side? Most events like onClientPlayerDamage also have their equivalent on the server side.

I understand the argument that removing these functions wouldn't change anything in terms of cheaters, because they can simply modify the value in memory, but in my opinion, it's still a big step forward. Let's not forget that most cheaters barely know how to start a computer, they use ready-made tools like lua executors, type in setElementHealth in some onClientRender and are literally invincible.

Regarding the issue itself... I think it's perfectly fine to add safe value reduction when receiving synchronization packets on the server side. In that case, an example value like 2047 will only be seen by the client who executed setPedArmor with that value, while the server and other players will receive the 'corrected' value.

As for the problem described by @derxgbb with MAX_HEALTH, it was explained in his issue. Regarding setPedStat, it's a matter of how these stats work in GTA. In San Andreas, it functions in such a weird and clunky way, and I honestly doubt anyone would be willing to overhaul the entire stat system to 'simplify' it for scripters. Also, let's not forget the legendary backward compatibility.

The problem with MAX_ARMOR, I think we can fix it, even if the bug is in GTA's code, but we'll need more details for that, preferably a separate issue

@PlatinMTA
Copy link
Contributor

PlatinMTA commented Oct 23, 2024

Most events like onClientPlayerDamage also have their equivalent on the server side.

Thats because of puresync. If the client doesnt get damaged then it wont trigger the serversided event (even if it should). The server has no actual clue if a bullet hit, or if someone has fallen, because it has no idea of where the world elements actually are and has no access to the engine. I don't think this is feasible or even possible.

I understand the argument that removing these functions wouldn't change anything in terms of cheaters

Yes and I don't get why we would want to limit scripters because of cheaters. Specially since this won't really fix the core issue (we need client's information to sync, MTA Server has no clue of the world). We as scripters just need to be clever about how we write our scripts.

For example if we disable the ability to skip damage clientside then we would let our players die if they jump those buggy walls that make the game think you are falling at high fps.

if weapon== 54 and getPedSimplestTask(localPlayer) == "TASK_SIMPLE_CLIMB" then
	cancelEvent()
	return
end

@FileEX
Copy link
Contributor

FileEX commented Oct 24, 2024

Most events like onClientPlayerDamage also have their equivalent on the server side.

Thats because of puresync. If the client doesnt get damaged then it wont trigger the serversided event (even if it should). The server has no actual clue if a bullet hit, or if someone has fallen, because it has no idea of where the world elements actually are and has no access to the engine. I don't think this is feasible or even possible.

I understand the argument that removing these functions wouldn't change anything in terms of cheaters

Yes and I don't get why we would want to limit scripters because of cheaters. Specially since this won't really fix the core issue (we need client's information to sync, MTA Server has no clue of the world). We as scripters just need to be clever about how we write our scripts.

For example if we disable the ability to skip damage clientside then we would let our players die if they jump those buggy walls that make the game think you are falling at high fps.

if weapon== 54 and getPedSimplestTask(localPlayer) == "TASK_SIMPLE_CLIMB" then
cancelEvent()
return
end

I think you misunderstood me. I'm not talking about completely removing synchronization or events, as that would be absurd. Synchronization is necessary to send game state information to other clients via the server, and that's not up for debate. What I meant is that functions like setElementHealth should not be available on the client side for non-local elements. This would slightly increase security, as a simple Lua executor wouldn't be able to manipulate player health anymore, and more advanced memory modification techniques would be needed (MTA has kernel-level protections, so bypassing them is a complicated process). In that case, canceling the onClientPlayerDamage event would work as intended. For years, the setPedArmor function was server-only, and it should stay that way. Additionally, the same should apply to setElementHealth. That's my humble opinion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants