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

targetcli as_root check is not sufficient #163

Open
alexmurray opened this issue Mar 23, 2020 · 5 comments
Open

targetcli as_root check is not sufficient #163

alexmurray opened this issue Mar 23, 2020 · 5 comments

Comments

@alexmurray
Copy link

If a command is to be disabled for non-root users, having targetcli check this via getuid() is not sufficient, since a local user could just make a copy of the targetcli python script and edit it to remove this check - instead targetclid should check the client's credentials via SCM_CREDENTIALS and do permissions checks itself against the client and return appropriate errors.

@gonzoleeman
Copy link
Contributor

targetclid is not required to be running.

@gonzoleeman
Copy link
Contributor

And the permissions on sysfs should require root, anyway. No?

@pkalever
Copy link
Contributor

And how is this different from targetcli script (dameon=false) itself?

@alexmurray
Copy link
Author

Whilst targetclid is not required to be running, the systemd unit and socket unit mean that as soon as any client tries to connect to the daemon socket it will be spawned. The daemon is then running as root and so has full permissions to modify sysfs etc - so any commands sent to it will be executed as root - even if the cient does not have root privileges. Hence the daemon should check the UID of the client to know if it has authority to perform the action before doing it on it's behalf (otherwise this is a classic confused deputy problem) - failing this, at least addresing issue #162 would ensure non-root privileged clients cannot connect to the daemon and so this would then not be necessary to validate the permissions of the client.

@pkalever
Copy link
Contributor

ack.
Fix: #164

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

3 participants