fix(security): close two ALLOWED_DOMAINS bypasses and add an SSRF guard - #153
Open
manantlerio wants to merge 1 commit into
Open
manantlerio wants to merge 1 commit into
manantlerio wants to merge 1 commit into
Conversation
…ddresses
The domain allowlist could be bypassed two different ways, and there was no
guard on where a fetch could be pointed. All three are fixed here, with tests.
1. ALLOWED_DOMAINS matched on a prefix
fetchSite used StringInSlice, which is strings.HasPrefix. An allowlist of
"nytimes.com" therefore also permitted "nytimes.com.example.net", a hostname
anyone can register. It also compared against u.Host, so a port defeated an
exact entry.
Host matching now goes through domainAllowed, which requires an exact match
or a match on a dot boundary, lowercases both sides, and tolerates a
trailing root dot and a port. StringInSlice is left alone: it is still the
right prefix test for rule paths, which is its only remaining caller.
2. ALLOWED_DOMAINS_RULESET allowed everything
RuleSet.Domains appended rule.Domain unconditionally, and rules written with
only the plural `domains:` key have an empty Domain field. Three of the
bundled rulesets are written that way, so an empty string landed in the
allowlist, and HasPrefix(anything, "") is true. Turning the option on
removed the restriction it was supposed to add.
Domains now skips blank entries, and the allowlist ignores blanks anyway, so
neither half can silently allow everything on its own. The same applies to
an unset ALLOWED_DOMAINS: strings.Split("", ",") returns []string{""}, which
used to become a match-everything entry rather than an empty list.
3. No SSRF guard
ladder fetches server-side, so anything that can reach it could ask it to
read http://localhost, the rest of the network it runs on, or a cloud
provider's instance metadata service at 169.254.169.254 and get the response
body back. That matters most in the common deployment, a container sitting
alongside other services.
Page fetches now use a transport whose dialer refuses loopback, private,
link-local, CGNAT, multicast and unspecified addresses. The check runs on
the address actually being dialled, immediately before connect, so it covers
redirects and hostnames that resolve into a private range rather than only
the URL as written. ALLOW_PRIVATE_ADDRESSES=true opts out for people
pointing ladder at their own network on purpose.
FlareSolverr and remote ruleset loading are deliberately untouched: those
hosts come from the operator's configuration, not from a request, and are
normally private.
The allowlist is also re-applied on every redirect hop, which it was not
before, and the redirect chain is bounded at the same 10 the stdlib uses.
Tests are in handlers/security_test.go rather than added to the existing
handlers/*.test.go files, because those are named with a dot rather than an
underscore and so are never picked up by `go test`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Summary
Three related security fixes, with tests. First off, thanks for ladder, it is a
genuinely useful little tool. I read the source before self-hosting it and found
that the
ALLOWED_DOMAINSallowlist can be bypassed two different ways, andthat there is no restriction on where a fetch can be pointed. Happy to split
this into separate PRs or drop part 3 if you would rather take these one at a
time.
1.
ALLOWED_DOMAINSmatched on a prefixfetchSitechecked the host withStringInSlice, which isstrings.HasPrefix:So
ALLOWED_DOMAINS=nytimes.comalso allowednytimes.com.example.net,which is a hostname anyone can register and point wherever they like. It also
compared against
u.Host, so a port defeated an otherwise exact entry.Confirmed against the current
main:Host matching now goes through
domainAllowed, which requires an exact match ora match on a dot boundary, lowercases both sides, and tolerates a trailing root
dot and a port.
StringInSliceis left as it is: it is still the correct prefixtest for rule paths, which is now its only caller.
2.
ALLOWED_DOMAINS_RULESET=trueallowed everythingRuleSet.Domains()appendedrule.Domainunconditionally:Rules written with only the plural
domains:key have an emptyDomainfield.Three of the bundled rulesets are written that way (
nytimes-com.yaml,_multi-conde-nast.yaml,_multi-metroland-media-group.yaml), so an emptystring ended up in the allowlist, and
strings.HasPrefix(anything, "")isalways true. Turning the option on therefore removed the restriction it was
meant to add. Against current
main:Domains()now skips blanks, and the allowlist ignores blanks independently, soneither half can quietly allow everything on its own.
The same class of bug applied to an unset
ALLOWED_DOMAINS:strings.Split("", ",")returns[]string{""}, which became a match-everythingentry rather than an empty list. It reached the right outcome by accident. It is
now an explicit empty list.
3. No SSRF guard
ladder fetches server-side, so anything that can reach it can ask it to read
http://localhost, the rest of the network it is running on, or a cloudprovider's instance metadata service at
169.254.169.254, and get the responsebody back. That matters most in the deployment the compose file encourages: a
container sitting on a network next to other services.
Page fetches now use a transport whose dialer refuses loopback, private,
link-local, CGNAT, multicast and unspecified addresses. The check runs on the
address actually being dialled, immediately before connect, rather than on the
URL as written, so it also covers redirects and hostnames that resolve into a
private range.
ALLOW_PRIVATE_ADDRESSES=trueopts out, for people deliberately pointing ladderat hosts on their own network.
FLARESOLVERR_HOSTand remoteRULESETloading are deliberately untouched:those hosts come from the operator's configuration rather than from a request,
and are normally private.
Also
The allowlist is now re-applied on every redirect hop. Previously an allowed
domain could redirect anywhere and only the first request was ever checked. The
chain is bounded at the same 10 the stdlib uses.
Compatibility
is the one thing in here that could surprise an existing user, and it is
opt-out with a single env var. Say the word if you would rather it defaulted
the other way and I will flip it.
dial is blocked too, and
ALLOW_PRIVATE_ADDRESSES=trueis the escape hatch.Noted in the README.
ALLOWED_DOMAINSnow rejects hostnames it used to accept. That is the point,but it will look like a change to anyone who was relying on the loose match.
ALLOWED_DOMAINSunset.README.zh-CN.mdhas the same env var table and has not been touched, since Ido not want to drop English into the translation. Happy to add it if you point
me at the wording you want.
Tests
go vet,gofmtandgo buildare clean, andgo test ./...passes.New tests live in
handlers/security_test.gorather than being added to theexisting
handlers/*.test.gofiles, because those are named with a dot ratherthan an underscore and so are never compiled as tests by
go test. Worth aseparate look at some point, since they are not running today.
Both allowlist tests were checked against unpatched
mainfirst and fail there,so they are real regression tests rather than tests written to fit the fix.