diff --git a/cmd/rdpgw/protocol/gateway.go b/cmd/rdpgw/protocol/gateway.go index c1f6869..e38b18e 100644 --- a/cmd/rdpgw/protocol/gateway.go +++ b/cmd/rdpgw/protocol/gateway.go @@ -75,6 +75,11 @@ func (g *Gateway) HandleGatewayProtocol(w http.ResponseWriter, r *http.Request) } } else { t = x.(*Tunnel) + if !tunnelOwnerMatches(t, id) { + log.Printf("rejecting reuse of Rdg-Connection-Id %q from a different identity", connId) + http.Error(w, "Tunnel is owned by a different session", http.StatusUnauthorized) + return + } } ctx = context.WithValue(ctx, CtxTunnel, t) @@ -106,6 +111,25 @@ func (g *Gateway) HandleGatewayProtocol(w http.ResponseWriter, r *http.Request) } } +// tunnelOwnerMatches reports whether the cached tunnel was opened by the same +// identity making the current request. The Rdg-Connection-Id is a client- +// chosen header that pairs the two halves of a session; without this check +// any caller who learns or guesses one can attach to another user's tunnel. +func tunnelOwnerMatches(t *Tunnel, id identity.Identity) bool { + if t == nil || t.User == nil || id == nil { + return false + } + if t.User.UserName() == "" || t.User.UserName() != id.UserName() { + return false + } + cachedIp, _ := t.User.GetAttribute(identity.AttrClientIp).(string) + reqIp, _ := id.GetAttribute(identity.AttrClientIp).(string) + if cachedIp == "" { + return false + } + return cachedIp == reqIp +} + // headerHasToken reports whether the HTTP header named by name contains // token, matched case-insensitively against comma-separated tokens. // Fields like Connection carry a list (e.g. "keep-alive, Upgrade") so a diff --git a/cmd/rdpgw/protocol/gateway_test.go b/cmd/rdpgw/protocol/gateway_test.go index fd288de..779966f 100644 --- a/cmd/rdpgw/protocol/gateway_test.go +++ b/cmd/rdpgw/protocol/gateway_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/bolkedebruin/rdpgw/cmd/rdpgw/identity" + "github.com/patrickmn/go-cache" ) func TestHeaderHasToken(t *testing.T) { @@ -144,3 +145,63 @@ func TestHandleGatewayProtocolRouting(t *testing.T) { }) } } + +// TestTunnelOwnershipEnforced asserts that an Rdg-Connection-Id which already +// has a cached tunnel cannot be reused by a different identity. The connection +// id travels in plain HTTP headers, so a client that learns or guesses one +// must not be able to attach to the original tunnel — the cache is for +// matching the two halves of the same client's session, not for authorizing +// access. +func TestTunnelOwnershipEnforced(t *testing.T) { + connId := "shared-connid-ownership" + + aliceID := identity.NewUser() + aliceID.SetUserName("alice") + aliceID.SetAttribute(identity.AttrRemoteAddr, "10.1.1.1:1234") + aliceID.SetAttribute(identity.AttrClientIp, "10.1.1.1") + c.Set(connId, &Tunnel{ + RDGId: connId, + RemoteAddr: "10.1.1.1:1234", + User: aliceID, + }, cache.DefaultExpiration) + defer c.Delete(connId) + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + bob := identity.NewUser() + bob.SetUserName("bob") + bob.SetAttribute(identity.AttrRemoteAddr, "10.2.2.2:5678") + bob.SetAttribute(identity.AttrClientIp, "10.2.2.2") + r = identity.AddToRequestCtx(bob, r) + (&Gateway{}).HandleGatewayProtocol(w, r) + }) + srv := httptest.NewServer(handler) + defer srv.Close() + + addr := strings.TrimPrefix(srv.URL, "http://") + conn, err := net.DialTimeout("tcp", addr, 2*time.Second) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + if err := conn.SetDeadline(time.Now().Add(3 * time.Second)); err != nil { + t.Fatalf("set deadline: %v", err) + } + + req := "RDG_OUT_DATA /remoteDesktopGateway/ HTTP/1.1\r\n" + + "Host: " + addr + "\r\n" + + "Rdg-Connection-Id: " + connId + "\r\n" + + "\r\n" + if _, err := conn.Write([]byte(req)); err != nil { + t.Fatalf("write: %v", err) + } + + line, err := bufio.NewReader(conn).ReadString('\n') + if err != nil { + t.Fatalf("read status line: %v", err) + } + line = strings.TrimRight(line, "\r\n") + + if strings.HasPrefix(line, "HTTP/1.1 2") { + t.Fatalf("a different identity attached to a cached tunnel via Rdg-Connection-Id (status %q); the header was treated as authorization", line) + } +}