fix(files_external): add SSRF host validation to external storage backend#41581
fix(files_external): add SSRF host validation to external storage backend#41581DeepDiver1975 wants to merge 2 commits into
Conversation
…kend UserStoragesController::create()/update() (@NoAdminRequired) accepted arbitrary host values for DAV, SMB, and other network backends without validating against private IP ranges, loopback, or link-local addresses. With user mounting enabled, authenticated users could force the server to make HTTP requests to cloud metadata endpoints (169.254.169.254), localhost services, or internal network hosts. Add validateHostOption() to StoragesController::validate(), which blocks RFC-1918 private ranges, loopback (127.x.x.x / ::1), IPv4 link-local (169.254.x.x), IPv6 link-local (fe80::/10), and ULA (fc00::/7). Admin escape-hatch: files_external_allow_private_address=true in config. 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>
DeepDiver1975
left a comment
There was a problem hiding this comment.
Code Review — fix(files_external): add SSRF host validation to external storage backend configuration
Overview: Adds validateHostOption() to StoragesController that checks the host backend option against private/loopback/link-local ranges before a storage config is saved. Admin opt-out via files_external_allow_private_address config key.
Correctness
The validation logic covers:
- Loopback (
localhost,localhost.localdomain,127.0.0.0/8,::1) ✅ - RFC-1918 private ranges via
FILTER_FLAG_NO_PRIV_RANGE✅ - IPv4 link-local
169.254.0.0/16(separate check since PHP's flag doesn't cover it) ✅ - IPv6 link-local
fe80::/10✅
One gap in IPv6 link-local detection: The check uses:
stripos($ip, 'fe80:') === 0 || stripos($ip, 'fe81:') === 0 || preg_match('/^fe[89ab][0-9a-f]:/i', $ip)fe80::/10 covers fe80:: through febf::. The regex covers fe80–feb9 and feba–febf correctly, but only prefixes like fe80:, fe81:, etc. An address like fe9a:... would be matched by the regex but not the explicit stripos checks — however since the regex is evaluated via || it would still be caught. The logic is correct but redundant. The two stripos checks are subsumed by the regex and could be removed for clarity. Minor, not blocking.
$config = null default parameter in constructor signatures: this allows existing callers without the new parameter to continue working, falling back to \OC::$server->getConfig(). This is a pragmatic backwards-compatibility choice for the controller injection. ✅
Tests
The test suite in StoragesControllerTest is comprehensive:
blockedHostProvidercovers IPv4 loopback, localhost, all RFC-1918 ranges, IPv4 link-local (including the AWS metadata endpoint), port suffixes, scheme-prefixed variants, and path-suffixed addressestestCreateBlockedSsrfHostandtestUpdateBlockedSsrfHostboth assertSTATUS_FORBIDDEN✅testCreateAllowedPublicHostassertsSTATUS_CREATEDforexample.com✅testCreateBlockedSsrfHostAllowedByAdminverifies the opt-out config key allows private addresses through ✅
Summary
| Aspect | Assessment |
|---|---|
| Security fix | ✅ SSRF via host option blocked at controller layer |
| Admin escape hatch | ✅ files_external_allow_private_address opt-out |
| IPv6 link-local | Minor: redundant checks, logic is correct |
| Tests | ✅ Comprehensive with data providers |
Verdict: Ready to merge.
Summary
UserStoragesController(@NoAdminRequired) accepted arbitraryhostvalues and immediately tested the connection, forcing outbound HTTP to any attacker-specified targetvalidateHostOption()inStoragesController::validate()blocking private ranges, loopback, link-local, and IPv6 equivalents; admin can opt out viafiles_external_allow_private_address=trueSecurity Impact
High — SSRF allowing cloud metadata exfiltration and internal network scanning for any authenticated user when user mounting is enabled
Note
This PR touches
StoragesController.php— merge beforesecurity/fix-files-external-info-disclosureto avoid conflicts.Test plan
make test TEST_PHP_SUITE=apps/files_external🤖 Generated with Claude Code