THU-MAIC / THU-MAIC/OpenMAIC

[RFC] SSRF hardening: redirect re-validation, second-hop protection, IP pinning

Open
#398 4 comments 0 reactions 0 assignees View on GitHub
area:infra priority:P1 status:ready type:rfc
Dominant language
TypeScript
Stars
37.2k
Forks
5.9k
Avg merge
1d 3h
Merged PRs (30d)
195

Description

## Background

PR #386 fixed the primary SSRF DNS rebinding bypass. During follow-up analysis, we identified a few remaining hardening opportunities in the server-side outbound HTTP layer.

## Remaining Items

### 1. Redirect Re-validation (Low-Medium)
Several SSRF-guarded routes follow HTTP redirects by default. A validated initial URL could redirect to an internal address that is never re-checked. Requires an authenticated user to intentionally enter a malicious URL, so practical impact is limited to multi-tenant deployments.

### 2. Second-Hop URL Validation (Low)
Some server-side code fetches URLs returned in provider API responses (e.g. download links for generated media). These second-hop URLs are not validated against SSRF rules. Would require a compromised AI provider to exploit.

### 3. TOCTOU / IP Pinning (Low)
DNS is resolved during validation but re-resolved independently at fetch time, leaving a theoretical race window. Exploitation requires millisecond-precision DNS manipulation — extremely difficult in practice.

### 4. ISATAP Tunnel Coverage (Informational)
A rare IPv6 tunnel format is not currently detected by `isPrivateIP()`. Deprecated protocol, negligible practical risk.

## Proposed Solution

Create a unified `outboundFetch()` server-side HTTP client that:
- Resolves DNS once and pins the validated IP for the actual connection
- Handles redirects manually with per-hop re-validation
- Supports two trust modes: strict (user-supplied URLs) and permissive (server-configured, e.g. Ollama)
- Falls back to alternate resolved IPs on connection failure

Implementation is ready locally and passing all tests.

A lighter alternative: simply add `redirect: 'manual'` to all affected routes without building the full transport layer.

## Proposed PR Roadmap (if full approach is desired)

| PR | Scope | Independent Value |
|----|-------|-------------------|
| **A** | Transport core + AI SDK injection | LLM API calls protected |
| **B** | Direct-fetch route migration | All user-supplied-URL routes protected |
| **C** | Second-hop + adapter layer | Provider-returned URLs protected |

Each PR is independently mergeable.

## Questions for Maintainers

1. **Is this level of hardening desired**, or would the lightweight fix (`redirect: 'manual'` on affected routes) be sufficient?
2. **Is replacing `fetch()` with a unified transport across server routes acceptable?**

## Related
- Original vulnerability: #378
- DNS rebinding fix (merged): #386

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.