Xrdp as unprivileged user #2974
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matt335672
force-pushed
the
xrdp_as_unprivileged_user
branch
from
e7d906f
to
2fa9520
Compare
|
Thanks! I'll look into this soon. |
This was referenced Mar 21, 2024
matt335672
force-pushed
the
xrdp_as_unprivileged_user
branch
from
2fa9520
to
a7fdc27
Compare
|
Overall, LGTM. Isn't xrdp-chkpriv script installed? Also, we need a check for |
runtime_user and runtime_group are added to the xrdp.ini file so that the service knows how to reduce privilege
- xrdp_listen.c is refactored so we can create the listening socket(s) before dropping privileges. - The code which reads startup params from xrdp.ini is moved from xrdp_listen.c to xrdp.c, so it is only called once if we test the listen before starting the daemon.
Now we have g_file_open_rw() we don't need to try to write to the PID file to see if we can. Just leave the file open and write to it after forking.
If xrdp is running with dropped privileges it won't be able to delete the PID file it's created. Places where xrdp is stopped need to cater for this. It's prefereable to do this than make the PID file writeable by xrdp with dropped privileges, as this can still lead to DoS attacks if an attacker manages to modify the PID file from a compromised xrdp process.
|
Thanks for that. I've just rebased to latest devel, built the software and installed to then:- so the script is installed, but in There's a privilege check for $ sudo /usr/local/share/xrdp/xrdp-chkpriv [sudo] password for mjb: Settings - [xrdp.ini] runtime_user : xrdp - [xrdp.ini] runtime_group : xrdp - [xrdp.ini] certificate : /etc/xrdp/cert.pem - [xrdp.ini] key_file : /etc/xrdp/key.pem - [sesman.ini] SessionSockdirGroup : xrdp -Info- runtime_user 'xrdp' appears to exist -Info- runtime_group 'xrdp' appears to exist -Info- xrdp.ini and sesman.ini agree on group ownbership -Info- /etc/xrdp/rsakeys.ini has correct permissions -Info- /etc/xrdp/cert.pem is readable by xrdp:xrdp -Info- /etc/xrdp/key.pem is readable by xrdp:xrdp -Summary- Permissions appear to be correct to run xrdp unprivileged Is that what you were expecting, or should I add something else? I'll re-check the rebase on FreeBSD 14. |
matt335672
force-pushed
the
xrdp_as_unprivileged_user
branch
from
a7fdc27
to
48255da
Compare
The unprivileged user needs to be able to read the certificate and key files to offer TLS, but should not be able to write to then. This commit checks the TLS files are read-only, rather than simply readable
|
I've just added another check to xrdp-chkpriv, which is that the TLS files are readable but NOT writeable by the unprivileged user:- $ sudo chmod 660 /etc/xrdp/cert.pem $ sudo ./xrdp-chkpriv Settings - [xrdp.ini] runtime_user : xrdp - [xrdp.ini] runtime_group : xrdp - [xrdp.ini] certificate : /etc/xrdp/cert.pem - [xrdp.ini] key_file : /etc/xrdp/key.pem - [sesman.ini] SessionSockdirGroup : xrdp -Info- runtime_user 'xrdp' appears to exist -Info- runtime_group 'xrdp' appears to exist -Info- xrdp.ini and sesman.ini agree on group ownership -Info- /etc/xrdp/rsakeys.ini has correct permissions -Error- /etc/xrdp/cert.pem is writeable by xrdp:xrdp -Info- /etc/xrdp/key.pem is read-only for xrdp:xrdp -Summary- 1 error(s) found. Please correct these and try again $ sudo chmod 640 /etc/xrdp/cert.pem $ sudo ./xrdp-chkpriv Settings - [xrdp.ini] runtime_user : xrdp - [xrdp.ini] runtime_group : xrdp - [xrdp.ini] certificate : /etc/xrdp/cert.pem - [xrdp.ini] key_file : /etc/xrdp/key.pem - [sesman.ini] SessionSockdirGroup : xrdp -Info- runtime_user 'xrdp' appears to exist -Info- runtime_group 'xrdp' appears to exist -Info- xrdp.ini and sesman.ini agree on group ownership -Info- /etc/xrdp/rsakeys.ini has correct permissions -Info- /etc/xrdp/cert.pem is read-only for xrdp:xrdp -Info- /etc/xrdp/key.pem is read-only for xrdp:xrdp -Summary- Permissions appear to be correct to run xrdp unprivileged It's rather obvious, looking back on it now. |
|
See also https://github.com/neutrinolabs/xrdp/wiki/Running-the-xrdp-process-as-non-root which I've updated in anticipation of merging this. |
Okay, I see. |
metalefty
approved these changes
Jul 2, 2024
metalefty
reviewed
Jul 12, 2024
| # mode | ||
|
|
||
| # Change these if they do not match your installation | ||
| CONF_DIR=/etc/xrdp |
There was a problem hiding this comment.
@matt335672 We should use @sysconfdir@/xrdp for the default location. It will be set to /usr/local/etc/xrdp on FreeBSD.
metalefty
added a commit
to metalefty/xrdp
that referenced
this pull request
Jul 12, 2024
While here, ignore build artifacts of chkpriv tools. Follow-up to: neutrinolabs#2974
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2965
Parameters are added to xrdp.ini and sesman.ini to allow running as a non-privileged user.
The following issues were found in implementing this:-
PID files either need to be made alterable and deleteable by the non-privileged user, or just left to be deleted elsewhere. It seems current best-practice is the second of these. I've gone with that approach.
A problem was found with Update sockdir security #2731 which is fixed in commit 4471012. This should probably be factored out as a separate PR.Edit 2024-3-22: See Fix permissions on user socket directory #3011We know it's tricky to get permissions right for non-privileged xrdp - there's a constant theme of users who don't understand why TLS doesn't work out-of-the-box on Debian/Ubuntu (see Debian BTS #860890). To address this I've provided a script which checks file permissions are correct when the parameters in xrdp.ini are entered or changed. This is referred to from the xrdp.ini manpage.
By default, xrdp runs as root. User interaction is required to address this. I've done this more for porting than anything else.
I've completed initial testing on Ubuntu and FreeBSD. The permissions check script may not work on some more oddball systems like Alpine Linux.
This is a complex area and I may well have omitted something. Feedback most welcome!
BTW: As I write this, Github CI is broken. Hopefully it will be working by Monday.