LuckPerms

LuckPerms

41.4k Downloads

Don't allow relative paths for files in the /lp import/export commands

Archengius opened this issue · 7 comments

commented

This opens severe security hole if person manages to abuse permissions somehow. That way he can dump file contents at access of user who is running LP server.

commented

I disagree that this is a critical issue. An issue perhaps, but critical, I don't think so.

I'll address both parts separately:

The /lp import command has basically no attack vector - except from maybe verifying that some file does exist somewhere on the system. A user wouldn't be able to dump or read file contexts, unless by coincidence the file happened to contain lines starting with /luckperms .... All other lines are filtered out.

The /lp export command will not overwrite existing files.

commented

There are two things I’d like to add to this:

There’s practically no way to “fix” this “issue” other than to have the commands be protected by permissions. If you have an alternative solution, let us know as that would be very interesting.

Next the commands are protected through permissions. Meaning it’s the users (server owner/admins) responsibility to make sure nobody that isn’t supposed to use these commands has the permissions to do so. And unless you can provide a way to bypass that check that is not caused by an improper setup (like unprotected backend servers) there really is no concern. Like the root user having access to everything on a Linux system isn’t a security issue. As the server owner needs to make sure nobody that shouldn’t have access to it has access to it.
You can’t blame the system granting access to sensitive information if you are responsible for not restricting said access in the first place.

So I’d even go a step further and say it’s not even an issue at all, as the command can only be used when granted permission to it.

commented

lp import tries to scan file, dumping "errored" lines into the chat.

commented

it can be used to expose sensitive information. For example:

/lp import /root/.ssh/id_rsa

it will dump entire file contents into the chat, because it has "errors"

commented

I don't think this is what it should do. Instead, it should report line and column of error, rather than dumping raw file contents into the game chat.
Current way, if user bypassed permissions system somehow, it can ruin not only server, but also a host that runs it. And it's all would be caused by LP.

commented

Addressed in the above commit.


it can be used to expose sensitive information. For example:
/lp import /root/.ssh/id_rsa

It shouldn't be that simple. Hopefully, you're a) not using the root user, b) not storing your private key on a remote machine and c) not running your server as root.

My point is, it would require a string of security mistakes, including an unauthorised user gaining access to such commands, for the issue to be possible. It's not like anyone can just log into your server and immediately have access to the whole system.

Filesystem permissions should prevent the server process (and indirectly LuckPerms in the example you give) from reading system/private files.

commented

Regardless, I still consider this an issue - thank you for bringing it to my attention. :)