Skip to content

feat: block file transfers for security #13501

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

Merged
merged 15 commits into from
Jun 10, 2024
Merged

feat: block file transfers for security #13501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
94a4067
feat: block file transfers
mtojek Jun 7, 2024
9915cde
commands
mtojek Jun 7, 2024
abd97df
fix
mtojek Jun 7, 2024
00e0a8a
fix
mtojek Jun 7, 2024
6cec719
Merge branch 'main' into 13383-scp
mtojek Jun 7, 2024
fa0010d
Fix
mtojek Jun 7, 2024
dbaa763
fix
mtojek Jun 7, 2024
94926bd
fix
mtojek Jun 7, 2024
d92c070
fix
mtojek Jun 7, 2024
47ee9b0
fix
mtojek Jun 7, 2024
4e45761
PR comments
mtojek Jun 10, 2024
d852417
WIP
mtojek Jun 10, 2024
96ca9ae
Fix
mtojek Jun 10, 2024
19fe5cf
fix lint
mtojek Jun 10, 2024
e3a9dba
Merge branch 'main' into 13383-scp
mtojek Jun 10, 2024
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
4 changes: 4 additions & 0 deletions agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ type Options struct {
ModifiedProcesses chan []*agentproc.Process
// ProcessManagementTick is used for testing process priority management.
ProcessManagementTick <-chan time.Time
BlockFileTransfer bool
}

type Client interface {
Expand Down Expand Up @@ -184,6 +185,7 @@ func New(options Options) Agent {
modifiedProcs: options.ModifiedProcesses,
processManagementTick: options.ProcessManagementTick,
logSender: agentsdk.NewLogSender(options.Logger),
blockFileTransfer: options.BlockFileTransfer,

prometheusRegistry: prometheusRegistry,
metrics: newAgentMetrics(prometheusRegistry),
Expand Down Expand Up @@ -239,6 +241,7 @@ type agent struct {
sessionToken atomic.Pointer[string]
sshServer *agentssh.Server
sshMaxTimeout time.Duration
blockFileTransfer bool

lifecycleUpdate chan struct{}
lifecycleReported chan codersdk.WorkspaceAgentLifecycle
Expand Down Expand Up @@ -277,6 +280,7 @@ func (a *agent) init() {
AnnouncementBanners: func() *[]codersdk.BannerConfig { return a.announcementBanners.Load() },
UpdateEnv: a.updateCommandEnv,
WorkingDirectory: func() string { return a.manifest.Load().Directory },
BlockFileTransfer: a.blockFileTransfer,
})
if err != nil {
panic(err)
Expand Down
93 changes: 93 additions & 0 deletions agent/agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,99 @@ func TestAgent_SCP(t *testing.T) {
require.NoError(t, err)
}

func TestAgent_FileTransferBlocked(t *testing.T) {
t.Parallel()

assertFileTransferBlocked := func(t *testing.T, errorMessage string) {
// NOTE: Checking content of the error message is flaky. Most likely there is a race condition, which results
// in stopping the client in different phases, and returning different errors:
// - client read the full error message: File transfer has been disabled.
// - client's stream was terminated before reading the error message: EOF
// - client just read the error code (Windows): Process exited with status 65
isErr := strings.Contains(errorMessage, agentssh.BlockedFileTransferErrorMessage) ||
strings.Contains(errorMessage, "EOF") ||
strings.Contains(errorMessage, "Process exited with status 65")
require.True(t, isErr, fmt.Sprintf("Message: "+errorMessage))
}

t.Run("SFTP", func(t *testing.T) {
t.Parallel()

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()

//nolint:dogsled
conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) {
o.BlockFileTransfer = true
})
sshClient, err := conn.SSHClient(ctx)
require.NoError(t, err)
defer sshClient.Close()
_, err = sftp.NewClient(sshClient)
require.Error(t, err)
assertFileTransferBlocked(t, err.Error())
})

t.Run("SCP with go-scp package", func(t *testing.T) {
t.Parallel()

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()

//nolint:dogsled
conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) {
o.BlockFileTransfer = true
})
sshClient, err := conn.SSHClient(ctx)
require.NoError(t, err)
defer sshClient.Close()
scpClient, err := scp.NewClientBySSH(sshClient)
require.NoError(t, err)
defer scpClient.Close()
tempFile := filepath.Join(t.TempDir(), "scp")
err = scpClient.CopyFile(context.Background(), strings.NewReader("hello world"), tempFile, "0755")
require.Error(t, err)
assertFileTransferBlocked(t, err.Error())
})

t.Run("Forbidden commands", func(t *testing.T) {
t.Parallel()

for _, c := range agentssh.BlockedFileTransferCommands {
t.Run(c, func(t *testing.T) {
t.Parallel()

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()

//nolint:dogsled
conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) {
o.BlockFileTransfer = true
})
sshClient, err := conn.SSHClient(ctx)
require.NoError(t, err)
defer sshClient.Close()

session, err := sshClient.NewSession()
require.NoError(t, err)
defer session.Close()

stdout, err := session.StdoutPipe()
require.NoError(t, err)

//nolint:govet // we don't need `c := c` in Go 1.22
err = session.Start(c)
require.NoError(t, err)
defer session.Close()

msg, err := io.ReadAll(stdout)
require.NoError(t, err)
assertFileTransferBlocked(t, string(msg))
})
}
})
}

