Pugmatt / BedrockConnect

Join any Minecraft Bedrock Edition server IP on Xbox One, Nintendo Switch, and PS4/PS5
GNU General Public License v3.0
1.45k stars 164 forks source link

Remove shell and system from useradd #308

Closed NotoriousPyro closed 2 years ago

NotoriousPyro commented 2 years ago

What happened?

Having a --shell set to something valid like /bin/bash and --system for the user running the service is insecure. If BedrockConnect service is vulnerable, someone could use it to escalate privileges on the system with a valid shell.

The mcbc user can be created without --system and with --shell /dev/null to ensure nobody can login to it. If someone needs to modify files, they can escalate using sudo or by adding their user to mcbc group.

https://github.com/Pugmatt/BedrockConnect/blob/ffe10054a959cb976d551a9fb6d431d103431056/scripts/install-service.sh#L36

Suggesting this script change, in particular:

$ADDUSER \
   --shell /dev/null \
   --user-group \
   --password "$(dd bs=36 count=1 if=/dev/urandom 2>/dev/null | base64)" \
   --home /home/mcbc \
   --create-home \
   mcbc

The service can still be started with this:

craig@minecraft:~$ journalctl -fu bedrock-connect.service
Jul 31 20:07:22 minecraft systemd[1]: Started Bedrock Connect.
Jul 31 20:07:22 minecraft java[2666]: -= BedrockConnect =-
Jul 31 20:07:22 minecraft java[2666]: Server Limit: 100
Jul 31 20:07:22 minecraft java[2666]: Port: 19132
Jul 31 20:07:22 minecraft java[2666]: Loaded 0 custom servers
Jul 31 20:07:23 minecraft java[2666]: Bedrock Connection Started: 0.0.0.0:19132

Excepted Behaviour?

The script should not create a system user with a valid shell to run the jar file.

Steps to reproduce.

  1. Run the script.
  2. Observe the user is a system user with a valid shell.

Screenshots/Videos

No response

Minecraft Bedrock Version

No response

Console

No response

Additional Context

No response

jdextraze commented 2 years ago

LOL insecure. It has a 42 characters random password to login. Also --shell is for the login shell, if the BedrockConnect service is vulnerable setting the shell to /dev/null doesn't prevent it from running anything it wants under that user.

Also I was wondering, are you running your Bedrock Connect in a bank or government facility?

If you still want it to be Fort Knox, why not do a PR instead of showing off your security skills.

NotoriousPyro commented 2 years ago

If you knew what you were talking about you'd appreciate the submission, clearly you do not so you should avoid commenting.

Redirecting to /dev/null prevents login via ssh using mcbc user... Derp

NotoriousPyro commented 2 years ago

Security is about giving least access needed to do something. Running as system with a valid shell does neither of these. And FYI, I work for a company who has major clients, my network would not afford running something as system or with shell perms... Good job I read the script before running it eh?

kmpoppe commented 2 years ago

Guys, could you please stop insulting and patronising oneanother? This is neither the right time threat-wise nor the place to do so.

Security concerns are always valid and need to be looked at, it makes no difference if anyone personally feels it would be too much of a hassle or not valid in their scenario.

NotoriousPyro commented 2 years ago

Agreed @kmpoppe, I raised this with a valid concern I see no need to be insulting.

jdextraze commented 2 years ago

Still waiting for that PR...

NotoriousPyro commented 2 years ago

Still waiting for that PR...

Issues are there to track issues, not necessarily result in PRs. When you're ready to engage in normal adult debate about real issues (PoLP), feel free to rejoin the conversation.

You joined the conversation with an incorrect response. If what you said was even remotely correct, we might as well run everything on a computer in ring 0 (kernel mode).

jdextraze commented 2 years ago

You are making this very difficult with your edits on last post but ok no more arguing and just talk about this issue... Let's talk about it seriously:

Sorry... haaa ring 0 that reminds me when I studied the kernel and wrote one as an exercise to understand threading at a CPU level ;)

NotoriousPyro commented 2 years ago
  • Empty shell uses /usr/sbin/nologin or /bin/false not /dev/null

And if someone replaces sbin/nologin with a valid shell, then what? /dev/null can never be a valid shell, it is by far the most secure way, while sbin/nologin is in a binary directory... How do you expect NULL to ever be a valid shell? Have you even tried to do this? You'll get a permission denied if you try to login with /dev/null

  • We might want to remove that random password if login is not possible

This is irrelevant.

  • Using --system puts the user with a lower uid/gid (depends on system, but Ubuntu is usually 100-400) and this is what we want for a service

This is irrelevant and not needed for a process running in user space and doesn't appear to touch any system areas, e.g. system directories, and even runs from its own user-space directory /home/mcbc.

  • if we do remove --system then --create-home is redundant

No, it is not, read the above.

Sorry, can't take any of your response seriously. Please read up a bit more on security.

jdextraze commented 2 years ago

Wow. Clearly I've touched a nerve. I'm not going to waste anymore time on this. You're a GOD, you WIN. Now can you please do that PR because clearly no one else can do it.

jdextraze commented 2 years ago

BTW my point hidden behind all this sarcasm was that this is way overkill for a BedrockConnect server but I agree with @kmpoppe that this might not be valid for me but might be for others like @NotoriousPyro . So have a nice day.

Pugmatt commented 2 years ago

Let's keep the issue threads civil guys, please. None of this toxicity was relevant or productive. Both of you have valid concerns, so it just made everything harder to decypher and painful to read in an otherwise helpful thread.

Back to the original topic at hand: I performed some tests on my end with the service creation script and @NotoriousPyro 's proposed script changes, and I did not notice any issues or BedrockConnect functionality changes when the added user wasn't a system user, so having it be a system user is probably not needed here as you said. That doesn't make the original script bad, just means like most things in programming we can always make things more secure.

Is it possibly overkill for this project? It can be seen that way, especially since the main BedrockConnect server 104.238.130.180 does not use the scripts under the "scripts" folder. (Unsure about other servers under "Publicly available BedrockConnect instances" in the README) But, I don't think that means these security concerns aren't helpful to point out and discuss, even if small.

I've gone ahead and implemented the original proposed changes by @NotoriousPyro . If anyone has any further concerns, or catches any malfunctioning functionality from the software with this change, please state it here, politely. Maybe it's overkill, but still good to have these things more secure either way.

Thank you both for your input on this. Closing this for now, but feel free to continue this if needed. But, please keep any further replies in this thread civil. 🙂

NotoriousPyro commented 2 years ago

carl-sagan-youre-awesome

jdextraze commented 2 years ago

That's fine. I would have used what other services do which you can see by looking at /etc/passwd or this example: https://github.com/aiven/pglookout/blob/main/debian/postinst

NotoriousPyro commented 2 years ago

I think that's because pglookout writes to /var/lib which only root can write to... Anyway I'm not really sure what your problem is since the issue has been resolved re the above, but have a great day.

Pugmatt commented 2 years ago

I think that's because pglookout writes to /var/lib which only root can write to... Anyway I'm not really sure what your problem is since the issue has been resolved re the above, but have a great day.

I don't think he was trying to continue fighting, just showing his side and I think that link was worth bringing up. But yes that project linked is probably using --system because of what you said.

jdextraze commented 2 years ago

I am trying to learn. Like what's the link between root and --system? I can't find anything in the Linux man page or online (was reading this: https://www.cyberithub.com/system-users-and-human-users-in-linux-explained-with-examples/) about that... Any good link?