Update sockdir security #2731
Merged
Update sockdir security #2731
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
update_sockdir_security
branch
from
61f65b1 to
6d1ad83
Compare
The sockdir is only used when sesman is active. The call g_mk_socket_path() is removed from os_calls and moved to sesman. We also change the permissions on this directory to 0755 rather than 01777 (01000 is the 'sticky bit', S_ISVTX). The behaviour of g_create_dir() has been modified to not set S_ISVTX on Linux directories. This is implementation-defined behaviour according to 1003.1, and is no longer required for the sockdir.
matt335672
force-pushed
the
update_sockdir_security
branch
2 times, most recently
from
4736271 to
0971b3e
Compare
|
I'm bumping this up priority-wise as it's necessary to get the security of the PCSC socket sorted for #1825 If nobody's got time to look at it I'll merge in a week or so. I think the idea of what it's trying to do is fairly clear. |
matt335672
force-pushed
the
update_sockdir_security
branch
2 times, most recently
from
fc59fd4 to
398ba41
Compare
The top level socket directory is now called XRDP_SOCKET_ROOT_PATH. Below that are user-specific directories referred to with the XRDP_SOCKET_PATH macro - this name is hard-coded into xorgxrdp and the audio modules as an environment variable. XRDP_SOCKET_PATH now looks like $XRDP_SOCKET_ROOT_PATH/<uid> XRDP_SOCKET_PATH is only writeable by the user, and readable by the user and the xrdp process.
Now we've made the XRDP_SOCKET_PATH only writeable by root, it's safe to move the sesman socket back into this directory. We no longer need a separate sesmanruntimedir
matt335672
force-pushed
the
update_sockdir_security
branch
from
398ba41 to
c51ec2e
Compare
|
I will look into it. |
|
Thanks. I'll merge later today if I get the time. |
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.
This is one of two updates which (in total) will support running xrdp as a non-privileged user without needing to patch it, as Debian/Ubuntu does at present.
The main purpose of this update is to remove the need to use the sticky bit on the socketdir and simplify the security in this area.
#2472 changed sesman to identify sessions by UID rather than username. This fixed #1823. As a result, sesman is now UID aware.
This lets us insert a UID-specific directory into the socketdir. Now users can't see each others xrdp sockets, resulting in a small security improvement.
Once we've done this, we can get rid of the sesmanruntimedir introduced by #2472, as the socketdir is no longer world-writeable. This moves all the sockets back into a single directory.
As an example, if I log on to my development box using display 11, I now get this under
/run/xrdp:-At the moment the xrdp process is running as
root, and can access sockets for user 1001 (testuser) by using group permissions.There are also some tidy-ups in here, like introducing
XRDP_SOCKETS_MAXPATHfor the maximum length of a socket path name, and simplifying the parsing of the sesman.ini[security]section.Currently draft as more testing is required. Comments welcome.