Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Dockerfile: Support socat use cases (#13208)" #13369

Merged
merged 1 commit into from Oct 31, 2020

Conversation

sapk
Copy link
Member

@sapk sapk commented Oct 31, 2020

This reverts commit ff50274.

I think we shouldn't pack socat binary by default in the gitea docker image.

It is not a big threat but it is better to not offer some leverage to help an attacker as it can be used for two-way shell and data exfiltration.

If some users want it for their very specific use case they can easily do so and do what is needed to limit the risk knowingly.

For example: https://gtfobins.github.io/gtfobins/socat/

@sapk sapk added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Oct 31, 2020
@codecov-io
Copy link

Codecov Report

Merging #13369 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13369      +/-   ##
==========================================
+ Coverage   42.14%   42.16%   +0.02%     
==========================================
  Files         690      690              
  Lines       75871    75888      +17     
==========================================
+ Hits        31975    32001      +26     
+ Misses      38668    38648      -20     
- Partials     5228     5239      +11     
Impacted Files Coverage Δ
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
modules/indexer/stats/db.go 52.17% <0.00%> (-8.70%) ⬇️
routers/api/v1/repo/release_tags.go 53.57% <0.00%> (-7.97%) ⬇️
modules/git/utils.go 73.77% <0.00%> (-3.28%) ⬇️
modules/git/repo.go 46.19% <0.00%> (-1.53%) ⬇️
services/pull/check.go 51.82% <0.00%> (-0.73%) ⬇️
modules/context/api.go 75.22% <0.00%> (ø)
routers/api/v1/api.go 79.50% <0.00%> (+0.05%) ⬆️
modules/notification/webhook/webhook.go 49.65% <0.00%> (+0.34%) ⬆️
modules/queue/workerpool.go 60.00% <0.00%> (+1.22%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e16a5bb...b76eef3. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 31, 2020
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 31, 2020
@silverwind
Copy link
Member

I'm not sure what one can achieve with socat inside a docker container, but I guess it may be better to stay on the safe side afterall.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 31, 2020
@6543 6543 added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Oct 31, 2020
@lafriks lafriks merged commit 7cb22d6 into go-gitea:master Oct 31, 2020
@sapk sapk deleted the revert-ocat branch October 31, 2020 13:32
@bbros-dev
Copy link
Contributor

bbros-dev commented Nov 1, 2020

Ack the container space is a red-hot mess in terms of security, but it is what it is, and is improving. However in the mean time that mess does leak its pollution into applications. Again there is limited use in whining, and we just have to work around these things until the situation improves.

Apologies for not being more specific about the exact use case, but the detail seemed extraneous to:

  1. A Dockerfile that is aimed at rootful use - by definition a very relaxed security posture (ack you just now added a rootless Dockerfile)
  2. A container that provides bash

The issue arises when mounting WireGuard in a network name space, and then running the container within that namespace.
Not sure if Docker can do that, but with Podman you lose the port sharing/publishing - this is Podman issue #8031. To workaround that you configure Gitea to publish data as a socket and then publish that to the container host port. But this leaves the network namespace without a port to connect other applications to.

Gitea (currently) cannot publish to both socket and port, and thus socat in the container is a workaround for that Gitea behavior, and supports running the Gitea container in a WireGuarded network namespace.

Of course we work around all this with own Gitea container, but we prefer pushing improvements upstream.

Having given you the detail we leave it in your court.

@sapk
Copy link
Member Author

sapk commented Nov 3, 2020

@bbros-dev Your problem is quite limited to a very specific case. You can easily create your own image freely by creating a repository with only a Dockerfile that contain.

FROM gitea/gitea:latest

RUN apk --no-cache add socat 

and publish it to the registry of your choice (docker hub, quay, ...) that will build it for you.

I known that it should be a very complicated attack to access the socat binary but I feel we shouldn't let this possibility for every user since they don't need it and since it would ease the process after.

We can find solution to your problem but it shouldn't possibly impact every users.

For root container, we start to offer a rootless image and I would personally try to reduce the number of binary we need (for security but also maintenance of multiple version compatibility)

I hope you will not take it personally because is is a logic we apply in most cases. Ex: We try not to implement each rendering of file possible but make it an optional configuration if possible.

@bbros-dev
Copy link
Contributor

Your problem is quite limited to a very specific case.

Yup, unfortunately securing containers is not trivial.... so insecure by default seems to be the Docker world default.

You can easily create your own image

Yes, as we said we do.

Of course we work around all this with own Gitea container...

I known that it should be a very complicated attack to access the socat binary

Yes it seems quite peculiar to be relaxed about have bash in the container but object to socat.

Perhaps a middle ground is:

  1. keeping socat in the rootful container but not in the rootless container.
  2. adding a note that Gitea recomends not running rootful containers, but provides it a convenience for very limited use cases and advanced users.

We can find solution to your problem

If you wanted to it would be to allow both http and socket connections. Right now its either/or, but not both. Unfortunately we can't commit to making that change, so this was the next best thing.

I hope you will not take it personally

Of course not.

@go-gitea go-gitea locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants