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.

Sign up for GitHub

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

Prevent DNS rebinding attack on admin routes #108

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Prevent DNS rebinding attack on admin routes #108

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions cmd/hetty/hetty.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,17 +221,12 @@ func (cmd *HettyCommand) Exec(ctx context.Context, _ []string) error {
hostname, _ := os.Hostname()
host, _, _ := net.SplitHostPort(req.Host)

// Serve local admin routes when either:
// - The `Host` is well-known, e.g. `hetty.proxy`, `localhost:[port]`
// or the listen addr `[host]:[port]`.
// - The request is not for TLS proxying (e.g. no `CONNECT`) and not
// for proxying an external URL. E.g. Request-Line (RFC 7230, Section 3.1.1)
// 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) ||
Copy link

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.

req.Host == "hetty.proxy" ||
Copy link

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.

Copy link

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 (?).

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)
Copy link

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.

Copy link

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.

Copy link

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);

}).Subrouter().StrictSlash(true)

// GraphQL server.
Expand Down