x509util: block private/loopback hosts in URL-fetching helpers#1761
x509util: block private/loopback hosts in URL-fetching helpers#17611seal wants to merge 2 commits intogoogle:masterfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
1seal
force-pushed
the
fix/ssrf-url-validation-x509util
branch
from
9b95c1d to
b06af35
Compare
|
This PR makes the claim that I don't see these calls happening in anything other than standalone binaries. Crucially, this is not happening on the serving paths for CT. Given that the main users of these checks are standalone tools that may be used by testing piplines etc, I think there is a real risk that changing this behaviour breaks someone's workflow. And without a clear information leak risk. Have I missed something? |
|
@mhutchinson thanks for the careful review. you’re right that these callsites are primarily in standalone tools (e.g. ctutil/* and x509util/*check), not the ctfe serving paths, and i’m not claiming impact on the main ct serving components. for clarity: in this repo ReadFileOrURL is generally fed from flags/config (e.g. log list path), while the certificate-extension-derived inputs are GetIssuer (AIA) and ReadPossiblePEMURL (CDP). the reason i still think this is worth hardening is that these tools are commonly run in ci / monitoring pipelines on certificates from untrusted sources (ct log submissions and arbitrary tls endpoints). in that setting, AIA/CDP values are attacker-controlled and the process can end up issuing outbound HTTP requests from a privileged network location (ssrf capability), even if the response isn’t intentionally “returned to the attacker”. concrete callsites in this repo: sctcheck.go (line 131) calls x509util.GetIssuer when the issuer is missing from the chain (“attempting online retrieval”) |
|
Can you rebase this? I'm not convinced that this fixes a real security issue given my understanding of how these tools are used. That said, given #1775 has come in on the same issue, it's clear that automated scanning will keep bringing this up. In the interests of lowering maintenance burden, I think it's worth accepting this given the low expected downsides. Thanks. |
PR google#1761 introduced rejectPrivateHost() to block SSRF via private IP addresses, but only checked net.ParseIP() which returns nil for hostnames. DNS hostnames resolving to private or loopback addresses bypassed the fix entirely. This change resolves hostnames via net.LookupHost() before IP validation, blocking bypasses such as: - localhost → 127.0.0.1 (loopback) - metadata.google.internal → 169.254.169.254 (GCP) - kubernetes.default.svc → 10.x.x.x (k8s) Also adds: - rejectPrivateHost() to ReadFileOrURL() and GetIssuer() - defer rsp.Body.Close() to prevent resource leaks - Tests for hostname bypass and SSRF protection Fixes incomplete fix in PR google#1761 (Issue google#1759)
PR google#1761 introduced rejectPrivateHost() to block SSRF via private IP addresses, but only checked net.ParseIP() which returns nil for hostnames. DNS hostnames resolving to private or loopback addresses bypassed the fix entirely. This change resolves hostnames via net.LookupHost() before IP validation, blocking bypasses such as: - localhost → 127.0.0.1 (loopback) - metadata.google.internal → 169.254.169.254 (GCP) - kubernetes.default.svc → 10.x.x.x (k8s) Also adds: - rejectPrivateHost() to ReadFileOrURL() and GetIssuer() - defer rsp.Body.Close() to prevent resource leaks - Tests for hostname bypass and SSRF protection Fixes incomplete fix in PR google#1761 (Issue google#1759)
|
Would this fix address HTTP redirects from a hostname to a private/loopback IP? What about hostnames with an A/AAAA record to a private/loopback IP address? |
GetIssuer, ReadFileOrURL, and ReadPossiblePEMURL fetch URLs that may originate from attacker-controlled certificate fields (AIA, CDP). Add private/loopback IP rejection and close response bodies on all paths to prevent SSRF and resource leaks.
rejectPrivateHost only checked literal IP addresses in the URL. Two bypasses remained: 1. A hostname resolving to a private/loopback IP (e.g. evil.com → 127.0.0.1) was not caught because net.ParseIP returns nil for hostnames. 2. An HTTP redirect from a public host to a private IP was followed by the default http.Client without re-checking the target. Fix: introduce safeTransport (custom DialContext that resolves and validates all IPs before connecting) and rejectPrivateRedirect (CheckRedirect hook that blocks redirects to private literal IPs). All three URL-fetching helpers (ReadPossiblePEMURL, ReadFileOrURL, GetIssuer) now use safeClient which combines both guards. Add tests for redirect-to-loopback and localhost-hostname scenarios.
1seal
force-pushed
the
fix/ssrf-url-validation-x509util
branch
from
07aeecd to
13209c4
Compare
|
@jub0bs good catch — the initial version only checked literal IPs in the URL, so both scenarios you described were indeed bypasses:
just pushed a fix (rebased as well per @mhutchinson's request). the approach:
all three URL-fetching helpers ( |
|
@mhutchinson If I were you, I'd be wary of self-described "LLM-augmented" contributions. |
3 participants
Summary
GetIssuer,ReadFileOrURL, andReadPossiblePEMURLaccept URLs that may originate from certificate extension fields (AIA, CDP). These fields are controlled by whoever created the certificate.rejectPrivateHostcheck that blocks loopback, link-local, and private IP addresses before issuing HTTP requests, preventing SSRF to internal endpoints.defer rsp.Body.Close()inReadPossiblePEMURLandReadFileOrURLto prevent resource leaks.Test plan
go test ./x509util/ -v— all existing tests pass, newTestRejectPrivateHostcovers loopback (v4/v6), link-local, private ranges (10/172/192), AWS IMDS, and public hostsctutil/sctcheck,x509util/crlcheck)