-
-
Notifications
You must be signed in to change notification settings - Fork 170
fix: prevent SSRF bypass via 302 redirect, DNS rebinding and IPv4-mapped IPv6 CGNAT evasion #305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -272,10 +272,16 @@ func fillDefaultArgs(tool *config.ToolConfig, args map[string]any) { | |
| } | ||
| } | ||
|
|
||
| // createHTTPClient creates an HTTP client with proxy support if configured | ||
| func createHTTPClient(tool *config.ToolConfig) (*http.Client, error) { | ||
| // createHTTPClient creates an HTTP client with proxy support if configured. | ||
| // It installs a CheckRedirect hook that re-validates every redirect target | ||
| // against the internal-network allowlist (preventing 302-based SSRF), and | ||
| // pins the initial connection to pinnedAddr (when non-empty) to prevent | ||
| // DNS-rebinding attacks. | ||
| func (s *Server) createHTTPClient(tool *config.ToolConfig, pinnedAddr string) (*http.Client, error) { | ||
| var baseTransport *http.Transport | ||
|
|
||
| if tool != nil && tool.Proxy != nil { | ||
| transport := &http.Transport{} | ||
| baseTransport = &http.Transport{} | ||
|
|
||
| switch tool.Proxy.Type { | ||
| case "http", "https": | ||
|
|
@@ -284,22 +290,47 @@ func createHTTPClient(tool *config.ToolConfig) (*http.Client, error) { | |
| if err != nil { | ||
| return nil, fmt.Errorf("invalid %s proxy configuration: %w", tool.Proxy.Type, err) | ||
| } | ||
| transport.Proxy = http.ProxyURL(proxyURL) | ||
| baseTransport.Proxy = http.ProxyURL(proxyURL) | ||
|
|
||
| case "socks5": | ||
| dialer, err := proxy.SOCKS5("tcp", fmt.Sprintf("%s:%d", tool.Proxy.Host, tool.Proxy.Port), nil, proxy.Direct) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create SOCKS5 dialer: %w", err) | ||
| } | ||
| transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| baseTransport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| return dialer.Dial(network, addr) | ||
| } | ||
| } | ||
| } else { | ||
| baseTransport = http.DefaultTransport.(*http.Transport).Clone() | ||
| } | ||
|
|
||
| return &http.Client{Transport: otelhttp.NewTransport(transport)}, nil | ||
| // Pin DNS: when validateToolEndpoint resolved a hostname to an IP, we | ||
| // force the transport to connect to that IP instead of re-resolving. | ||
| // This closes the DNS-rebinding TOCTOU window. | ||
| if pinnedAddr != "" && baseTransport.DialContext == nil { | ||
| stdDialer := &net.Dialer{} | ||
| baseTransport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
|
Comment on lines
+311
to
+313
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚨 issue (security): DNS pinning is skipped for the default transport because DialContext is already non-nil there. Because |
||
| _, port, err := net.SplitHostPort(addr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return stdDialer.DialContext(ctx, network, net.JoinHostPort(pinnedAddr, port)) | ||
| } | ||
| } | ||
|
|
||
| return &http.Client{Transport: otelhttp.NewTransport(http.DefaultTransport)}, nil | ||
| return &http.Client{ | ||
| Transport: otelhttp.NewTransport(baseTransport), | ||
| CheckRedirect: func(req *http.Request, via []*http.Request) error { | ||
| if len(via) >= 10 { | ||
| return fmt.Errorf("stopped after 10 redirects") | ||
| } | ||
| if _, err := s.validateToolEndpoint(req.Context(), req.URL); err != nil { | ||
| return fmt.Errorf("redirect to %s blocked: %w", req.URL.Redacted(), err) | ||
| } | ||
| return nil | ||
| }, | ||
| }, nil | ||
| } | ||
|
|
||
| // executeHTTPTool executes a tool with the given arguments | ||
|
|
@@ -355,7 +386,8 @@ func (s *Server) executeHTTPTool(c *gin.Context, conn session.Connection, tool * | |
| return nil, err | ||
| } | ||
|
|
||
| if err := s.validateToolEndpoint(ctx, req.URL); err != nil { | ||
| pinnedAddr, err := s.validateToolEndpoint(ctx, req.URL) | ||
| if err != nil { | ||
| logger.Warn("blocked tool endpoint", | ||
| zap.String("tool", tool.Name), | ||
| zap.String("session_id", conn.Meta().ID), | ||
|
|
@@ -403,7 +435,7 @@ func (s *Server) executeHTTPTool(c *gin.Context, conn session.Connection, tool * | |
| processArguments(req, tool, args) | ||
|
|
||
| // Execute request | ||
| cli, err := createHTTPClient(tool) | ||
| cli, err := s.createHTTPClient(tool, pinnedAddr) | ||
| if err != nil { | ||
| logger.Error("failed to create HTTP client", | ||
| zap.String("tool", tool.Name), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (testing): Strengthen
TestCreateHTTPClient_PinnedAddrto actually verify that the pinned address is usedThe test comment promises to verify that the client actually connects to
pinnedAddr, but the current assertions only check for a 200 response. Becausehttptest.NewServeralready binds to127.0.0.1, the test would still pass even if the pinned address were ignored.To better match the stated intent, you could either:
net.Dialer(or wrapper aroundDialContext) that records the dialedaddrand assert the host matchespinnedAddr, orpinnedAddrworks, and add a more targeted unit test around the dial function to validate the pinning behavior.That way, the test either truly enforces DNS pinning or accurately documents its limited scope.