sec: harden UtilMethods.getURL — restrict to http(s) and block non-routable hosts

[mbiuki]

Closes dotCMS/private-issues#668

Proposed Changes

UtilMethods.getURL(String) opened a connection to any URI passed to it and returned the body, with no scheme allowlist and no host restriction. It is exposed to the Velocity template context as $UtilMethods.getURL (VelocityUtil), which makes it reachable by any user with design-layer (template/container) write access — a lower bar than administrator. That allowed local file read (file://) and full-read SSRF to loopback / link-local (cloud-metadata) / private hosts. The Velocity introspector denylist blocks reflection/Runtime/etc. but is class-based and does not cover this method.

This change makes getURL self-defend:

• Allow only http / https schemes (blocks file:, jar:, ftp:, gopher:, …).
• Reject loopback / any-local / link-local / site-local / multicast target hosts.
• Disable HTTP redirect following (autofix commit) so a redirect can't bounce to an internal target.
• On a blocked request, return an empty result and log via SecurityLogger.

Scope / compatibility: getURL has no callers in the Java source or in any shipped .vtl, so there is no functional impact. $UtilMethods stays in the template context and its other helpers (isSet, date/HTML utilities used by ~24 bundled templates) are unchanged.

Regression test

SstiGetUrlReproTest guards the fix — asserts file:// read returns empty and a loopback request is never made. Verified locally:

./mvnw test -pl :dotcms-core -Dtest=SstiGetUrlReproTest  ->  Tests run: 1, Failures: 0, Errors: 0  (BUILD SUCCESS)

Notes

Sensitive detail, full reproduction, and follow-up hardening (audit other context tools $import/$filetool/$velutil/$dotcontent; move the introspector to an allowlist; split content-edit from template-edit permissions) are tracked privately in dotCMS/private-issues#668.

🤖 Generated with Claude Code

[claude]

Claude finished @mbiuki's task in 4m 0s —— View job

---

Review: harden UtilMethods.getURL

• Read the diff (UtilMethods.java, SstiGetUrlReproTest.java)
• Cross-check against CircuitBreakerUrl / IPUtils and the two prior review comments
• Verify scheme allowlist, host guard, and test coverage

Verdict

The latest commit (7a4db6e6) resolves both prior review comments: getURL now routes through CircuitBreakerUrl (wezell) and adds a comprehensive isNonRoutable pre-check that closes the gaps rsh1k flagged in IPUtils. The bit-masking and getAllByName multi-record loop are correct. No blocking issues.

Resolved

• ✅ UtilMethods.java:1279 — now uses the shared CircuitBreakerUrl path (circuit breaker, timeout, redirects disabled) instead of a hand-rolled HttpURLConnection, as wezell requested.
• ✅ UtilMethods.java:1302 — isNonRoutable covers what IPUtils.isIpPrivateSubnet misses (rsh1k): all of 127/8 (isLoopbackAddress), 0.0.0.0 (isAnyLocalAddress), the full 169.254/16 link-local range incl. 169.254.170.2 ECS creds, IPv6 ::1/fe80::/10/fd00::/7 ULA, and IPv4 CGNAT 100.64/10. Masks verified: (b[0] & 0xFE) == 0xFC = fc00::/7; b[0]==100 && (b[1] & 0xC0)==0x40 = 100.64/10. Checking every resolved A/AAAA record closes the multi-record bypass.

New Issues

• 🟡 Medium: UtilMethods.java:1265 — DNS-rebinding TOCTOU (residual, non-blocking). The host is resolved once here (getAllByName) for the guard, then re-resolved independently inside CircuitBreakerUrl (both IPUtils.isIpPrivateSubnet(host) and the actual socket connect). A hostname whose DNS flips between resolutions could pass this pre-check with a public A record, then connect to an internal one; CircuitBreakerUrl's connect-time defense is IPUtils, which — per rsh1k — misses 127.0.0.2, IPv6, etc. This is not introduced by the PR (before it there was no protection), and closing it fully would require resolve-and-pin-the-IP. Assumption: the shared client re-resolves rather than reusing the pre-checked address. What to verify: whether pinning the vetted InetAddress into the request is feasible; otherwise fine to track in private-issues#668 as follow-up hardening. Fix this →

Notes (non-blocking, no action required)

• Behavior change with no shipped callers: the old code issued POST and returned null on non-2xx; the new path issues GET (builder default) and appends whatever body doString() returns even on a 4xx/5xx (it logs a warning but still returns the body). Given getURL has no Java or .vtl callers this is immaterial, but worth being aware of if it's ever reintroduced as a template helper.
• Fail-closed paths are correct: UnknownHostException and a null host (getAllByName(null) → loopback → blocked) both return the empty buffer.

The regression test (SstiGetUrlReproTest) exercises file://, loopback, IPv6 loopback, CGNAT, ULA, any-local, and the ECS link-local endpoint — good coverage of the guard.
· branch sec/harden-utilmethods-geturl-668

[mbiuki]

fixes https://github.com/dotCMS/private-issues/issues/668

[mbiuki]

CI results (head 4217d59c8f):

Check	Result
PR Build / Initial Artifact Build	✅ pass
PR Test / JVM Unit Tests (runs SstiGetUrlReproTest)	✅ pass
Add Issue to PR / link-issue	✅ pass
PR Test / Integration Tests – MainSuite 2a	❌ fail — unrelated

The MainSuite 2a failure is com.dotmarketing.portlets.personas.business.PersonaAPITest (AssertionError: expected:<200> but was:<500> in the auth/ResponseUtil path). This PR's diff is two files — the UtilMethods.getURL guard and its regression test — with no persona/auth code touched, and JVM Unit Tests (which exercises the change) is green. It's a flaky/environmental failure, not caused by this change; re-ran the failed job.

The security change itself is verified: SstiGetUrlReproTest passes in CI (JVM Unit Tests) and locally (file:// read blocked, loopback SSRF blocked). Details in dotCMS/private-issues#668.

🤖 Generated with Claude Code

[wezell]

I would just call CircuitBreakerUrl....toString which already has all this hardening.

[rsh1k]

I agree to this. $import already goes through CircuitBreakerUrl, and $xmltool gates on
IPUtils. getURL is the odd one out, so it should use the shared path too.
But IPUtils.isIpPrivateSubnet is weaker than the check in this PR.
Its default list (127.0.0.1/32, 10/8, 172.16/12, 192.168/16, 169.254.169.254/32) misses
127.0.0.2+ (it's /32, not /8), 0.0.0.0, all of IPv6 (SubnetUtils throws on v6 and
isIpInCIDR swallows it, so http://[::1]:8080/ passes), and link-local beyond
169.254.169.254 — e.g. 169.254.170.2, the ECS credentials endpoint.

[mbiuki]

Done — getURL now fetches through CircuitBreakerUrl.builder()…doString() (same shared path $import/ImportTool uses): circuit breaker, timeout, IPUtils private-subnet gate, redirects disabled. Being HTTP-only it also drops the file:// read path. Pushed in 7a4db6e622.

[mbiuki]

Agreed on both points, and thanks for the precise gap list. Routed getURL through CircuitBreakerUrl — but kept a strong pre-connect host guard exactly because IPUtils.isIpPrivateSubnet is weaker: it now resolves via getAllByName and rejects if any address is non-routable (multi-record bypass), using an isNonRoutable() helper that covers what you listed — full 127/8, 0.0.0.0, all of 169.254/16 (incl. 169.254.170.2), and IPv6 (::1, fe80::/10, plus fd00::/7 ULA and 100.64/10 CGNAT). Verified: file://, [::1], 100.64.0.1, [fd00::1], 0.0.0.0, 169.254.170.2 all return empty in the regression test. 7a4db6e622.

Strengthening IPUtils itself (IPv4 default list + IPv6 support so $import/$xmltool benefit) is worth doing but has wider blast radius — filed as a follow-up on dotCMS/private-issues#668 rather than widening this PR.

[mbiuki]

Pushed 7a4db6e622 addressing all review feedback:

@wezell / @rsh1k — reuse the shared path: getURL now fetches via CircuitBreakerUrl (the client $import uses), replacing the hand-rolled URLConnection. HTTP-only, so file:// is gone; circuit breaker + timeout + IPUtils gate + redirects-disabled come for free.

Automated review items:

• ✅ Multi-address bypass — now resolves with getAllByName and rejects if any A/AAAA record is non-routable.
• ✅ IPv6 ULA (fd00::/7) + CGNAT (100.64/10) — added explicit checks in a new isNonRoutable() helper (the JDK isXxx() predicates miss both).
• ✅ Cosmetic jar://→jar: — fixed.
• ⏳ TOCTOU / DNS-rebinding — deferred to dotCMS/private-issues#668. Closing it means pinning the connection to the already-validated InetAddress, which CircuitBreakerUrl doesn't expose; it's a client-level change better done once for all consumers. Noted as non-blocking in the review.
• ℹ️ URL in security log — left as-is (intentional audit signal; no credentials involved).

Regression test extended to assert file://, IPv6 ::1, CGNAT, ULA, 0.0.0.0, and 169.254.170.2 all return empty — ./mvnw test -pl :dotcms-core -Dtest=SstiGetUrlReproTest → BUILD SUCCESS.

@wezell — ready for another look when you have a moment.

🤖 Generated with Claude Code