fix(files/storage): add SSRF host validation to DAV storage constructor#41576
fix(files/storage): add SSRF host validation to DAV storage constructor#41576DeepDiver1975 wants to merge 3 commits into
Conversation
The DAV storage class accepted arbitrary host values (including loopback, link-local 169.254.x.x, RFC-1918 private ranges, and IPv6 equivalents) without validation. When user external storage mounting is enabled, any authenticated user could force the server to make HTTP requests to cloud metadata endpoints, localhost services, or internal network hosts. Adds validateHost() called immediately after scheme-stripping so all downstream HTTP sinks (Guzzle GET/PUT and Sabre DAV Client) are covered. Signed-off-by: Thomas Müller <thomas.mueller@owncloud.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
… pass SSRF validation Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
DeepDiver1975
left a comment
There was a problem hiding this comment.
Code Review — fix(files/storage): add SSRF host validation to DAV storage constructor
Overview: DAV::__construct() accepted arbitrary host values without IP range validation. When user external storage mounting is enabled, an authenticated user could supply 169.254.169.254 (AWS metadata), loopback, or RFC-1918 addresses. The fix adds validateHost() called immediately after prefix-stripping, throwing InvalidArgumentException for blocked ranges.
Architecture
The implementation splits responsibility cleanly:
validateHost(string $host)— parses the (already-prefix-stripped) host string and dispatches tocheckIpNotBlocked()checkIpNotBlocked(string $ip)— appliesFILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE
Correctness Analysis
IPv6 bare-colon detection:
if (\strpos($host, ':') !== false && \strpos($host, '[') === false) {
$ipv6 = \explode('/', $host, 2)[0];
if (\filter_var($ipv6, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false) {
$this->checkIpNotBlocked($ipv6);
return;
}
}Handles ::1/some/path correctly. Falls through to parse_url path for non-IPv6 strings that contain : (e.g. host:port). ✅
parse_url reconstruction:
$parsed = \parse_url('http://' . $host);
$hostname = isset($parsed['host']) ? $parsed['host'] : $host;
$hostname = \trim($hostname, '[]');Strips port and path, handles [::1] bracket notation. ✅
FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE: Covers RFC-1918, loopback (127.0.0.0/8), link-local (169.254.0.0/16), and the IPv6 equivalents including ULA (fc00::/7) and link-local (fe80::/10). ✅
localhost-by-name block: preg_match('/^localhost(\..*)?$/i', $hostname) catches localhost, localhost.localdomain. Does not perform DNS resolution — an attacker who controls DNS to resolve a hostname to a private IP would bypass this. This is the accepted limitation at the HTTP client layer (where connect-time validation would be needed for full SSRF prevention). The current fix is a substantial improvement over no validation. ✅
Existing test update: ManagerTest test data updated from http://localhost to https://oc-federation-test.example.com — necessary because the new constructor validation would have thrown on localhost. ✅
Test Coverage
ssrfBlockedHostDataProvider (23 cases):
- IPv4 loopback, link-local, all three RFC-1918 ranges, with and without ports ✅
- IPv6 loopback, bracketed and bare ✅
- IPv6 link-local and ULA ✅
- localhost by name and FQDN variant ✅
- Note: scheme-prefixed variants
http://127.0.0.1/https://169.254.169.254are listed but these have the scheme stripped beforevalidateHost()is called in the constructor — the test instantiatesDAVdirectly, so the stripping happens. ✅
ssrfAllowedHostDataProvider (7 cases): public IPv4, public IPv6, hostnames, ports. ✅
Summary
| Aspect | Assessment |
|---|---|
| SSRF blocked | ✅ Private, loopback, link-local, ULA ranges |
| DNS rebinding | |
| IPv6 parsing | ✅ Both bare and bracketed forms |
| Tests | ✅ 23 blocked + 7 allowed cases |
| Existing tests | ✅ Updated localhost → public hostname |
Verdict: Ready to merge.
Summary
hostwithout any IP range or hostname validationvalidateHost()method blocks loopback, link-local, private IPv4/IPv6 ranges before the value is storedSecurity Impact
High — SSRF allowing internal network reconnaissance and cloud metadata exfiltration when user mounting is enabled
Test plan
testSsrfBlockedHostThrows()— 23 blocked hosts (loopback, link-local, private ranges, IPv6 equivalents) each throw\InvalidArgumentExceptiontestSsrfAllowedHostDoesNotThrow()— 7 legitimate public hostnames pass through unchangedmake test TEST_PHP_SUITE=lib/private/Files/Storage🤖 Generated with Claude Code