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

Ship a more secure configuration by default #31

Closed
panda2134 opened this issue Jul 31, 2024 · 17 comments
Closed

Ship a more secure configuration by default #31

panda2134 opened this issue Jul 31, 2024 · 17 comments

Comments

@panda2134
Copy link

panda2134 commented Jul 31, 2024

Currently, both the daemon and the frontend are run as root in the default configuration. This is too dangerous even for self-deployed instances. It is much better if we can get rid of root access, so that even if MCSManager is compromised, the system as a whole stays safe.

Attack model: Attacker can find ways to do remote execution with MCSManager. We need to isolate damages to the system even if this is done.

On the other hand, the source code in both web/ and daemon/ folders are writable. Even if we run MCSManager as a non-root user, the attacker can easily change the contents of the server code. It would be slightly better if we set app.js to be read-only for the user that executes MCSManager.

It will be easy to run web/app.js as an ordinary user. However, running daemon/app.js as an ordinary user would be difficult. To gain access of docker, one has to be added to the docker group. This immediately enables delegation attack -- an attacker who has shell access to host system (for instance, by compromising the daemon) will be able to modify every file on the host system as root by simply mounting / into the container. This will allow the attacker to do privilege escalation.

The solution would by user namespaces, either by using rootless docker or podman. This PR enables integration with other container runtimes like podman. With that in mind, the following changes should be made to the installation script:

  1. fix subtle incompatibilities caused by podman (e.g., changing default network name to "bridge")
  2. enable podman.sock for the user mcsm-daemon
  3. run the daemon with user mcsm-daemon while enabling related systemd hardening options.
@panda2134
Copy link
Author

I've already working on an implementation. PR should be done in a week.

@panda2134
Copy link
Author

Upstream issue on podman incompatibility found in testing: containers/podman#23453

@YuMao233
Copy link
Member

YuMao233 commented Aug 1, 2024

We have plans to improve security regarding permission issues.

MCSManager still requires root privileges to run because it does more than just run Docker images. It also supports running existing programs on the server. If we use a non-root user, it could cause a range of permission problems and increase the user's learning curve.

However, we plan to have all child processes (also called instances) started by MCSManager run with non-root privileges, including Docker containers.

All files in the instance working directory will have non-root permissions.

@panda2134
Copy link
Author

panda2134 commented Aug 1, 2024

We've addressed the permission issues with MCSManager and plan to incorporate the solution in upcoming versions.

MCSManager still requires root privileges to run because it does more than just run Docker images. It also supports running existing programs on the server. If we use a non-root user, it could cause a range of permission problems and increase the user's learning curve.

However, we plan to have all child processes (also called instances) started by MCSManager run with non-root privileges, including Docker containers.

All files in the instance working directory will have non-root permissions.

I can also help implement that. What's you current plan on dropping the root privilege for child processes?

Also, maybe it would suffice to add an option to the installation script for choosing whether existing programs will be run using root. I feel it rather safe to use a non-root user, because mostly this tool is for Minecraft (and alike) servers which listen on a port > 1024. Those who use existing tools that require su via MCSManager should be fine if they choose to enable root when installing.

@YuMao233
Copy link
Member

YuMao233 commented Aug 1, 2024

  1. First, the installation script automatically creates a Linux user named "mcsm" when installing MCSManager.

  2. Upon the first startup of MCSManager, it checks if the "mcsm" user exists in the system. If not, it will run subprocesses as the root user to maintain backward compatibility.

  3. Modifying the startup method:

    If the "mcsm" user exists, all instance subprocesses will be started as the "mcsm" user.

  4. Upgrading the MCSManager file management module:

    If a user performs file operations (such as editing, uploading, renaming) within the MCSManager file management, it will change the file permissions (because the MCSManager Daemon process is still running as the root user). Therefore, after any file operation, the chown -R command must be executed to reset the file owner to "mcsm" to ensure no access denial issues occur.

In summary, I think this involves quite a bit of work. Do you have any better ideas?

If you agree, you can implement it according to my plan. If there are any other issues, feel free to bring them up here.

If possible, you can also join our Discord, which might be more convenient.

@YuMao233
Copy link
Member

YuMao233 commented Aug 1, 2024

If you just want to create an "mcsm" user by installing the script and use this user to start MCSManager:

This shouldn't be a problem for users familiar with Linux servers, but most users will encounter a series of permission issues, such as:

  1. Unable to import existing programs on the system because they probably don't belong to the "mcsm" user

  2. The file management feature in MCSManager "Node Terminal" won't work because it defaults to accessing the root directory (maybe we should remove this feature?)

  3. Other potential issues that I'm not thinking of.

However, this development cost is the lowest.

@panda2134
Copy link
Author

panda2134 commented Aug 1, 2024

If you just want to create an "mcsm" user by installing the script and use this user to start MCSManager:

This shouldn't be a problem for users familiar with Linux servers, but most users will encounter a series of permission issues, such as:

  1. Unable to import existing programs on the system because they probably don't belong to the "mcsm" user

  2. The file management feature in MCSManager "Node Terminal" won't work because it defaults to accessing the root directory (maybe we should remove this feature?)

  3. Other potential issues that I'm not thinking of.

However, this development cost is the lowest.

This is indeed much easier to implement. In fact in my deployment I'm using this model (along with rootless containers).

