Commit 61585fac priv check bug (and breaks extension)

bacon-cheeseburger commented 1 month ago

Is there already an issue for your problem?

[X] I have checked older issues, open and closed

NZBGet Version

v24.3-testing

Platform

Linux/Docker

Environment

Debian Sid 64bit
Compiled git master 24.3-r2492-git+9657e6ee

Current Behavior

With commit 61585fac12e697baafa547012ed2970135de687f applied, the extension I use that auto-unzip's .nzb's in the monitored incoming dir no longer works.

Expected Behavior

I expect the .zip's to be unpacked, as was the case prior to this commit.

Steps To Reproduce

Compile git from the 61585fac forward.
Enable an auto-unzip script (I'm specifically using https://github.com/Prinz23/nzbgetpp)
Run nzbget, drop a .zip contained .nzb's into your incoming dir and wait for nothing to happen.

Logs

No response

Extra information

I run nzbget in a network namespace so it's started with sudo privs but I also set DaemonUsername to the user I want the privs dropped to (which I've confirmed occurs). If I clone git master and revert 61585fac, the unzip extension works again as expected.

dnzbk commented 1 month ago

Yes, it implicitly worked in your case, but it is still a bug and a potential security risk that should have been fixed much earlier. In your case, you can try adding your DaemonUsername to the Network group and ensure that DaemonUsername which drops zip files have umask 0002.

bacon-cheeseburger commented 1 month ago

Yes, it implicitly worked in your case, but it is still a bug and a potential security risk that should have been fixed much earlier. In your case, you can try adding your DaemonUsername to the Network group and ensure that DaemonUsername which drops zip files have umask 0002.

The user is a member of the network group. The umask setting was 1000 but I changed to 0002 per your suggestion and still got the same broken behavior.

@bket Any ideas why this is happening?

bket commented 1 month ago

Any ideas why this is happening?

@bacon-cheeseburger, not really. Just to be sure: can you confirm that DaemonUsername has permission to auto-unzip a .nzb in the specific directory? Could you share ownership and permissions of the dir, and confirm that DaemonUsername is owner or is in the right group?

You are running in a docker, right? Is the docker user mapped to the host user?

bacon-cheeseburger commented 1 month ago

@bket Your patch splits InitOptions() in daemon/main/Options.cpp and creates a separate CheckDirs(). If I add a CheckDirs(); to the beginning of the new InitOptions() to mimick the pre-split behavior then everything works again. Could this have been an oversight?

bket commented 1 month ago

No, that is not an oversight. Previously, nzbget would create specific dirs before dropping privileges. This would result in DaemonUsername being unable to write to those specific dirs (which are owned by root). In the current situation, nzbget drops privs to DaemonUsername, and then creates the dirs (owner.will be DaemonUsername).

Could you have a look at the questions in my previous reply. It seems that this is related to permissions.

bacon-cheeseburger commented 1 month ago

No, that is not an oversight. Previously, nzbget would create specific dirs before dropping privileges. This would result in DaemonUsername being unable to write to those specific dirs (which are owned by root). In the current situation, nzbget drops privs to DaemonUsername, and then creates the dirs (owner.will be DaemonUsername).

Could you have a look at the questions in my previous reply. It seems that this is related to permissions.

User is "nzb". My import dir is samba-shared so I can dump .nzb/.zip(ped nzbs) into it from a different box (using nzb user credentials): drwxr-sr-x 2 nzb nzb 516096 Aug 15 09:20 import

Creating a file from another box using nzb credentials: -rw------- 1 nzb nzb 0 Aug 15 09:24 test.zip

The user is valid, owns the dir (which is defined at Paths->NzbDir in nzbget settings), has privs and can read/write to that dir using user credentials. All dirs defined in Paths already exist and are owned by the user so nzbget shouldn't have to create anything, right? I do not use a docker but am using a dedicated network namespace which nzbget runs in. Without this commit privs are dropped and everything written is correctly as the user. With the commit, privs are dropped, .nzb's are queued, it's only .zip's that aren't processed anymore by the extension. Rather than a permission issue, could this be a placement/race issue related to extensions?

bket commented 1 month ago

@bacon-cheeseburger, I'm in the dark here. As you were able to pinpoint 61585fa as culprit, more specific the CheckDirs() bit, it would make sense to either revert the whole commit or undo the `CheckDirs()' bit. The first is a bad idea as it reintroduces a security risk. The second would mean that directories, which are needed upon at start and not present, are made with root permissions,which is a nuisance but previously accepted behavior and not a security risk.

@dnzbk, what do you think? I can open a PR for reverting part of 61585fa.

dnzbk commented 1 month ago

@bacon-cheeseburger, the issue should be resolved now. Thanks for the report and the assistance.

@bket, I put back dirs checking as it was before, thank you. Yes, just like you said, there is a problem now in missing directories needed at startup created by root even if DaemonUsername is not valid - if we could find a way to fix that too, that would be great.

nzbgetcom / nzbget