Fix artwork webserver IP allowlist bypass over IPv6 - #1255
Open
clevervi wants to merge 1 commit into
Open
Conversation
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
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.
Closes #1256
While reading through the custom models component I noticed the artwork webserver only enforces its player-IP allowlist for IPv4 connections. The pre-routing handler checks
AF_INETagainstallowedIPs_, but theAF_INET6branch was just an empty// TODO: Add IPV6 support, so any request coming in over IPv6 skipped the check entirely and got served.Since
allowedIPs_is only ever populated with the IPv4 address of connected players (allowIPAddresstakes auint32_t, andonPlayerConnectpassesnetworkID.address.v4), an IPv6 client can never legitimately be on the allowlist anyway. In practice that meant the restriction was silently bypassed and anyone could pull every.txd/.dffthe server hosts just by requesting over IPv6.This makes the handler deny by default: only allowlisted IPv4 requests get through, everything else gets a 401. It closes the bypass without pretending to add IPv6 allowlisting — doing that properly would need the map and
allowIPAddress/onPlayerConnectto actually track v6 addresses, which I'm happy to do as a follow-up if you'd prefer that route.The fix stands on the current behaviour alone:
allowedIPs_only ever holds IPv4 addresses, so an IPv6 request can never legitimately be in it and should be denied rather than served.For context, this also looks like it lines up with the behaviour before the
sockaddrrefactor in 9cbf0f9: back then the handler rangetpeernameand cast the result tosockaddr_inunconditionally, so a non-IPv4 peer would have been run through the IPv4 allowlist check anyway and, not being in it, denied. When the check was split into explicitAF_INET/AF_INET6branches, the empty IPv6 branch started falling through toreturn Unhandled. I haven't verified the old path at runtime, so treat that part as context rather than the reason for the change — the point is just to deny by default while IPv6 allowlisting stays aTODO.Worth noting: IPv6 players couldn't download custom models before this change either (they were never added to the allowlist), so this doesn't remove any working behaviour — it just makes the denial explicit instead of accidentally allowing everyone.