Bullet point 1 would not be a problem, since most programs are accessible by normal users. Ordinary game servers do not require root privileges, and even if they do so for binding to lower ports such capability can be explicitly granted in the systemd service, avoiding root access.

The node terminal should be disabled by default. It sounds pretty scary to have a root shell on the webpage, even if we impose strong password complexity requirements.

Note that removing the node terminal feature along won't eliminate root access from web. The only way to do so would be running the service as a non-root user.

@panda2134
Copy link
Author

panda2134 commented Aug 1, 2024

Regarding the docker user issue: Docker does not use user namespaces. Instead, it uses a different /etc/passed file in the container. Thus, users inside the container and outside is not related at all, and uid in the container may not be valid outside. This will also cause issues when running with mcsm user in the container if we continue using docker as-is.

@YuMao233
Copy link
Member

YuMao233 commented Aug 1, 2024

This is indeed much easier to implement. In fact in my deployment I'm using this model (along with rootless containers).

Bullet point 1 would not be a problem, since most programs are accessible by normal users. Ordinary game servers do not require root privileges, and even if they do so for binding to lower ports such capability can be explicitly granted in the systemd service, avoiding root access.

The node terminal should be disabled by default. It sounds pretty scary to have a root shell on the webpage, even if we impose strong password complexity requirements.

Note that removing the node terminal feature along won't eliminate root access from web. The only way to do so would be running the service as a non-root user.

If you're just aiming to have the setup script automatically create and use the "mcsm" user, I think someone's already helped me get that done. I'm just still on the fence about whether or not to use it.

This is a brand new installation script, but we haven't officially used it yet.

echo " -u Specify the user (mcsm or root), default is 'mcsm'"

@panda2134
Copy link
Author

panda2134 commented Aug 1, 2024 via email

@YuMao233
Copy link
Member

YuMao233 commented Aug 2, 2024

I actually wanna add installation and configuration of podman into the script

I do not agree with this idea, nor is it necessary.
If other users also need to use podman, just specify in the documentation how to make MCSManager use podman.

@panda2134
Copy link
Author

panda2134 commented Aug 2, 2024 via email

@YuMao233
Copy link
Member

YuMao233 commented Aug 6, 2024

明白了。但至少我们应该考虑合并使用 mcsm 用户运行守护进程的 PR,或者允许用户选择是否使用 root 运行守护进程。除此之外,当使用 mcsm 用户时,我们最好禁用与 docker 相关的东西(同样,默认情况下),因为任何使用 docker 都可能允许该用户的权限提升。如果用户 mcsm 不在组 docker 中,docker.sock 无论如何都无法使用。实现这一点的一个好方法是询问您是否正在为其他用户商业运行该工具。如果不是,我们避免使用 root 并隐藏与 docker 相关的东西。我同意你的观点,与无根容器相关的内容应该放在文档而不是脚本中。2024 年 8 月 2 日星期五 16:47,Unitwk @.> 写道:我实际上想将 podman 的安装和配置添加到脚本中我不同意这个想法,也没有必要。如果其他用户也需要使用 podman,只需在文档中指定如何让 MCSManager 使用 podman。— 直接回复此电子邮件、在 GitHub 上查看或取消订阅。您收到此邮件是因为您创作了该主题。消息 ID:@.>

Merged

@YuMao233 YuMao233 closed this as completed Aug 6, 2024
@huangsijun17
Copy link
Contributor

Got it. But at least we should consider merging the PR that uses mcsm user to run the daemon, or allow the user to choose whether root should be used for running the daemon. Along with that we might better disable docker-related things when mcsm user is used (again, by default), since any use of docker may allow privilege escalation from that user. if the user mcsm is not in group docker, docker.sock will not be usable anyway. A good way of implementing this would be asking if you’re running the tool commercially for other users. If you’re not, we avoid using root and hide docker-related things. I agree with you that the things related to rootless container should go into the docs instead of the script. On Fri, Aug 2, 2024 at 16:47, Unitwk @.> wrote: I actually wanna add installation and configuration of podman into the script I do not agree with this idea, nor is it necessary. If other users also need to use podman, just specify in the documentation how to make MCSManager use podman. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.>

Even if it is not a commercial scenario, using Docker is still an excellent solution for coexistence of multiple versions of MC. If you want to open multiple MC servers of different versions at the same time, you will encounter the problem that they have different requirements for Java versions. The Docker used is a simple, fast and stable solution for coexistence of different versions of Java.

@panda2134
Copy link
Author

panda2134 commented Aug 6, 2024 via email

@panda2134
Copy link
Author

panda2134 commented Aug 6, 2024 via email

@KevinLu2000
Copy link
Member

As @unitwk mentioned, the current script already supports installation using a general user "mcsm" and this is the default behavior. It also supports upgrading to the latest version (9 or 10).

In my opinion, MCSM, as a control panel program, will likely need sudo/root permissions at some point (e.g., managing Docker, system services, or permissions for files owned by other users). In that case, our goal should be to minimize the use of root permissions when possible. But this would require significant coding and restructuring of the entire architecture, and would make the program unnecessarily complex.

I personally would vote for the default installation using user 'mcsm' (potentially with Docker disabled as @panda2134 suggested). For experienced users who can manage Docker correctly, using root is probably just fine. For others, a generic user should be sufficient for most cases and also provides better security.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants