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

security: apply least-privilege by removing chown on binaries #45

Closed

Conversation

patricklodder
Copy link
Member

I was triggered by the test shown in #33 because chowning files under /usr/local to be owned by a user is not a good practice.

It is not needed for dogecoin:dogecoin to have permissions on binaries installed in /usr/local/bin because we allow anyone to
execute these anyway through the chmod 4555. All the chown directive really does is give running processes like dogecoind access to these files, which the process then can chmod and subsequently overwrite.

Instead, let root own the files, but keep allowing execution by anyone, which will disallow a process spawned by the dogecoin user to change these files - this reduces potential impact of future / unknown remote code execution vulnerabilities in Dogecoin Core.

Before this PR, a process running as dogecoin:dogecoin inside the container could for example perform the equivalent of chmod 755 /usr/local/bin/dogecoin-cli && echo "#!/bin/bash\necho je suis un virus" > /usr/local/bin/dogecoin-cli. After this PR, it cannot.

@patricklodder patricklodder added the bug Something isn't working label Dec 12, 2021
It is not needed for dogecoin:dogecoin to have permissions on
binaries installed in /usr/local/bin because we allow anyone to
execute these anyway. All this does is give running processes
like dogecoind access to these files, which the process then can
chmod and overwrite.

Instead, let root own the files, but keep allowing execution by
anyone, which will disallow a process spawned by the dogecoin
user to change these files.
Copy link
Member

@xanimo xanimo left a comment

Choose a reason for hiding this comment

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

overwhelmingly ACK! after learning and confirming that running chown, chmod and mv effectively duplicate the targeted file in the underlying layer, this PR shaves off a whopping 16 MB from layer 9 as shown here (not to mention the enhanced security this applies...):

from 90.44 MB:
https://hub.docker.com/layers/xanimo/1.14.5-dogecoin/fix_docker-lint-fixes/images/sha256-000500ec19984f0aca58ab9b69c136840a90bfb06da7817560b7c90606971897?context=repo

to 74.7 MB:
https://hub.docker.com/layers/xanimo/1.14.5-dogecoin/rework_no-exec-chown/images/sha256-aee5d7afea99fd14d383a8881c37810f0c4c8905f5ab84f7bfffb47c97a4351c?context=repo

ps -- have a rework of my closed 'buildx-lean' pr that addresses the former issues mentioned while not using buildkit but it's still a draft. hopefully with push later tonight.

@patricklodder
Copy link
Member Author

Missed that setuid depends on this - doing more tests, reverting to draft.

@patricklodder patricklodder marked this pull request as draft December 13, 2021 02:14
@AbcSxyZ
Copy link
Contributor

AbcSxyZ commented Dec 13, 2021

Yes, the intent of this chown and setuid was for docker exec commands. It allowed any users who will run a dogecoin executable to run it as dogecoin user (vs run as root otherwise) without using docker exec --user dogecoin.

@patricklodder
Copy link
Member Author

patricklodder commented Dec 13, 2021

executable to run it as dogecoin user

Which means that it always runs it as the kernel's 1000:1000 (of the HOST) right now, with no way to change it. So if I'm 1001:1001 on the host, I will give wallet access to 1000:1000... This is a big issue.

(edit: my /etc/passwd inside the container puts dogecoin at 1000, not 1001 as I misread earlier, so I changed the uid:gid mentioned)

@AbcSxyZ
Copy link
Contributor

AbcSxyZ commented Dec 13, 2021

My main host user is uid 1000. I create a new user bob with uid 1001. bob start a container, has a bash as root, adduser inside the container. bob new user is now uid 1000 🤦.

I missed something with Docker permissions and I do not find an appropriate explanation...

@patricklodder
Copy link
Member Author

I'm closing this, will raise an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants