Roblox / Core-Scripts

All of ROBLOX's core client scripts.
Apache License 2.0
255 stars 182 forks source link

Sound StarterCharacterScript is needlessly insecure #1028

Closed Kampfkarren closed 7 years ago

Kampfkarren commented 7 years ago

For those who don't know, there is a property in the Sound object called "Playing", which can be used to toggle whether a sound is playing or not, as the name suggests. The reason I bring this up is because this Property is different from almost every other Roblox property in that it replicates to the server, that is it bypasses non-Experimental Mode. checks

This is a problem and has caused any game unaware of it to have an exploit where a hacker can repeatedly play every sound in the game over and over again, regardless of whether Experimental Mode is on or off. This has caused many developers, including me, to write our own sound systems that create sounds on the client rather than having them on the server at all.

The Sound StarterCharacterScript responsible for creating the sounds of a character (walking, jumping, dying, etc) problematically creates the sounds of the character on the server. This means any unaware developer will have this exploit used on their games with every sound a character could have be repeatedly played on loop. There is no reason for the sounds to be created on the server. LocalSound should be completely responsible for the playing and creating of sounds without any involvement of the server. This will solve the issue of the exploit (for character sounds at least) because all the sounds are created on the client, not on the server.

I can create this functionality as I already have done for my own game, but the main problem is backwards compatibility. I can assume some games rely on the sounds being created on the server, so the more secure character sound system will have to be enabled rather than be the default. Because of the limitations of CoreScripts (in this case, CoreScripts cannot create custom properties), it will have to be a dirty way to implement rather than a property as it should be. When I create my PR, if it gets approved I ask that it not be merged until Roblox creates a property on either SoundService or more accurately StarterPlayer to enable it. I think the way I am going to have it enabled is it will check if you have an object named "UseNewSoundSystem" in SoundService.

Jody7 commented 7 years ago

I believe this is resolved by setting Soundscape to "RespectFilteringEnabled"

Kampfkarren commented 7 years ago

That's a feature? I didn't know that. My bad.