Security and robustness fixes#73
Open
ASTROWwwW wants to merge 8 commits into
Open
Conversation
The step-counting `while true` loops had no Wait and never terminated when `time <= 0` (addVolume gets the wrong sign so the volume never reaches the exit bound) which hard-freezes the client thread. A zero starting volume made `called` stay 0, so the second loop did `Wait(time / 0)`. Replaced the busy counting loop with a direct step computation, guarded time/volume inputs, and added a soundExists re-check after each Wait so a sound destroyed mid-fade no longer nil-indexes.
Most info/manipulation exports indexed soundInfo[name].field directly, so any call on a missing or already-destroyed sound raised an attempt-to-index-nil error (Distance, Position, Resume, Pause, setVolume, setVolumeMax, setSoundLoop, getLink, getPosition, isLooped, isPlaying, getDistance, getVolume, isDynamic, and the timestamp getters). Added soundExists guards that return a safe default. Destroy now snapshots the info before clearing the entry, so the onPlayEnd callback receives the real sound info instead of nil.
getYoutubeUrlId did urlParts[1].substring(...) without checking the split result, so a URL containing "youtube"/"youtu.be" but no video id (a playlist link, a bare domain) threw and broke create()/sendMaxDurationToClient. Added length/undefined and type guards. sanitizeURL only ran DOMPurify (an HTML sanitizer, not a URL validator) and stripped tags, accepting any scheme; it now requires http(s) or a relative path and returns "" otherwise, so data:, javascript: and other unexpected schemes are rejected before reaching Howl.
soundList was declared as an array ([]) but only ever keyed by sound name
(soundList[item.name]) and iterated with for...in. Mixing array and dictionary
semantics is fragile and misleading. Switched it to a plain object ({}), which
matches how it is actually used.
The muzik-* server events trusted client-supplied arguments and rebroadcast them to everyone (-1). A client could spoof another player's id (muzikAdi / serverId were taken from the payload, not from source) to hijack or stop other players' music, and could pass an arbitrary link that every client would then load, which leaks every player's IP to an attacker-controlled URL. The owner id is now taken from source, and the link is validated against the only two shapes the addon produces (phone-ring/<file> or a youtube watch URL) before broadcast.
These server events are designed to be client-triggered, so they cannot be permission-gated without breaking normal usage, but they were exploitable two ways. The soundFile argument was concatenated into a path on the client (./sounds/<file>) with no validation, allowing path traversal (../) out of the sounds folder; it is now rejected unless it is a plain relative name. And any client could spam global/targeted playback in a loop; a short per-source cooldown now drops rapid-fire triggers. Dropped players are cleared from the cooldown table.
The timestamp thread called getInfo(v.id) four times per sound per second; it now resolves the info once into a local and nil-checks it. The far-away cleanup thread fetched the player ped and coords every 500ms even with no active sounds; it now skips that work when soundInfo is empty.
The position thread had two identical ~25-line if/elseif speed ladders, one per mp3 branch, differing only in the on-foot default distance (10 vs 15). Extracted the ladder into distanceForSpeed and kept the on-foot default inline, removing the duplication without changing behaviour.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A set of focused security and robustness fixes for xsound. Each fix is its own
commit with the reasoning in the commit message. No behaviour change for valid
usage; the changes harden the failure and abuse paths.
Security
muzik-*server eventsrebroadcast client-supplied arguments to everyone. A client could spoof
another player's id to hijack/stop their music, and pass an arbitrary
linkthat every client would load — leaking every player's IP to an
attacker-controlled URL. The owner id is now taken from
sourceand the linkis validated against the only two shapes the addon produces.
soundFilewasconcatenated into
./sounds/<file>on the client with no validation,allowing
../traversal out of the sounds folder; it is now rejected unlessit is a plain relative name. A short per-source cooldown drops rapid-fire
global/targeted playback spam. These events are client-triggered by design,
so they are hardened rather than permission-gated to avoid breaking normal
usage.
sanitizeURLonly ran DOMPurify (an HTML sanitizer)and accepted any scheme; it now requires http(s) or a relative path, so
data:/javascript:and other unexpected schemes are rejected beforereaching Howl.
Robustness
while trueloops hadno
Waitand never terminated whentime <= 0, hard-freezing the client; azero volume caused a
Wait(time / 0). Replaced with a direct stepcomputation, guarded inputs, and a
soundExistsre-check after eachWait.soundInfo[name].fielddirectly, raising attempt-to-index-nil on a missingor already-destroyed sound. Added guards returning safe defaults.
onPlayEndwas called after the entry was cleared, soit received
nil; it now gets the real info snapshot.youtube/youtu.bebut no videoid threw on
urlParts[1].substring; guarded.an object.
Performance
getInfoonce per sound in the timestamp thread instead of fourtimes; skip ped/coord work in the far-away cleanup thread when no sounds are
active.
Not included (intentional)
The NUI loads howler, jQuery and DOMPurify from remote CDNs in
index.html.Serving them locally would remove the runtime network dependency, but vendoring
third-party libraries is an infra decision left to the maintainer.
Validation
All modified Lua files pass
luac -p; all modified JS files passnode --check. Not tested live in-game — runtime verification on a server isrecommended before merging.