diff --git a/client/client.go b/client/client.go index 49b62e65..59698be6 100644 --- a/client/client.go +++ b/client/client.go @@ -105,7 +105,7 @@ func NewClient(c *Config) (*Client, error) { tlsConfig: nil, } //set default log level - client.Logger.Info = true + client.Logger.Info = c.Verbose //configure tls if u.Scheme == "wss" { tc := &tls.Config{} diff --git a/go.mod b/go.mod index d665ad8a..73cf3c04 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/jpillora/chisel -go 1.26.4 +go 1.26.5 require ( github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 @@ -10,7 +10,7 @@ require ( github.com/jpillora/requestlog v1.0.0 github.com/jpillora/sizestr v1.0.0 golang.org/x/crypto v0.53.0 - golang.org/x/net v0.55.0 + golang.org/x/net v0.56.0 golang.org/x/sync v0.21.0 ) diff --git a/go.sum b/go.sum index 359b1cc4..a2e8edb8 100644 --- a/go.sum +++ b/go.sum @@ -18,8 +18,8 @@ github.com/tomasen/realip v0.0.0-20180522021738-f0c99a92ddce h1:fb190+cK2Xz/dvi9 github.com/tomasen/realip v0.0.0-20180522021738-f0c99a92ddce/go.mod h1:o8v6yHRoik09Xen7gje4m9ERNah1d1PPsVq1VEx9vE4= golang.org/x/crypto v0.53.0 h1:QZ4Muo8THX6CizN2vPPd5fBGHyogrdK9fG4wLPFUsto= golang.org/x/crypto v0.53.0/go.mod h1:DNLU434OwVakk9PzuwV8w62mAJpRJL3vsgcfp4Qnsio= -golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= -golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= +golang.org/x/net v0.56.0 h1:Rw8j/hFzGvJUZwNBXnAtf5sVDVt+65SK2C7IxCxZt5o= +golang.org/x/net v0.56.0/go.mod h1:D3Ku6r+V6JROoZK144D2XfMHFcMq/0zSfLelVTCFKec= golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= diff --git a/share/settings/remote.go b/share/settings/remote.go index bfe6cbae..520b2fcd 100644 --- a/share/settings/remote.go +++ b/share/settings/remote.go @@ -221,6 +221,13 @@ func (r Remote) UserAddr() string { if r.Reverse { return "R:" + r.LocalHost + ":" + r.LocalPort } + //forward socks is granted via the literal "socks" token, matching the + //per-channel ACL check in tunnel_out_ssh.go (ExtraData == "socks"). + //Without this it would be ":" (empty host:port), which is opaque and + //inconsistent with the channel-level check. + if r.Socks { + return "socks" + } return r.RemoteHost + ":" + r.RemotePort } diff --git a/share/tunnel/tunnel_out_ssh.go b/share/tunnel/tunnel_out_ssh.go index 0f2fb5d2..3b380fa8 100644 --- a/share/tunnel/tunnel_out_ssh.go +++ b/share/tunnel/tunnel_out_ssh.go @@ -47,7 +47,9 @@ func (t *Tunnel) handleSSHChannel(ch ssh.NewChannel) { return } //check ACL against the actual requested destination - if t.Config.ACL != nil && !socks && !t.Config.ACL(hostPort) { + //(hostPort == "socks" for socks channels, so socks is gated too, + //symmetric with the config-time UserAddr() check in server_handler.go) + if t.Config.ACL != nil && !t.Config.ACL(hostPort) { t.Debugf("Denied connection to %s (ACL)", hostPort) ch.Reject(ssh.Prohibited, "access denied") return diff --git a/share/tunnel/wg.go b/share/tunnel/wg.go index e915ad0a..e3a3d8eb 100644 --- a/share/tunnel/wg.go +++ b/share/tunnel/wg.go @@ -1,30 +1,36 @@ package tunnel -import ( - "sync" - "sync/atomic" -) +import "sync" type waitGroup struct { + mu sync.Mutex inner sync.WaitGroup - n int32 + n int } func (w *waitGroup) Add(n int) { - atomic.AddInt32(&w.n, int32(n)) + w.mu.Lock() + w.n += n w.inner.Add(n) + w.mu.Unlock() } func (w *waitGroup) Done() { - if n := atomic.LoadInt32(&w.n); n > 0 && atomic.CompareAndSwapInt32(&w.n, n, n-1) { + w.mu.Lock() + if w.n > 0 { + w.n-- w.inner.Done() } + w.mu.Unlock() } func (w *waitGroup) DoneAll() { - for atomic.LoadInt32(&w.n) > 0 { - w.Done() + w.mu.Lock() + for w.n > 0 { + w.n-- + w.inner.Done() } + w.mu.Unlock() } func (w *waitGroup) Wait() { diff --git a/share/tunnel/wg_test.go b/share/tunnel/wg_test.go new file mode 100644 index 00000000..01e0b844 --- /dev/null +++ b/share/tunnel/wg_test.go @@ -0,0 +1,63 @@ +package tunnel + +import ( + "sync" + "testing" + "time" +) + +// TestWaitGroupAddDoneAllRace stresses concurrent Add/DoneAll to reproduce the +// race fixed in #585. Without the mutex, Add() is non-atomic between bumping +// the counter and the inner sync.WaitGroup, so DoneAll()'s loop can call +// inner.Done() before inner.Add() runs and panic with "negative WaitGroup +// counter". +func TestWaitGroupAddDoneAllRace(t *testing.T) { + var wg waitGroup + stop := make(chan struct{}) + var workers sync.WaitGroup + for i := 0; i < 8; i++ { + workers.Add(2) + go func() { + defer workers.Done() + for { + select { + case <-stop: + return + default: + wg.Add(1) + } + } + }() + go func() { + defer workers.Done() + for { + select { + case <-stop: + return + default: + wg.DoneAll() + } + } + }() + } + time.Sleep(500 * time.Millisecond) + close(stop) + workers.Wait() + wg.DoneAll() + wg.Wait() +} + +func TestWaitGroupDoneAllDrains(t *testing.T) { + var wg waitGroup + wg.Add(3) + wg.DoneAll() + wg.Wait() +} + +func TestWaitGroupDoneIsIdempotent(t *testing.T) { + var wg waitGroup + wg.Add(1) + wg.Done() + wg.Done() // extra Done must be a no-op, not a panic + wg.Wait() +} diff --git a/test/e2e/acl_channel_test.go b/test/e2e/acl_channel_test.go index 65e18388..0b5ce147 100644 --- a/test/e2e/acl_channel_test.go +++ b/test/e2e/acl_channel_test.go @@ -6,6 +6,8 @@ import ( "io" "net" "net/http" + "strconv" + "strings" "testing" "time" @@ -321,3 +323,138 @@ func TestAuthWildcardChannel(t *testing.T) { } t.Logf("wildcard user correctly allowed") } + +// TestAuthSocksChannelDenied verifies that a user who is NOT granted socks +// cannot open a socks channel, even when the server runs with --socks5. +// socks channels are authorized against the per-user ACL like any other +// destination (the channel token is the literal "socks"). +func TestAuthSocksChannelDenied(t *testing.T) { + allowedPort := availablePort() + + // SOCKS5 is enabled, so any rejection can only come from the ACL, + // not from the "SOCKS5 is not enabled" guard. + s, err := chserver.NewServer(&chserver.Config{ + KeySeed: "acl-socks-denied", + Socks5: true, + }) + if err != nil { + t.Fatal(err) + } + s.Debug = debug + // user is pinned to a single TCP destination; socks is NOT granted + if err := s.AddUser("user", "pass", fmt.Sprintf(`^127\.0\.0\.1:%s$`, allowedPort)); err != nil { + t.Fatal(err) + } + serverPort := availablePort() + if err := s.Start("127.0.0.1", serverPort); err != nil { + t.Fatal(err) + } + defer s.Close() + + // declare ONLY the allowed remote so the config handshake passes + sc, _, _ := dialChiselSSH(t, "127.0.0.1:"+serverPort, "user", "pass") + defer sc.Close() + r, err := settings.DecodeRemote(fmt.Sprintf("0.0.0.0:%s:127.0.0.1:%s", allowedPort, allowedPort)) + if err != nil { + t.Fatal(err) + } + sendConfig(t, sc, []*settings.Remote{r}) + + // open a raw channel whose ExtraData is "socks" — must be denied by the ACL + ch, _, err := sc.OpenChannel("chisel", []byte("socks")) + if err == nil { + ch.Close() + t.Fatal("socks channel was accepted for a user without socks access") + } + // it must be the ACL talking, not "SOCKS5 is not enabled" + if !strings.Contains(err.Error(), "access denied") { + t.Fatalf("socks channel rejected for the wrong reason: %v", err) + } + t.Logf("socks channel correctly rejected by ACL: %v", err) +} + +// TestAuthSocksChannelAllowed verifies the ACL does not over-block: a user +// explicitly granted socks (a regex matching the "socks" channel token) can +// open a socks channel and reach a destination through the SOCKS5 proxy. +func TestAuthSocksChannelAllowed(t *testing.T) { + targetPort := availablePort() + + // a destination to CONNECT to through socks + ln, err := net.Listen("tcp", "127.0.0.1:"+targetPort) + if err != nil { + t.Fatal(err) + } + defer ln.Close() + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + conn.Write([]byte("VIA-SOCKS")) + conn.Close() + } + }() + + s, err := chserver.NewServer(&chserver.Config{ + KeySeed: "acl-socks-allowed", + Socks5: true, + }) + if err != nil { + t.Fatal(err) + } + s.Debug = debug + // grant socks at the channel level (the channel token is the literal "socks") + if err := s.AddUser("user", "pass", `^socks$`); err != nil { + t.Fatal(err) + } + serverPort := availablePort() + if err := s.Start("127.0.0.1", serverPort); err != nil { + t.Fatal(err) + } + defer s.Close() + + sc, _, _ := dialChiselSSH(t, "127.0.0.1:"+serverPort, "user", "pass") + defer sc.Close() + // no remotes to declare; the socks channel is opened directly + sendConfig(t, sc, []*settings.Remote{}) + + ch, reqs, err := sc.OpenChannel("chisel", []byte("socks")) + if err != nil { + t.Fatalf("socks channel rejected for a user granted socks: %v", err) + } + go ssh.DiscardRequests(reqs) + defer ch.Close() + + // drive a minimal SOCKS5 CONNECT to the allowed local listener + if _, err := ch.Write([]byte{0x05, 0x01, 0x00}); err != nil { // greeting: no-auth + t.Fatal(err) + } + if _, err := io.ReadFull(ch, make([]byte, 2)); err != nil { // method selection + t.Fatal(err) + } + port, err := strconv.Atoi(targetPort) + if err != nil { + t.Fatal(err) + } + connect := []byte{0x05, 0x01, 0x00, 0x01, 127, 0, 0, 1, byte(port >> 8), byte(port)} + if _, err := ch.Write(connect); err != nil { + t.Fatal(err) + } + reply := make([]byte, 10) + if _, err := io.ReadFull(ch, reply); err != nil { + t.Fatal(err) + } + if reply[1] != 0x00 { + t.Fatalf("socks CONNECT failed, rep=%d", reply[1]) + } + buf := make([]byte, 16) + n, err := ch.Read(buf) + if err != nil && err != io.EOF { + t.Fatalf("read via socks: %v", err) + } + if string(buf[:n]) != "VIA-SOCKS" { + t.Fatalf("expected 'VIA-SOCKS' via socks, got %q", buf[:n]) + } + t.Logf("socks channel allowed and functional for a granted user") +} diff --git a/test/e2e/socks_test.go b/test/e2e/socks_test.go index 420f4337..cc3b155a 100644 --- a/test/e2e/socks_test.go +++ b/test/e2e/socks_test.go @@ -1,5 +1,113 @@ package e2e_test -//TODO tests for: -// - SOCKS-client -> [client -> server SOCKS] -> endpoint -// - SOCKS-client -> [server -> client SOCKS] -> endpoint +import ( + "context" + "io" + "net" + "net/http" + "strings" + "testing" + "time" + + "golang.org/x/net/proxy" + + chclient "github.com/jpillora/chisel/client" + chserver "github.com/jpillora/chisel/server" +) + +//TODO test: SOCKS-client -> [server -> client SOCKS] -> endpoint (reverse socks) + +// TestSocksEndToEnd exercises the full forward-SOCKS5 path for a user granted +// socks via "^socks$": +// +// http client -> chisel client's local SOCKS5 -> tunnel -> +// chisel server's SOCKS5 proxy -> endpoint +// +// It covers both ACL checks: the config-time check (UserAddr() == "socks") +// lets the client register its socks remote, and the per-channel check lets +// each tunnelled connection through. A "^socks$"-only user only succeeds when +// both checks agree on the "socks" token. +func TestSocksEndToEnd(t *testing.T) { + // endpoint reached via the socks proxy: echoes the request body + '!' + endpoint := &http.Server{ + Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + b, _ := io.ReadAll(r.Body) + w.Write(append(b, '!')) + }), + } + endpointPort := availablePort() + el, err := net.Listen("tcp", "127.0.0.1:"+endpointPort) + if err != nil { + t.Fatal(err) + } + go endpoint.Serve(el) + defer endpoint.Close() + + // chisel server: SOCKS5 enabled, user granted ONLY socks + s, err := chserver.NewServer(&chserver.Config{ + KeySeed: "socks-e2e", + Socks5: true, + }) + if err != nil { + t.Fatal(err) + } + s.Debug = debug + if err := s.AddUser("user", "pass", `^socks$`); err != nil { + t.Fatal(err) + } + serverPort := availablePort() + if err := s.Start("127.0.0.1", serverPort); err != nil { + t.Fatal(err) + } + defer s.Close() + + // chisel client: local SOCKS5 listener, authed as the socks-only user + socksPort := availablePort() + c, err := chclient.NewClient(&chclient.Config{ + Server: "http://127.0.0.1:" + serverPort, + Auth: "user:pass", + Fingerprint: s.GetFingerprint(), + Remotes: []string{"127.0.0.1:" + socksPort + ":socks"}, + }) + if err != nil { + t.Fatal(err) + } + c.Debug = debug + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if err := c.Start(ctx); err != nil { + t.Fatal(err) + } + defer c.Close() + + // HTTP request through the local SOCKS5 proxy to the endpoint, retrying + // until the tunnel is established (bounded ~5s). + dialer, err := proxy.SOCKS5("tcp", "127.0.0.1:"+socksPort, nil, proxy.Direct) + if err != nil { + t.Fatal(err) + } + httpClient := &http.Client{ + Timeout: 2 * time.Second, + Transport: &http.Transport{ + DialContext: func(_ context.Context, network, addr string) (net.Conn, error) { + return dialer.Dial(network, addr) + }, + }, + } + endpointURL := "http://" + net.JoinHostPort("127.0.0.1", endpointPort) + "/" + var body string + for i := 0; i < 50; i++ { + resp, err := httpClient.Post(endpointURL, "text/plain", strings.NewReader("foo")) + if err == nil { + b, _ := io.ReadAll(resp.Body) + resp.Body.Close() + body = string(b) + break + } + time.Sleep(100 * time.Millisecond) + } + if body != "foo!" { + t.Fatalf("expected \"foo!\" through socks, got %q", body) + } + t.Logf("forward socks end-to-end works for a ^socks$ user") +}