Skip to content
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

net/http/httputil: ReverseProxy can remove headers added by Director #50580

Closed
neild opened this issue Jan 12, 2022 · 3 comments
Closed

net/http/httputil: ReverseProxy can remove headers added by Director #50580

neild opened this issue Jan 12, 2022 · 3 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@neild
Copy link
Contributor

neild commented Jan 12, 2022

An HTTP request may contain header fields which are intended for the immediate recipient ("hop-by-hop"), and ones intended for all recipients on the chain of a forwarded request ("end-to-end"). For example, the "Keep-Alive" header is a hop-by-hop header, because it applies to a single TCP connection. The "Cache-Control" header, in contrast, is an end-to-end header which should be forwarded by an HTTP proxy.

RFC 2616, section 13.5.1 specified a list of hop-by-hop headers which HTTP proxies should not forward.

RFC 7230, section 6.1 replaces the hardcoded list of hop-by-hop headers with the ability for the originator of a request to specify the hop-by-hop headers in the "Connection" header.

The httputil.ReverseProxy proxy both understands the obsolete hardcoded list of hop-by-hop headers from RFC 2616 and the Connection header from RFC 7230. ReverseProxy will remove all hop-by-hop headers before forwarding a request.

When ReverseProxy forwards a request, it follows the following steps in order:

  1. Clone the incoming request to produce an outbound request, outreq.
  2. Pass outreq to the Director function, which may modify it.
  3. Remove hop-by-hop headers from outreq.
  4. Send outreq to the backend.

This can cause headers added by the Director function to be removed before the request is forwarded to the backend.

For example, if an inbound request contains a Connection: forwarded header, then any Forwarded header added by the Director will not be sent to the backend. This is probably surprising; under some circumstances, it may be a security vulnerability.

The user of httputil.ReverseProxy can prevent a header from being stripped in this fashion by removing any headers that should not be dropped from the Connection header. This is cumbersome, and relies on the user knowing that they must do this.

Some proposed solutions:

  • Remove hop-by-hop headers before calling the Director function: This breaks any existing Director function that examines hop-by-hop headers like Proxy-Authorization.

  • Detect what headers were modified by the Director function and avoid removing them: This does not avoid the problem, because the attacker could send the exact header they want to remove. For example, an attacker can send

    Connection: x-forwarded-proto
    X-Forwarded-Proto: https
    

    and after Director runs we cannot tell whether it intends the X-Forwarded-Proto header to stay.

  • Only remove RFC 2616 hop-by-hop headers and do not remove headers listed in the Connection header: This is a violation of RFC 7230, which states that proxies MUST remove headers listed in the Connection header.

I do not believe it is possible to fix this problem within the context of the existing ReverseProxy API. The problem is that the Director function makes no distinction between the inbound request and the outbound one: We cannot remove hop-by-hop headers before passing a request to Director, since Director may need to examine those headers, but removing those headers after Director runs can strip headers that should be preserved for the next hop.

I propose that we add a new field to ReverseProxy, superseding Director, which takes both the inbound and outbound request.

type ReverseProxy struct {
  // ModifyRequest is an optional function that modifies the Request
  // sent using Transport. The inreq is the unmodified inbound request,
  // including including hop-by-hop headers. The outreq is the request
  // to be sent, with hop-by-hop headers removed and the `X-Forwarded-For`
  // header added.
  //
  // If ModifyRequest returns an error, ErrorHandler is called
  // with its error value. If ErrorHandler is nil, its default
  // implementation is used.
  ModifyRequest func(inreq, outreq *http.Request) error
}

We do not usually backport new APIs. Since in this case the existing API is inherently insecure, I propose that we backport ModifyRequest and mark Director as deprecated.

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 13, 2022
@neild
Copy link
Contributor Author

neild commented Jan 14, 2022

We would need to define how ModifyRequest and Director compose: What happens if both are set? My inclination is to call this an error.

@bradfitz proposed an interesting alternative to ModifyRequest. If we pass a type to the rewrite function, we can add additional methods to that type to make it easier for users to customize the proxy behavior. For example:

proxy := &ReverseProxy{
  Rewrite: func(r *Rewriter) {
    r.SetXForwarded()    // sets X-Forwarded-{For,Host,Proto}
    r.AppendXForwarded() // appends to X-Forwarded-For (current behavior)
    r.SetForwarded()     // sets RFC 7239 Forwarded header
    r.SetURL(url)        // NewSingleHostReverseProxy-style routing
  },
}

@gopherbot
Copy link

Change https://go.dev/cl/407214 mentions this issue: net/http/httputil: add ReverseProxy.Rewrite

@neild neild added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 22, 2022
@dmitshur dmitshur added this to the Go1.20 milestone Aug 16, 2022
@zepatrik
Copy link

zepatrik commented May 5, 2023

May I ask why this has neither a CVE nor a deprecation warning on the director hook?
We had to issue a security advisory because of this: GHSA-w9mr-28mw-j8hg
image

There is also a ton of guides out there describing the director hook, so people will keep using it unless a deprecation warning is added (the best possible action IMO).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants