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 @wezell's task in 2m 25s —— View job

---

PR Review — harden UtilMethods.getURL

• Read diff, final method, and CircuitBreakerUrl shared path
• Verify prior review findings addressed
• Check for new issues

The change is solid and correctly addresses all the earlier review feedback. getURL now routes through CircuitBreakerUrl (the same hardened path $import uses) with redirects disabled, backed by a strong pre-connect host guard. No blocking issues.

New Issues

None.

Existing

• 🟡 Medium: UtilMethods.java:1264-1284 — TOCTOU / DNS rebinding. The strong isNonRoutable() check resolves getHost() via getAllByName, but CircuitBreakerUrl.doString() re-resolves DNS independently at connect time (UtilMethods.java:279). A rebinding host that returns a public A record on the first lookup and a private one on the second is only gated by the weaker IPUtils.isIpPrivateSubnet inside CircuitBreakerUrl (CircuitBreakerUrl.java:274) — which, as rsh1k noted, misses 127.0.0.2+, 0.0.0.0, IPv6, and link-local beyond 169.254.169.254. So the strong guard is bypassable via rebinding. Non-blocking — explicitly acknowledged and deferred to dotCMS/private-issues#668 (closing it means pinning the connection to the validated InetAddress, a client-level change for all consumers). Noted here only so it stays tracked.

Resolved

• ✅ UtilMethods.java — now fetches via CircuitBreakerUrl instead of the hand-rolled URLConnection (wezell). file:///jar: read path is gone (http(s)-only), and System.setProperty(...) calls removed (aligns with the no-System.* rule).
• ✅ UtilMethods.java:1265 — multi-address bypass closed: iterates all A/AAAA records from getAllByName and rejects if any is non-routable.
• ✅ UtilMethods.java:1296-1313 — isNonRoutable() covers the IPUtils gaps rsh1k listed: full 127/8, 0.0.0.0, all of 169.254/16 (incl. ECS creds 169.254.170.2), IPv6 (::1, fe80::/10, fd00::/7 ULA), and IPv4 CGNAT 100.64/10. Bit masks verified correct (fc00::/7 → b[0] & 0xFE == 0xFC; 100.64/10 → b[0]==100 && b[1] & 0xC0 == 0x40).
• ✅ Redirects disabled (setAllowRedirects(false)) so a 3xx can't bounce to an internal target after the check.

Notes (non-blocking)

• getURL now returns an empty StringBuffer on a blocked/failed request rather than the previous null (old non-200 path returned null). Since the PR confirms getURL has no Java or .vtl callers, this is a benign, arguably safer, contract change — worth a one-line mention in the PR body.
• IPv4-mapped IPv6 literals (::ffff:127.0.0.1) are safe here: the JDK normalizes them to Inet4Address, so the loopback/link-local predicates still fire.

Good, well-scoped security fix — approve once the TOCTOU follow-up stays tracked in #668.

_{· 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

[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

[wezell]

Good, thanks Mehdi!