[ANCHOR-1236]: SEP-10 client_domain parameter causes server-side HTTP requests to arbitrary attacker-controlled hosts (SSRF) - #1984
Merged
Conversation
* add client domain allow list enforcement for non-custodial sep10 authentication * prevent fetching client domain signing keys from domains not in the allow list, improving security and efficiency * refactor error messages for client domain signing key fetch to prevent leaking attempted urls * add tests for new client domain validation logic and secure error messaging
amandagonsalves
changed the title
client_domain parameter causes server-side HTTP requests to arbitrary attacker-controlled hosts (SSRF)client_domain parameter causes server-side HTTP requests to arbitrary attacker-controlled hosts (SSRF)
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens SEP-10 client attribution against SSRF and reduces URL disclosure in failed TOML fetches.
Changes:
- Validates supplied client domains against configured domains.
- Replaces URL-bearing fetch errors with a generic message.
- Adds rejection, permissive-default, and disclosure tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ClientDomainHelper.java |
Sanitizes fetch failure messages. |
Sep10Service.java |
Adds optional-attribution allow-list enforcement. |
ClientDomainHelperTest.kt |
Tests URL disclosure behavior. |
Sep10ServiceTest.kt |
Tests allow-list rejection and empty-list behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* add `sep10.client_allow_list` configuration to explicitly define allowed client domains * update `sep10` validation logic to use the new explicit client allow list * add tests for the updated client domain validation behavior
JiahuiWho
approved these changes
Jul 27, 2026
9 tasks
amandagonsalves
added a commit
that referenced
this pull request
Aug 19, 2026
… requests to arbitrary attacker-controlled hosts (SSRF) (#1991) ### Description The ANCHOR-1236 fix (PR #1984) closed the missing-allow-list gap but never touched `ClientDomainHelper.validateDomainNotPrivateNetwork`, the private-network guard that runs on mainnet before fetching a `client_domain`'s `stellar.toml`. Follow-up review on report 3824178 found that guard doesn't actually protect the fetch it's meant to gate: it resolves the hostname once (`InetAddress.getAllByName`) to decide pass/fail, but the fetch itself goes through OkHttp's default client, which resolves the hostname again, independently, at connect time. An attacker who controls DNS for their own domain can return a public address for the check and an internal address for the fetch — the two resolutions never have to agree. Fixing just the resolution race isn't enough either: the same four `InetAddress` predicates the guard relies on (`isLoopbackAddress`, `isSiteLocalAddress`, `isLinkLocalAddress`, `isAnyLocalAddress`) never recognized carrier-grade NAT (`100.64.0.0/10`, notably used by AWS EKS/VPC-CNI for pod ranges) or IPv6 unique-local addresses (`fc00::/7` — distinct from the deprecated `fec0::/10` IPv6 site-local range `isSiteLocalAddress` actually checks). Those ranges pass the guard with a single static DNS answer, no rebinding needed. Both gaps are fixed together in `ClientDomainHelper`, since they're independent ways of reaching the same outcome (an internal-network fetch on mainnet) and share the same underlying address classifier. **Changes** - [x] `ClientDomainHelper.isNonPublicAddress`: new shared classifier, extending the existing four checks with `100.64.0.0/10`, `fc00::/7`, and — since we were touching this anyway — the related `192.0.0.0/24` (IETF protocol assignments) and `198.18.0.0/15` (benchmarking) reserved ranges. `validateDomainNotPrivateNetwork` now delegates to it, so its existing behavior/tests are unchanged beyond the wider coverage. - [x] `ClientDomainHelper.pinnedValidatingDns`: new `okhttp3.Dns` implementation that resolves and validates a hostname in one step — `Dns.SYSTEM.lookup`, then `isNonPublicAddress` against the same result, then returns those exact addresses to OkHttp. `fetchSigningKeyFromClientDomain` builds this once (mainnet only) and threads it through to the actual connection, so there's no second, independent resolution to disagree with the one that was validated. Non-mainnet callers are unaffected — `dns` stays `null` and the fetch uses the plain, unparameterized path exactly as before. - [x] `NetUtil.fetch` / `OkHttpUtil.buildClient`: new overloads accepting a `Dns`, additive only — the existing no-arg/`Dns`-less overloads are untouched and still used by every other caller in the codebase. - [x] `Sep1Helper.readToml`: new `Dns`-accepting overload alongside the existing one, same reasoning. - [x] `ClientDomainHelperTest.kt`: added coverage for the four new ranges (including boundary cases just outside the CGNAT range, and a public-IPv6 sanity check to guard against false positives), plus an end-to-end test that runs a real loopback HTTP server and confirms `fetchSigningKeyFromClientDomain(..., allowHttpRetry=false)` can't reach it — proves the new `Dns` wiring is actually connected through `NetUtil`/`Sep1Helper`/`OkHttpUtil`, not just that the classifier rejects the string in isolation. **Acceptance Criteria** - [x] On mainnet, a `client_domain` that resolves to `100.64.0.0/10` or `fc00::/7` is rejected before any fetch, the same as the existing loopback/RFC-1918/link-local ranges. - [x] On mainnet, a `client_domain` whose DNS answer differs between the validation lookup and the fetch's connection lookup cannot reach an address that would have failed validation — the fetch always connects to the address that was actually checked. - [x] Non-mainnet (`allowHttpRetry=true`) behavior is unchanged: no private-network validation, same permissive fetch path as before. - [x] SEP-45 (`Sep45Service`, which calls the same `ClientDomainHelper.fetchSigningKeyFromClientDomain`) gets both fixes automatically, with no SEP-45-specific change needed. ### Context [HackerOne #3824178](https://hackerone.com/reports/3824178) ### Testing - Unit: `./gradlew :core:test --tests "org.stellar.anchor.util.ClientDomainHelperTest"` - Unit: `./gradlew :core:test --tests "org.stellar.anchor.util.NetUtilTest" --tests "org.stellar.anchor.util.SepHelperTest" --tests "org.stellar.anchor.util.OkHttpUtilTest"` - Regression: `./gradlew :core:test --tests "org.stellar.anchor.sep10.Sep10ServiceTest" --tests "org.stellar.anchor.sep45.Sep45ServiceTest"` - Full module: `./gradlew :core:test :platform:test :lib-util:test` ### Documentation N/A ### Known limitations N/A
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.
Description
Sep10Service.validateChallengeRequestClientonly checks a submittedclient_domainagainst the configured allow list (sep10Config.getAllowedClientDomains()) whensep10.client_attribution_requiredistrue. That flag defaults tofalse(PropertySep10Config.java:32), so on a default configuration any hostname supplied asclient_domainis passed straight through toClientDomainHelper.fetchSigningKeyFromClientDomainwith no allowlist check at all. On mainnet,validateDomainNotPrivateNetworkstill blocks loopback/RFC-1918/link-local targets, so the practical impact is SSRF to arbitrary public hosts, plus the fetch failure message echoing the full attempted URL(s) back to the caller — a minor information disclosure on top.The fix has two independent parts, and neither alone is sufficient: validating the allow list closes the SSRF path itself, but leaves the error message leaking exactly which URLs the server attempted to reach for any input, including domains rejected for other reasons upstream (e.g. the private-network check on mainnet). Both needed fixing.
Changes
Sep10Service.validateChallengeRequestClient: added anelse ifbranch so that wheneverclient_domainis provided andclient_attribution_requiredisfalse, it's still checked againstsep10Config.getAllowedClientDomains()— but only rejected if that list is non-empty. This is deliberately separate from the existingclient_attribution_requiredbranch (which stays fail-closed on an empty list, unchanged) rather than merged into one shared check: the new branch is intentionally lenient on an empty list so operators with no non-custodial clients configured at all (today's fully-permissive default for that edge case) aren't suddenly locked out, while any operator who does have clients/an allow list configured is now protected regardless of whether they've explicitly opted intoclient_attribution_required.ClientDomainHelper.fetchSigningKeyFromClientDomain: theIOExceptionhandler no longer echoes the attempted URL(s) into theSepExceptionmessage returned to the caller (previously"Unable to read from both %s and %s"); now a fixed, generic"Unable to read client_domain's SIGNING_KEY". The attempted URL is still logged server-side via the existinginfoF("Unable to read from {}", url)call, just not surfaced in the API response.Sep10ServiceTest.kt: added a test proving aclient_domainoutside a configured allow list is rejected withSepNotAuthorizedExceptionand thatClientDomainHelper.fetchSigningKeyFromClientDomainis never invoked (verified viaverify(exactly = 0)) — the SSRF-triggering fetch genuinely never happens, not just that the request eventually fails. Added a companion test confirming the pre-existing permissive behavior is preserved when no clients are configured at all (empty allow list).ClientDomainHelperTest.kt: added a test asserting the exception message from a failed fetch contains neither the target domain nor"http".Acceptance Criteria
POST /auth(orGET /auth) with aclient_domainnot present in the configured allow list is rejected before any outbound HTTP request is made, when the operator has at least one non-custodial client or an explicitsep10.client_allow_listconfigured — regardless ofclient_attribution_required.client_domaincontinues to work exactly as before.client_domainTOML fetch returns a generic error message; the attempted URL(s) are not present in the HTTP response body.client_attribution_required = truebehavior (reject on empty allow list, requireclient_domainto be present) is unchanged.Context
HackerOne #3824178
Testing
./gradlew :core:test --tests "org.stellar.anchor.sep10.Sep10ServiceTest"./gradlew :core:test --tests "org.stellar.anchor.util.ClientDomainHelperTest"./gradlew :core:test :platform:test(full suite, confirms no regressions in either module)Documentation
N/A
Known limitations
N/A