func TestAgent_EnvironmentVariables(t *testing.T) {
t.Parallel()
key := "EXAMPLE"
Expand Down
53 changes: 53 additions & 0 deletions agent/agentssh/agentssh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,16 @@ const (
// MagicProcessCmdlineJetBrains is a string in a process's command line that
// uniquely identifies it as JetBrains software.
MagicProcessCmdlineJetBrains = "idea.vendor.name=JetBrains"

// BlockedFileTransferErrorCode indicates that SSH server restricted the raw command from performing
// the file transfer.
BlockedFileTransferErrorCode = 65 // Error code: host not allowed to connect
BlockedFileTransferErrorMessage = "File transfer has been disabled."
)

// BlockedFileTransferCommands contains a list of restricted file transfer commands.
var BlockedFileTransferCommands = []string{"nc", "rsync", "scp", "sftp"}

// Config sets configuration parameters for the agent SSH server.
type Config struct {
// MaxTimeout sets the absolute connection timeout, none if empty. If set to
Expand All @@ -74,6 +82,8 @@ type Config struct {
// X11SocketDir is the directory where X11 sockets are created. Default is
// /tmp/.X11-unix.
X11SocketDir string
// BlockFileTransfer restricts use of file transfer applications.
BlockFileTransfer bool
}

type Server struct {
Expand Down Expand Up @@ -272,6 +282,18 @@ func (s *Server) sessionHandler(session ssh.Session) {
extraEnv = append(extraEnv, fmt.Sprintf("DISPLAY=:%d.0", x11.ScreenNumber))
}

if s.fileTransferBlocked(session) {
s.logger.Warn(ctx, "file transfer blocked", slog.F("session_subsystem", session.Subsystem()), slog.F("raw_command", session.RawCommand()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of scope: we may want to eventually have a way to surface this to admins as they may be interested in this sort of thing.

Copy link
Member Author

@mtojek mtojek Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a new metric? maybe consider this as a follow-up option. I don't know if an admin wants to review notifications from users accidentally trying to use scp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we run the risk here of logging a potentially large amount of text here?
Should we try and capture the user identity associated with this attempt, too?

Aside: this feels like a good use-case for an audit-log.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #13501 (comment)?

Yeah, the problem here is that it is logged on the agent side in /tmp/coder.blahblah/coder-agent-<some-file>.log, and it is not raised to admins. We may want to think about some metrics to notify them.


if session.Subsystem() == "" { // sftp does not expect error, otherwise it fails with "package too long"
// Response format: <status_code><message body>\n
errorMessage := fmt.Sprintf("\x02%s\n", BlockedFileTransferErrorMessage)
_, _ = session.Write([]byte(errorMessage))
}
_ = session.Exit(BlockedFileTransferErrorCode)
return
}

switch ss := session.Subsystem(); ss {
case "":
case "sftp":
Expand Down Expand Up @@ -322,6 +344,37 @@ func (s *Server) sessionHandler(session ssh.Session) {
_ = session.Exit(0)
}

// fileTransferBlocked method checks if the file transfer commands should be blocked.
//
// Warning: consider this mechanism as "Do not trespass" sign, as a violator can still ssh to the host,
// smuggle the `scp` binary, or just manually send files outside with `curl` or `ftp`.
// If a user needs a more sophisticated and battle-proof solution, consider full endpoint security.
func (s *Server) fileTransferBlocked(session ssh.Session) bool {
if !s.config.BlockFileTransfer {
return false // file transfers are permitted
}
// File transfers are restricted.

if session.Subsystem() == "sftp" {
return true
}

cmd := session.Command()
if len(cmd) == 0 {
return false // no command?
}

c := cmd[0]
c = filepath.Base(c) // in case the binary is absolute path, /usr/sbin/scp

for _, cmd := range BlockedFileTransferCommands {
if cmd == c {
return true
}
}
return false
}

func (s *Server) sessionStart(logger slog.Logger, session ssh.Session, extraEnv []string) (retErr error) {
ctx := session.Context()
env := append(session.Environ(), extraEnv...)
Expand Down
11 changes: 11 additions & 0 deletions cli/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"cdr.dev/slog/sloggers/slogstackdriver"
"github.com/coder/coder/v2/agent"
"github.com/coder/coder/v2/agent/agentproc"
"github.com/coder/coder/v2/agent/agentssh"
"github.com/coder/coder/v2/agent/reaper"
"github.com/coder/coder/v2/buildinfo"
"github.com/coder/coder/v2/codersdk"
Expand All @@ -48,6 +49,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
slogHumanPath string
slogJSONPath string
slogStackdriverPath string
blockFileTransfer bool
)
cmd := &serpent.Command{
Use: "agent",
Expand Down Expand Up @@ -314,6 +316,8 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
// Intentionally set this to nil. It's mainly used
// for testing.
ModifiedProcesses: nil,

BlockFileTransfer: blockFileTransfer,
})

promHandler := agent.PrometheusMetricsHandler(prometheusRegistry, logger)
Expand Down Expand Up @@ -417,6 +421,13 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
Default: "",
Value: serpent.StringOf(&slogStackdriverPath),
},
{
Flag: "block-file-transfer",
Default: "false",
Env: "CODER_AGENT_BLOCK_FILE_TRANSFER",
Description: fmt.Sprintf("Block file transfer using known applications: %s.", strings.Join(agentssh.BlockedFileTransferCommands, ",")),
Value: serpent.BoolOf(&blockFileTransfer),
},
}

return cmd
Expand Down
3 changes: 3 additions & 0 deletions cli/testdata/coder_agent_--help.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ OPTIONS:
--auth string, $CODER_AGENT_AUTH (default: token)
Specify the authentication type to use for the agent.

--block-file-transfer bool, $CODER_AGENT_BLOCK_FILE_TRANSFER (default: false)
Block file transfer using known applications: nc,rsync,scp,sftp.

--debug-address string, $CODER_AGENT_DEBUG_ADDRESS (default: 127.0.0.1:2113)
The bind address to serve a debug HTTP server.

Expand Down
Loading