Skip to content

Prevent DNS rebinding attack on admin routes#108

Open
dstotijn wants to merge 1 commit intomainfrom
fix/dns-rebind
Open

Prevent DNS rebinding attack on admin routes#108
dstotijn wants to merge 1 commit intomainfrom
fix/dns-rebind

Conversation

@dstotijn
Copy link
Owner

@dstotijn dstotijn commented May 22, 2022

With this change, DNS rebinding attacks on the admin routes should no longer be possible, and result in a 502 Bad Gateway response.

To test:

curl -X POST http://localhost:8080/api/graphql/ -H "Host: foobar.com" -H "Content-Type: application/json" -d '{"operationName":"CreateProject","variables":{"name":"Acme"},"query":"mutation CreateProject($name: String!) {\n  createProject(name: $name) {\n    id\n    name\n    __typename\n  }\n}"}' -v

@dstotijn
Copy link
Owner Author

@randomstuff, could you please have a look at this PR?

// Serve local admin routes when the `Host` is well-known, e.g. `[hostname]:[port]`,
// `hetty.proxy`, `localhost:[port]` or the listen addr `[host]:[port]`.
return strings.EqualFold(host, hostname) ||
req.Host == "hetty.proxy" ||

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to say that it would theoretically be possible to use a DNS rebinding attack on http.proxy assuming the attacker is in position of MITM. However, as the port is not included it won't work in normal deployments AFAIU and this might not be a real problem. So I believe this is OK as long as the application is not deployed on a standard port.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me however, whether this is really useful (?).

@randomstuff
Copy link

Look OK to me.

req.Host == fmt.Sprintf("%v:%v", "localhost", listenPort) ||
req.Host == fmt.Sprintf("%v:%v", listenHost, listenPort) ||
req.Method != http.MethodConnect && !strings.HasPrefix(req.RequestURI, "http://")
req.Host == fmt.Sprintf("%v:%v", listenHost, listenPort)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work if listenHost == ""? Alternatively, allowing anything of the form {rawIpv4}, {rawIpv4}:{port}, {rawIpv6}, {rawIpv6}:{port}` should be OK with respect to DNS rebinding.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example if listenHost == "0.0.0.0", the socket will be available to all IP addresses of the host. However, the server will only accept requests using 0.0.0.0:{port} which is what we want in this case.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether something along those lines would be OK:

host, port, splitErr := net.SplitHostPort(req.Host)
// ...
return ... ||
  ParseIP(req.Host) == nil ||
  (splitErr != nil && ParseIP(host) == nil && IsInteger(port);

// has no scheme.
// Serve local admin routes when the `Host` is well-known, e.g. `[hostname]:[port]`,
// `hetty.proxy`, `localhost:[port]` or the listen addr `[host]:[port]`.
return strings.EqualFold(host, hostname) ||

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DNS rebinding through this might theoretically be possible :

  1. when hetty is not bound to localhost;
  2. through a browser on another host which can reach the hetty instance (eg. on the same LAN).

But this might not be the most standard use case and the most compelling attack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants