Skip to content

Commit 6e0427c

Browse files
committed
fix(agent): fail native sessions loudly when setgroups is denied
In native mode the agent switches credentials via SysProcAttr.Credential when running as root, which makes the Go runtime call setgroups() during cmd.Start() — even for an empty group list. In an unprivileged user namespace /proc/self/setgroups is "deny", so setgroups fails with EPERM and every native SSH session dies right after auth with a cryptic error. Detect the denied policy up front (euid 0 AND /proc/self/setgroups == "deny") via a new command.CheckCredentialSwitch seam and refuse the session with a clear error and Exit(1) before launching the shell, rather than dropping supplementary groups silently. Docker mode is unaffected (it switches privileges via setpriv/nsenter) and gets a no-op. Also harden Heredoc against two latent nil-deref panics on a cmd.Start() failure (nil ProcessState, and the kill-goroutine racing on a nil Process), and add a non-prescriptive PTY-allocation hint to the Shell and Exec logs on ENOTTY.
1 parent dda5003 commit 6e0427c

7 files changed

Lines changed: 505 additions & 13 deletions

File tree

agent/server/modes/host/command/command_docker.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@ func nsenterArgs(present map[string]string) []string {
2929
return args
3030
}
3131

32+
// CheckCredentialSwitch is a no-op in Docker mode: the agent relies on
33+
// nsenter+setpriv for credential switching, so this check is not applicable.
34+
func CheckCredentialSwitch() error {
35+
return nil
36+
}
37+
3238
func NewCmd(u *osauth.User, shell, term, host string, envs []string, command ...string) *exec.Cmd {
3339
groups, err := osauth.ListGroups(u.Username)
3440
if err != nil {
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
//go:build docker
2+
// +build docker
3+
4+
package command
5+
6+
import "testing"
7+
8+
// Compile-time assertion: CheckCredentialSwitch is exported and has the expected
9+
// signature (func() error) under the docker build tag. This mirrors the
10+
// identical assertion in command_native_test.go for the !docker tag so that
11+
// neither tag set can silently break the build.
12+
var _ func() error = CheckCredentialSwitch
13+
14+
// TestCheckCredentialSwitchCompiles is a named test that anchors the
15+
// compile-time assertion above so it shows up in the test run output.
16+
// The assertion is evaluated at compile time; the test body itself is a
17+
// simple pass-through that confirms the symbol is reachable at runtime too.
18+
func TestCheckCredentialSwitchCompiles(t *testing.T) {
19+
// Calling the function ensures the linker includes the symbol and that
20+
// it actually returns nil in docker mode (documented no-op).
21+
if err := CheckCredentialSwitch(); err != nil {
22+
t.Errorf("CheckCredentialSwitch() returned unexpected error under -tags docker: %v", err)
23+
}
24+
}

agent/server/modes/host/command/command_native.go

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,68 @@
44
package command
55

66
import (
7+
"errors"
78
"os"
89
"os/exec"
10+
"strings"
911
"syscall"
1012

1113
"github.com/shellhub-io/shellhub/agent/pkg/osauth"
1214
log "github.com/sirupsen/logrus"
1315
)
1416

17+
// geteuidFn is a seam for os.Geteuid used in setgroupsDenied and NewCmd.
18+
// Tests can replace it to simulate running as root or non-root.
19+
var geteuidFn = os.Geteuid
20+
21+
// readSetgroupsPolicyFn is a seam for reading /proc/self/setgroups.
22+
// Tests can replace it to control the kernel policy value without filesystem access.
23+
var readSetgroupsPolicyFn = func() ([]byte, error) {
24+
return os.ReadFile("/proc/self/setgroups")
25+
}
26+
27+
// setgroupsDenied reports whether the kernel has denied setgroups(2) for this
28+
// process by checking /proc/self/setgroups.
29+
//
30+
// Return values:
31+
// - true: the policy file trims to "deny".
32+
// - false: the file does not exist (kernel too old or not in a user-ns); silent.
33+
// - false: any other read error; a warning is emitted via the logger.
34+
func setgroupsDenied() bool {
35+
data, err := readSetgroupsPolicyFn()
36+
if err != nil {
37+
if !errors.Is(err, os.ErrNotExist) {
38+
log.WithError(err).Warn("failed to read /proc/self/setgroups; assuming setgroups is allowed")
39+
}
40+
41+
return false
42+
}
43+
44+
return strings.TrimSpace(string(data)) == "deny"
45+
}
46+
47+
// CheckCredentialSwitch reports whether the process can switch credentials via
48+
// setgroups(2). It is a pre-flight check that must be called before attempting
49+
// to execute a command as a different user.
50+
//
51+
// Short-circuit: when the effective UID is not root (euid != 0), credential
52+
// switching is a no-op and the check always succeeds (nil).
53+
//
54+
// When euid == 0, the kernel may still forbid setgroups inside an unprivileged
55+
// user namespace (Linux ≥ 3.19). In that case the function returns a sentinel
56+
// error whose message contains "setgroups denied in unprivileged user namespace".
57+
func CheckCredentialSwitch() error {
58+
if geteuidFn() != 0 {
59+
return nil
60+
}
61+
62+
if setgroupsDenied() {
63+
return errors.New("setgroups denied in unprivileged user namespace")
64+
}
65+
66+
return nil
67+
}
68+
1569
func NewCmd(u *osauth.User, shell, term, host string, envs []string, command ...string) *exec.Cmd {
1670
groups, err := osauth.ListGroups(u.Username)
1771
if err != nil {
@@ -44,7 +98,7 @@ func NewCmd(u *osauth.User, shell, term, host string, envs []string, command ...
4498
cmd.Dir = u.HomeDir
4599
}
46100

47-
if os.Geteuid() == 0 {
101+
if geteuidFn() == 0 {
48102
cmd.SysProcAttr = &syscall.SysProcAttr{}
49103
cmd.SysProcAttr.Credential = &syscall.Credential{Uid: u.UID, Gid: u.GID, Groups: groups}
50104
}
Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
//go:build !docker
2+
// +build !docker
3+
4+
package command
5+
6+
import (
7+
"errors"
8+
"os"
9+
"sync/atomic"
10+
"testing"
11+
12+
"github.com/stretchr/testify/assert"
13+
)
14+
15+
// Compile-time assertion: CheckCredentialSwitch must be defined under the !docker tag.
16+
var _ = CheckCredentialSwitch
17+
18+
// TestSetgroupsDenied verifies the setgroupsDenied helper across all documented
19+
// scenarios. Tests do NOT call t.Parallel() because they mutate package-level
20+
// seam variables.
21+
//
22+
//nolint:paralleltest
23+
func TestSetgroupsDenied(t *testing.T) {
24+
origGeteuidFn := geteuidFn
25+
origReadSetgroupsPolicyFn := readSetgroupsPolicyFn
26+
27+
t.Cleanup(func() {
28+
geteuidFn = origGeteuidFn
29+
readSetgroupsPolicyFn = origReadSetgroupsPolicyFn
30+
})
31+
32+
t.Run("'deny' returns true", func(t *testing.T) {
33+
readSetgroupsPolicyFn = func() ([]byte, error) {
34+
return []byte("deny"), nil
35+
}
36+
37+
assert.True(t, setgroupsDenied())
38+
})
39+
40+
t.Run("'deny\\n' returns true", func(t *testing.T) {
41+
readSetgroupsPolicyFn = func() ([]byte, error) {
42+
return []byte("deny\n"), nil
43+
}
44+
45+
assert.True(t, setgroupsDenied())
46+
})
47+
48+
t.Run("' deny \\n' returns true", func(t *testing.T) {
49+
readSetgroupsPolicyFn = func() ([]byte, error) {
50+
return []byte(" deny \n"), nil
51+
}
52+
53+
assert.True(t, setgroupsDenied())
54+
})
55+
56+
t.Run("'allow' returns false", func(t *testing.T) {
57+
readSetgroupsPolicyFn = func() ([]byte, error) {
58+
return []byte("allow"), nil
59+
}
60+
61+
assert.False(t, setgroupsDenied())
62+
})
63+
64+
t.Run("ErrNotExist returns false without warning", func(t *testing.T) {
65+
readSetgroupsPolicyFn = func() ([]byte, error) {
66+
return nil, os.ErrNotExist
67+
}
68+
69+
// No warning expected — just false.
70+
assert.False(t, setgroupsDenied())
71+
})
72+
73+
t.Run("other read error returns false with warning", func(t *testing.T) {
74+
readSetgroupsPolicyFn = func() ([]byte, error) {
75+
return nil, errors.New("permission denied")
76+
}
77+
78+
// Should return false (and emit a warning via log, but we do not
79+
// capture log output — the return value is what matters here).
80+
assert.False(t, setgroupsDenied())
81+
})
82+
}
83+
84+
// TestCheckCredentialSwitch verifies CheckCredentialSwitch across its four
85+
// documented scenarios. Tests do NOT call t.Parallel() because they mutate
86+
// package-level seam variables.
87+
//
88+
//nolint:paralleltest
89+
func TestCheckCredentialSwitch(t *testing.T) {
90+
origGeteuidFn := geteuidFn
91+
origReadSetgroupsPolicyFn := readSetgroupsPolicyFn
92+
93+
t.Cleanup(func() {
94+
geteuidFn = origGeteuidFn
95+
readSetgroupsPolicyFn = origReadSetgroupsPolicyFn
96+
})
97+
98+
t.Run("euid!=0 returns nil without reading setgroups policy", func(t *testing.T) {
99+
var calls atomic.Int64
100+
101+
geteuidFn = func() int { return 1000 }
102+
readSetgroupsPolicyFn = func() ([]byte, error) {
103+
calls.Add(1)
104+
105+
return []byte("deny"), nil
106+
}
107+
108+
err := CheckCredentialSwitch()
109+
110+
assert.NoError(t, err)
111+
assert.Equal(t, int64(0), calls.Load(), "readSetgroupsPolicyFn must never be called when euid!=0")
112+
})
113+
114+
t.Run("euid!=0 with deny policy still returns nil (stub never called)", func(t *testing.T) {
115+
var calls atomic.Int64
116+
117+
geteuidFn = func() int { return 500 }
118+
readSetgroupsPolicyFn = func() ([]byte, error) {
119+
calls.Add(1)
120+
121+
return []byte("deny"), nil
122+
}
123+
124+
err := CheckCredentialSwitch()
125+
126+
assert.NoError(t, err)
127+
assert.Equal(t, int64(0), calls.Load(), "readSetgroupsPolicyFn must never be called when euid!=0")
128+
})
129+
130+
t.Run("euid==0 and setgroups deny returns error containing 'setgroups denied in unprivileged user namespace'", func(t *testing.T) {
131+
geteuidFn = func() int { return 0 }
132+
readSetgroupsPolicyFn = func() ([]byte, error) {
133+
return []byte("deny"), nil
134+
}
135+
136+
err := CheckCredentialSwitch()
137+
138+
assert.Error(t, err)
139+
assert.ErrorContains(t, err, "setgroups denied in unprivileged user namespace")
140+
})
141+
142+
t.Run("euid==0 and setgroups allow returns nil", func(t *testing.T) {
143+
geteuidFn = func() int { return 0 }
144+
readSetgroupsPolicyFn = func() ([]byte, error) {
145+
return []byte("allow"), nil
146+
}
147+
148+
err := CheckCredentialSwitch()
149+
150+
assert.NoError(t, err)
151+
})
152+
}

0 commit comments

Comments
 (0)