Skip to content

Don't trust forwarding headers from clients, and fix the build - #12

Open
MarcosProWork wants to merge 1 commit into
Contaura:mainfrom
MarcosProWork:fix/trusted-proxy-and-build
Open

MarcosProWork wants to merge 1 commit into
Contaura:mainfrom
MarcosProWork:fix/trusted-proxy-and-build

Conversation

@MarcosProWork

Copy link
Copy Markdown

Forwarded headers

The panel read X-Forwarded-For and CF-Connecting-IP from any request and used the result as the client IP. That IP is what ends up in /var/log/tulio/auth.log, which the tulio-iptables fail2ban 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_IPS setting (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 and REMOTE_ADDR is used.

If you're behind Cloudflare or another proxy you'll need to set it, otherwise every client looks like the proxy:

v-change-sys-config-value PROXY_TRUSTED_IPS '198.51.100.10,203.0.113.0/24'

There's an upgrade script that adds the key and mentions this in the upgrade mail.

Same area, smaller things:

  • the session IP check used = instead of .= for the CF header, which dropped REMOTE_ADDR entirely
  • is_ip_format_valid and friends passed $1 to php unquoted, so 1.2.3.4 anything only validated the first word
  • the netmask regex had no end anchor
  • prevent_csrf.php compared a 22 char substr against a 21 char path, so that branch was dead

Build

I couldn't get a working install while testing the above, so this also fixes:

  • login page died with Undefined constant "load_tulio" on every request (load_tulio . config() should be load_tulio_config())
  • composer.lock pointed at a v1.1.0 tag that doesn't exist and a dist URL that 404s
  • the build never ran composer install, so web/inc/vendor was never in the package and the panel couldn't load at all
  • --with-debs globbed tulio_*.deb but we build tuliocp_*.deb, and 2>/dev/null hid it
  • the apt repo published an empty Packages and a Release with no hash sections, which is the E: Unable to locate package tuliocp people hit
  • .drone.yml pointed at hst_autocompile.sh (now tst_autocompile.sh) and passed --tuliocp, which isn't a real flag so nothing got built

CI

test.yml only ran the docs tests. Added php lint, phpstan and shellcheck. php -l is happy with load_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.1 trusted the header is used again. checks.bats passes 63/63.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant