Don't trust forwarding headers from clients, and fix the build - #12
Open
MarcosProWork wants to merge 1 commit into
Open
MarcosProWork wants to merge 1 commit into
MarcosProWork wants to merge 1 commit into
Conversation
The panel read X-Forwarded-For and CF-Connecting-IP from any request, so anyone could decide which IP got written to the auth log that fail2ban watches. That meant you could both dodge the login rate limit and get someone else's IP banned. Added a PROXY_TRUSTED_IPS setting: those headers are only used when the request actually comes from a proxy in that list, and it's empty by default. Set it if you're behind Cloudflare or similar. Also in the same area: the session IP check was overwriting REMOTE_ADDR with the CF header instead of appending it, the IP validators passed their argument to php unquoted so only the first word got checked, the netmask regex wasn't anchored, and the bin/ check in prevent_csrf used the wrong substr length. I couldn't get a clean install working while testing this, so the rest: - login page threw "Undefined constant load_tulio" on every request - composer.lock pointed at a tag that doesn't exist upstream - the build never ran composer install, so vendor/ was never in the deb - --with-debs globbed tulio_*.deb but we build tuliocp_*.deb, and the failure was hidden by 2>/dev/null - the apt repo published an empty Packages and a Release with no hashes, which is why apt says "Unable to locate package tuliocp" - .drone.yml and some docs still point at hst_autocompile.sh Added phpstan and shellcheck to CI. php -l parses load_tulio . config() just fine, so it needed something that actually resolves symbols.
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.
Forwarded headers
The panel read
X-Forwarded-ForandCF-Connecting-IPfrom any request and used the result as the client IP. That IP is what ends up in/var/log/tulio/auth.log, which thetulio-iptablesfail2ban jail bans on. So anyone could send a wrong password with a made up header value and either dodge the rate limit or get someone else's IP banned.Added a
PROXY_TRUSTED_IPSsetting (comma separated IPs and/or CIDRs, v4 and v6). Those headers are only honoured when the request actually reaches us from a proxy in that list. It's empty by default, so out of the box the headers are ignored andREMOTE_ADDRis used.If you're behind Cloudflare or another proxy you'll need to set it, otherwise every client looks like the proxy:
There's an upgrade script that adds the key and mentions this in the upgrade mail.
Same area, smaller things:
=instead of.=for the CF header, which droppedREMOTE_ADDRentirelyis_ip_format_validand friends passed$1to php unquoted, so1.2.3.4 anythingonly validated the first wordprevent_csrf.phpcompared a 22 char substr against a 21 char path, so that branch was deadBuild
I couldn't get a working install while testing the above, so this also fixes:
Undefined constant "load_tulio"on every request (load_tulio . config()should beload_tulio_config())composer.lockpointed at av1.1.0tag that doesn't exist and a dist URL that 404scomposer install, soweb/inc/vendorwas never in the package and the panel couldn't load at all--with-debsglobbedtulio_*.debbut we buildtuliocp_*.deb, and2>/dev/nullhid itPackagesand aReleasewith no hash sections, which is theE: Unable to locate package tuliocppeople hit.drone.ymlpointed athst_autocompile.sh(nowtst_autocompile.sh) and passed--tuliocp, which isn't a real flag so nothing got builtCI
test.ymlonly ran the docs tests. Added php lint, phpstan and shellcheck.php -lis happy withload_tulio . config()since it's valid syntax, so it needed something that resolves symbols. Also a check that the build scripts we reference still exist, since that's bitten us twice now.Testing
Installed on Debian 12 and checked the log before and after: with no proxy configured the spoofed header is ignored and the real IP is logged, with
127.0.0.1trusted the header is used again.checks.batspasses 63/63.