Răsfoiți Sursa

fix(fetcher): avoid possible SSRF TOCTOU/DNS-rebinding in private network check

Move the private-network IP check from a pre-flight DNS lookup into
net.Dialer.Control, which runs after DNS resolution but before the TCP
connection is established. This ensures the validated IP is the one
actually connected to, closing the TOCTOU window that allowed DNS
rebinding attacks.

As a side effect, the check now also applies to redirect targets,
which the previous pre-flight approach did not cover.
Frédéric Guillot 1 lună în urmă
părinte
comite
fe2bfb27cf

+ 30 - 19
internal/reader/fetcher/request_builder.go

@@ -13,6 +13,7 @@ import (
 	"net/http"
 	"net/url"
 	"slices"
+	"syscall"
 	"time"
 
 	"miniflux.app/v2/internal/config"
@@ -138,16 +139,39 @@ func (r *RequestBuilder) WithoutCompression() *RequestBuilder {
 }
 
 func (r *RequestBuilder) ExecuteRequest(requestURL string) (*http.Response, error) {
+	dialer := &net.Dialer{
+		Timeout:   10 * time.Second, // Default is 30s.
+		KeepAlive: 15 * time.Second, // Default is 30s.
+	}
+
+	// Perform the private-network check inside the dialer's Control callback,
+	// which fires after DNS resolution but before the TCP connection is made.
+	// This eliminates TOCTOU / DNS-rebinding vulnerabilities: the resolved IP
+	// that is checked is exactly the IP that will be connected to.
+	allowPrivateNetworks := config.Opts == nil || config.Opts.FetcherAllowPrivateNetworks()
+	if !allowPrivateNetworks {
+		dialer.Control = func(network, address string, c syscall.RawConn) error {
+			host, _, err := net.SplitHostPort(address)
+			if err != nil {
+				return err
+			}
+
+			ip := net.ParseIP(host)
+			if urllib.IsNonPublicIP(ip) {
+				return fmt.Errorf("%w %q", ErrPrivateNetworkHost, host)
+			}
+
+			return nil
+		}
+	}
+
 	transport := &http.Transport{
 		Proxy: http.ProxyFromEnvironment,
 		// Setting `DialContext` disables HTTP/2, this option forces the transport to try HTTP/2 regardless.
 		ForceAttemptHTTP2: true,
-		DialContext: (&net.Dialer{
-			Timeout:   10 * time.Second, // Default is 30s.
-			KeepAlive: 15 * time.Second, // Default is 30s.
-		}).DialContext,
-		MaxIdleConns:    50,               // Default is 100.
-		IdleConnTimeout: 10 * time.Second, // Default is 90s.
+		DialContext:       dialer.DialContext,
+		MaxIdleConns:      50,               // Default is 100.
+		IdleConnTimeout:   10 * time.Second, // Default is 90s.
 	}
 
 	if r.ignoreTLSErrors {
@@ -209,19 +233,6 @@ func (r *RequestBuilder) ExecuteRequest(requestURL string) (*http.Response, erro
 		return nil, err
 	}
 
-	allowPrivateNetworks := config.Opts == nil || config.Opts.FetcherAllowPrivateNetworks()
-	if !allowPrivateNetworks {
-		hostname := req.URL.Hostname()
-		isPrivate, err := urllib.ResolvesToPrivateIP(hostname)
-		if err != nil {
-			return nil, fmt.Errorf("%w %q: %w", ErrHostnameResolution, hostname, err)
-		}
-
-		if isPrivate {
-			return nil, fmt.Errorf("%w %q", ErrPrivateNetworkHost, hostname)
-		}
-	}
-
 	req.Header = r.headers
 	if r.disableCompression {
 		req.Header.Set("Accept-Encoding", "identity")

+ 28 - 0
internal/reader/fetcher/request_builder_test.go

@@ -436,6 +436,34 @@ func TestRequestBuilder_AllowPrivateNetworkWhenEnabled(t *testing.T) {
 	defer resp.Body.Close()
 }
 
+func TestRequestBuilder_RefusePrivateNetworkOnRedirect(t *testing.T) {
+	configureFetcherAllowPrivateNetworksOption(t, "0")
+
+	// Target server on a loopback address (private).
+	privateServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer privateServer.Close()
+
+	// Redirector that sends the client to the private server.
+	// Because the Control callback checks the IP at connection time, the
+	// redirect target is also validated (unlike a pre-flight DNS check).
+	redirectServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, privateServer.URL, http.StatusFound)
+	}))
+	defer redirectServer.Close()
+
+	builder := NewRequestBuilder()
+	_, err := builder.ExecuteRequest(redirectServer.URL)
+	if err == nil {
+		t.Fatal("Expected redirect to private network to be rejected")
+	}
+
+	if !strings.Contains(err.Error(), "refusing to access private network host") {
+		t.Fatalf("Unexpected error for redirected private network request: %v", err)
+	}
+}
+
 func TestRequestBuilder_TimeoutConfiguration(t *testing.T) {
 	// Create a slow server
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

+ 0 - 15
internal/urllib/url.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"net"
 	"net/url"
-	"slices"
 	"strings"
 )
 
@@ -155,20 +154,6 @@ func JoinBaseURLAndPath(baseURL, path string) (string, error) {
 	return finalURL, nil
 }
 
-// ResolvesToPrivateIP resolves a hostname and reports whether any resolved IP address is non-public.
-func ResolvesToPrivateIP(host string) (bool, error) {
-	ips, err := net.LookupIP(host)
-	if err != nil {
-		return false, err
-	}
-
-	if slices.ContainsFunc(ips, IsNonPublicIP) {
-		return true, nil
-	}
-
-	return false, nil
-}
-
 // IsNonPublicIP returns true if the given IP is private, loopback,
 // link-local, multicast, or unspecified.
 func IsNonPublicIP(ip net.IP) bool {

+ 0 - 34
internal/urllib/url_test.go

@@ -252,37 +252,3 @@ func TestIsNonPublicIP(t *testing.T) {
 		})
 	}
 }
-
-func TestResolvesToPrivateIP(t *testing.T) {
-	testCases := []struct {
-		name string
-		host string
-		want bool
-	}{
-		{"localhost", "localhost", true},
-		{"example.org", "example.org", false},
-		{"loopback IPv4 literal", "127.0.0.1", true},
-		{"loopback IPv6 literal", "::1", true},
-		{"private IPv4 literal", "192.168.1.1", true},
-		{"public IPv4 literal", "93.184.216.34", false},
-		{"public IPv6 literal", "2001:4860:4860::8888", false},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			got, err := ResolvesToPrivateIP(tc.host)
-			if err != nil {
-				t.Fatalf("unexpected error for %s: %v", tc.host, err)
-			}
-			if got != tc.want {
-				t.Fatalf("unexpected result for %s: got %v want %v", tc.name, got, tc.want)
-			}
-		})
-	}
-}
-
-func TestResolvesToPrivateIPError(t *testing.T) {
-	if _, err := ResolvesToPrivateIP(""); err == nil {
-		t.Fatalf("expected an error for empty host")
-	}
-}