From 449cd1e2fee773ce7be22ee8811767b3a5b08556 Mon Sep 17 00:00:00 2001 From: bolkedebruin Date: Thu, 30 Apr 2026 18:42:24 +0200 Subject: [PATCH] Gate hostselection=any to public destinations and a port allow-list (#188) The `roundrobin`, `signed`, and `unsigned` host-selection modes route requests against an operator-curated `Server.Hosts` list. The `any` mode does not -- it forwards to whatever `?host=` value the request carries, which makes the gateway usable as a generic TCP relay against whatever the gateway can reach (loopback, RFC1918, link-local, the cloud metadata service, arbitrary high-numbered ports on public hosts). Add a small destination policy applied only in `any` mode: * Reject hosts that resolve to loopback, RFC1918, IPv6 ULA, link-local, unspecified, or multicast addresses. Operators can opt back in with `Server.AllowPrivateDestinations: true`. * Restrict the destination port to `Server.AllowedDestinationPorts` (default {3389}). The other host-selection modes are unaffected -- the operator already curates their hosts list. The DestinationPolicy zero value is the secure default, so direct &Handler{} constructions in tests still get the expected behavior. DNS names are resolved at validation time and every returned address is checked. --- CHANGELOG.md | 24 +++++++ README.md | 12 +++- UPGRADING.md | 32 ++++++++++ cmd/rdpgw/config/configuration.go | 8 +++ cmd/rdpgw/main.go | 10 +-- cmd/rdpgw/web/web.go | 99 +++++++++++++++++++++++++++++ cmd/rdpgw/web/web_interface_test.go | 9 ++- cmd/rdpgw/web/web_test.go | 76 +++++++++++++++++++++- 8 files changed, 260 insertions(+), 10 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..c66df0b --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,24 @@ +# Changelog + +All user-visible changes to rdpgw will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), +and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [Unreleased] + +### Changed + +- `server.hostselection: any` now refuses destinations that resolve to + loopback, RFC1918, link-local, IPv6 ULA, unspecified, or multicast + addresses, and only forwards to ports in `Server.AllowedDestinationPorts` + (default `[3389]`). Operators that need the old behavior can opt back in + with `Server.AllowPrivateDestinations: true` and an extended port list. + See [UPGRADING.md](UPGRADING.md) for migration notes. The other + host-selection modes (`roundrobin`, `signed`, `unsigned`) already used + the operator-curated `Server.Hosts` list and are unaffected. + +### Added + +- `Server.AllowedDestinationPorts` (`[]int`, default `[3389]`). +- `Server.AllowPrivateDestinations` (`bool`, default `false`). diff --git a/README.md b/README.md index 47a7e4c..64c1f09 100644 --- a/README.md +++ b/README.md @@ -166,8 +166,18 @@ Server: # - roundrobin, which selects a random host from the list (default) # - signed, a listed host specified in the signed query parameter # - unsigned, a listed host specified in the query parameter - # - any, insecurely allow any host specified in the query parameter + # - any, allow any host specified in the query parameter, gated by the + # AllowedDestinationPorts / AllowPrivateDestinations options below. HostSelection: roundrobin + # When HostSelection: any, only these TCP ports may be forwarded to. + # Empty defaults to [3389]. Ignored for the curated host modes + # (roundrobin, signed, unsigned). + # AllowedDestinationPorts: + # - 3389 + # When HostSelection: any, refuse to forward to loopback / RFC1918 / + # link-local / IPv6 ULA / unspecified / multicast destinations unless + # this is true. Default false. Has no effect on the curated host modes. + # AllowPrivateDestinations: false # a random strings of at least 32 characters to secure cookies on the client # make sure to share this across the different pods SessionKey: thisisasessionkeyreplacethisjetzt diff --git a/UPGRADING.md b/UPGRADING.md index 74d7f4a..50989c1 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,3 +1,35 @@ +# Upgrading + +## Unreleased + +### `hostselection: any` now refuses non-routable destinations and non-RDP ports by default + +Previously, when `server.hostselection: any` was set, rdpgw forwarded +to whatever `?host=` value the request carried with no check on the +target. The gateway would happily relay TCP traffic to loopback, +RFC1918, link-local, or arbitrary high-numbered ports on public hosts. + +After upgrading, `any` mode rejects any destination that resolves to a +loopback / RFC1918 / link-local / IPv6 ULA / unspecified / multicast +address, and any port that is not in `AllowedDestinationPorts`. The +default port allow-list is `[3389]`. + +If your deployment legitimately reaches private destinations or extra +ports through `any` mode, opt back in: + +```yaml +Server: + HostSelection: any + AllowedDestinationPorts: + - 3389 + - 5985 # add what you actually need + AllowPrivateDestinations: true +``` + +The other host-selection modes (`roundrobin`, `signed`, `unsigned`) +already use the operator-curated `Server.Hosts` allow-list and are +unaffected by this change. + # Upgrading from 1.X to 2.0 In 2.0 the options for configuring client side RDP settings have been removed in favor of template file. diff --git a/cmd/rdpgw/config/configuration.go b/cmd/rdpgw/config/configuration.go index 0bff08f..ed2831b 100644 --- a/cmd/rdpgw/config/configuration.go +++ b/cmd/rdpgw/config/configuration.go @@ -56,6 +56,14 @@ type ServerConfig struct { Authentication []string `koanf:"authentication"` AuthSocket string `koanf:"authsocket"` BasicAuthTimeout int `koanf:"basicauthtimeout"` + // AllowedDestinationPorts gates the TCP ports `hostselection: any` may + // forward to. Empty defaults to {3389}. Ignored for the curated host + // modes (roundrobin, signed, unsigned). + AllowedDestinationPorts []int `koanf:"alloweddestinationports"` + // AllowPrivateDestinations, when true, lets `hostselection: any` + // forward to loopback, RFC1918, link-local, and IPv6 ULA targets. + // Default false. + AllowPrivateDestinations bool `koanf:"allowprivatedestinations"` } type KerberosConfig struct { diff --git a/cmd/rdpgw/main.go b/cmd/rdpgw/main.go index 023f794..598ea98 100644 --- a/cmd/rdpgw/main.go +++ b/cmd/rdpgw/main.go @@ -114,10 +114,12 @@ func main() { NoUsername: conf.Client.NoUsername, OverridableRdpKeys: conf.Client.RdpOverridableKeys, }, - GatewayAddress: url, - TemplateFile: conf.Client.Defaults, - RdpSigningCert: conf.Client.SigningCert, - RdpSigningKey: conf.Client.SigningKey, + GatewayAddress: url, + TemplateFile: conf.Client.Defaults, + RdpSigningCert: conf.Client.SigningCert, + RdpSigningKey: conf.Client.SigningKey, + AllowedDestinationPorts: conf.Server.AllowedDestinationPorts, + AllowPrivateDestinations: conf.Server.AllowPrivateDestinations, } if conf.Caps.TokenAuth { diff --git a/cmd/rdpgw/web/web.go b/cmd/rdpgw/web/web.go index 290b290..9449cf8 100644 --- a/cmd/rdpgw/web/web.go +++ b/cmd/rdpgw/web/web.go @@ -12,10 +12,12 @@ import ( "html/template" "log" rnd "math/rand" + "net" "net/http" "net/url" "os" "path/filepath" + "strconv" "strings" "time" @@ -42,6 +44,16 @@ type Config struct { RdpSigningCert string RdpSigningKey string TemplatesPath string + // AllowedDestinationPorts gates which TCP ports the `any` host-selection + // mode may forward to. Empty defaults to {3389}. Ignored for + // roundrobin / signed / unsigned where the operator already curates the + // hosts list. + AllowedDestinationPorts []int + // AllowPrivateDestinations, when true, lets `any` mode forward to + // loopback / RFC1918 / link-local / IPv6 ULA destinations. Default + // false: only globally-routable addresses are accepted. Operators that + // genuinely need to reach private destinations from `any` must opt in. + AllowPrivateDestinations bool } // WebConfig represents the web interface configuration @@ -96,6 +108,88 @@ type Handler struct { templatesPath string webConfig *WebConfig htmlTemplate *template.Template + destPolicy destinationPolicy +} + +// destinationPolicy gates the host strings accepted in `any` host-selection +// mode. With `signed` / `unsigned` / `roundrobin` the operator curates the +// hosts list; with `any` the value comes from the request, so the gateway +// must ensure it isn't being asked to act as a TCP relay against an +// internal-only target. +type destinationPolicy struct { + allowedPorts map[int]struct{} + allowPrivateDestinations bool +} + +func newDestinationPolicy(allowedPorts []int, allowPrivate bool) destinationPolicy { + if len(allowedPorts) == 0 { + allowedPorts = []int{3389} + } + set := make(map[int]struct{}, len(allowedPorts)) + for _, p := range allowedPorts { + set[p] = struct{}{} + } + return destinationPolicy{ + allowedPorts: set, + allowPrivateDestinations: allowPrivate, + } +} + +// allow validates a host:port (or bare host) string against the policy. +// Returns nil if the destination is acceptable, an error otherwise. The +// zero-value policy is treated as the secure default ({3389}, public-only). +func (p destinationPolicy) allow(hostport string) error { + host, port, err := net.SplitHostPort(hostport) + if err != nil { + // no port present -- assume the protocol default + host = hostport + port = "3389" + } + portNum, err := strconv.Atoi(port) + if err != nil { + return fmt.Errorf("invalid port %q in %q", port, hostport) + } + allowedPorts := p.allowedPorts + if len(allowedPorts) == 0 { + allowedPorts = map[int]struct{}{3389: {}} + } + if _, ok := allowedPorts[portNum]; !ok { + return fmt.Errorf("port %d not in allow-list", portNum) + } + + if p.allowPrivateDestinations { + return nil + } + + if ip := net.ParseIP(host); ip != nil { + return checkPublicIP(host, ip) + } + addrs, err := net.LookupIP(host) + if err != nil { + return fmt.Errorf("cannot resolve %q: %s", host, err) + } + for _, ip := range addrs { + if err := checkPublicIP(host, ip); err != nil { + return err + } + } + return nil +} + +func checkPublicIP(host string, ip net.IP) error { + switch { + case ip.IsLoopback(): + return fmt.Errorf("destination %q (%s) is loopback", host, ip) + case ip.IsPrivate(): + return fmt.Errorf("destination %q (%s) is in a private range", host, ip) + case ip.IsLinkLocalUnicast(): + return fmt.Errorf("destination %q (%s) is link-local", host, ip) + case ip.IsUnspecified(): + return fmt.Errorf("destination %q (%s) is unspecified", host, ip) + case ip.IsMulticast(): + return fmt.Errorf("destination %q (%s) is multicast", host, ip) + } + return nil } func (c *Config) NewHandler() *Handler { @@ -115,6 +209,7 @@ func (c *Config) NewHandler() *Handler { rdpOpts: c.RdpOpts, rdpDefaults: c.TemplateFile, templatesPath: c.TemplatesPath, + destPolicy: newDestinationPolicy(c.AllowedDestinationPorts, c.AllowPrivateDestinations), } // set up RDP signer if config values are set @@ -401,6 +496,10 @@ func (h *Handler) getHost(ctx context.Context, u *url.URL) (string, error) { if !ok { return "", errors.New("invalid query parameter") } + if err := h.destPolicy.allow(hosts[0]); err != nil { + log.Printf("rejecting `any` destination %q: %s", hosts[0], err) + return "", fmt.Errorf("destination not allowed: %s", err) + } return hosts[0], nil default: return h.selectRandomHost(), nil diff --git a/cmd/rdpgw/web/web_interface_test.go b/cmd/rdpgw/web/web_interface_test.go index 6d007e7..9ca3564 100644 --- a/cmd/rdpgw/web/web_interface_test.go +++ b/cmd/rdpgw/web/web_interface_test.go @@ -341,9 +341,12 @@ func TestHostSelectionIntegration(t *testing.T) { name: "any host allowed", hostSelection: "any", hosts: []string{"host1.com"}, - queryParams: "?host=any-host.com", - expectHost: "any-host.com", - expectError: false, + // TEST-NET-3 literal stays in the policy's "publicly + // routable + RDP port" branch, which is the only thing + // `any` mode accepts by default. + queryParams: "?host=203.0.113.5:3389", + expectHost: "203.0.113.5:3389", + expectError: false, }, } diff --git a/cmd/rdpgw/web/web_test.go b/cmd/rdpgw/web/web_test.go index 2e29394..1e1ecbb 100644 --- a/cmd/rdpgw/web/web_test.go +++ b/cmd/rdpgw/web/web_test.go @@ -84,9 +84,10 @@ func TestGetHost(t *testing.T) { t.Fatalf("host %s is not equal to input %s", host, hosts[0]) } - // check any + // check any -- TEST-NET-3 literal stays in the policy's "publicly + // routable" branch so this case still exercises the happy path. c.HostSelection = "any" - test := "bla.bla.com" + test := "203.0.113.5:3389" vals.Set("host", test) u.RawQuery = vals.Encode() h = c.NewHandler() @@ -119,6 +120,77 @@ func TestGetHost(t *testing.T) { } } +// TestGetHostAnyRejectsSensitiveDestinations asserts that with +// HostSelection="any" the gateway refuses hosts that resolve to addresses +// it should not be reachable as: loopback, RFC1918, link-local, the cloud +// metadata service, IPv6 loopback / ULA. Without this check an +// authenticated user can use the gateway as a generic TCP relay against +// any internal target the gateway can reach. +func TestGetHostAnyRejectsSensitiveDestinations(t *testing.T) { + cases := []struct { + name string + host string + }{ + {"loopback v4", "127.0.0.1:3389"}, + {"loopback name", "localhost:3389"}, + {"cloud metadata", "169.254.169.254:80"}, + {"rfc1918 10/8", "10.0.0.5:3389"}, + {"rfc1918 192.168/16", "192.168.1.10:3389"}, + {"rfc1918 172.16/12", "172.16.5.10:3389"}, + {"ipv6 loopback", "[::1]:3389"}, + {"ipv6 ula", "[fc00::1]:3389"}, + {"non-rdp port on public host", "203.0.113.5:6379"}, + } + + c := Config{ + HostSelection: "any", + Hosts: hosts, + } + h := c.NewHandler() + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + u := &url.URL{Host: "example.com"} + vals := u.Query() + vals.Set("host", tc.host) + u.RawQuery = vals.Encode() + + got, err := h.getHost(context.Background(), u) + if err == nil { + t.Errorf("getHost(%q) returned %q with no error; sensitive destinations must be refused in 'any' mode", tc.host, got) + } + }) + } +} + +// TestGetHostAnyAllowsExplicitOptIn confirms that an operator can re-enable +// access to private destinations and additional ports for `any` mode when +// the deployment legitimately needs it. +func TestGetHostAnyAllowsExplicitOptIn(t *testing.T) { + c := Config{ + HostSelection: "any", + Hosts: hosts, + AllowedDestinationPorts: []int{3389, 5985}, + AllowPrivateDestinations: true, + } + h := c.NewHandler() + + for _, target := range []string{"10.0.0.1:3389", "127.0.0.1:5985"} { + u := &url.URL{Host: "example.com"} + vals := u.Query() + vals.Set("host", target) + u.RawQuery = vals.Encode() + + got, err := h.getHost(context.Background(), u) + if err != nil { + t.Errorf("getHost(%q) rejected with %v; explicit opt-in must allow private and extra-port destinations", target, err) + } + if got != target { + t.Errorf("getHost(%q) = %q, want unchanged", target, got) + } + } +} + func TestHandler_HandleDownload(t *testing.T) { req, err := http.NewRequest("GET", "/connect", nil) if err != nil {