@iskunk - first thing I should say is this is a great idea, and we should definitely get something in to support this.

My biggest concern is support.

For SELinux (which is only really used by Fedora/RHEL and derivatives), we ship xrdp as SELinux-ready, and leave it to the distribution to configure. There are two reasons for this:-

1. We're not in a position to dictate to the distros how they should use the product.
2. We're also not in a position to support the testing effort which would be required to support multiple distros using this. You quite rightly allude to testing above. We don't have any AppArmor experience on the team I'm aware of - I've been a RH admin in the past, so I'm fine with a subset of SELinux issues, but not all. So we're not really in a position to fully support this in the base product. That could lead to a worst-case where we ship this and then can't maintain it. No-one would be happy with that.

What I would be happy with is getting the product AppArmor ready so that either the distros can use it by adding profiles, or securlty-conscious users with the experience to admin this could easily get it configured. Shipping example profiles would be OK I think, provided they were basic.

Anyone else want to comment on this? I'd really like to find the most productive way forward on what could be a great feature.

PR update pushed. A few notes:

• g_set_no_new_privs() could use some way of checking whether the process is running in a SELinux context (as NoNewPrivileges could interfere with that as well), but I don't know enough about that world to implement this correctly.

• I did away with the #define PR_SET_NO_NEW_PRIVS 38 bit. I see that it dates from 2016, in the original commit that added the NoNewPrivileges call. I don't think it should be needed any more.

• I moved the NoNewPrivileges transition up to the beginning of the X server child block. This is so that the AppArmor check inside g_set_no_new_privs() occurs before the env_set_user() call, so that the /proc file (which is owned by root) is opened by a process still running as root. This allows the addition of a same-user restriction to the AppArmor rule granting access to the /proc file.

  This also has the side effect of running all the different types of X servers under NoNewPrivileges (when unconfined by AppArmor), not just Xorg. I tested that this isn't a problem for Xvnc, and I don't imagine it should be one for any of the others.

• The commit that renames g_file_open() was produced mechanically, by running perl -pi -e 's/\bg_file_open\b/g_file_open_rw/g' on all source files and then reviewing manually, to avoid error.

• It wasn't clear if you also wanted to rename g_file_open_ro() back to g_file_open(), making RO access implicit at the same time that RW access is explicit. If this is the case, then let me know, and I'll add another commit with that change.

Some comments re support:

• Shipping AppArmor profiles would give distros a clear starting point for customization, and a clear owner/target for upstreaming their changes. I don't think it would amount to dictating usage, since there is a presumption that the project is willing to accommodate reasonable variations in how the product is deployed. (E.g. if a distro wants to put the binaries under /Programs/Xrdp/0.9.19/bin/ instead of /usr/{bin,sbin}/, then while /Programs/ would not fly in a PR, they might contribute some additional parameterization to facilitate it.)

  Perhaps the situation is different with SELinux, due to the complexity of the rulesets?

• Testing this is less a matter of AppArmor domain expertise, and more one of just being able to run xrdp through all the unusual and "creative" use cases that smaller groups of users rely on. It is not unlike the problem of code-coverage testing. Running through those odd use cases generates accesses that may not have been covered in the profile, possibly leading to breakage. Finding the precise modes of such breakage then allows the profile to be revised to allow those accesses.

• This form of testing is not unique to AppArmor, of course---the same is needed when developing an SELinux ruleset, or seccomp filter, or possibly other forms of security confinement. If there is not already some way of running through the odd use cases automatically, in a way that can be packaged into a CI test, then I think there would be a lot to gain in developing one.

• For what it's worth, AppArmor profiles are easier to grok than SELinux rulesets, due to being conceptually simpler (and less comprehensive). It's definitely not the same animal.

Fixed a thinko in sesman/session.c; should have used foo != 0 instead of !foo. This caused an error message to be logged on success.

Thanks @iskunk

I've enabled the CI on the above, part of which is a code formatting check to see if the code more-of-less complies with the coding standard on the wiki. As a hint you can run the scripts/run_astyle.sh script before pushing.

You raise an interesting point regarding SELinux which AFAIK doesn't need to disable NNP. It's been a while since I've looked at this though, and I'm not sure I can trust my memory. I'll go away and do a bit of research and get back to you.

The NNP disable test isn't needed for SELinux - the SELinux policy included with Fedora allows a process with NNP set to pivot to the SELinux unconfined_t process type which is all that is needed.

While it would be great to have scenario-based testing for releases, we don't have any at the moment - just unit testing for new features. So given we can't regression-test these profiles easily, I don't think we can enable them as part of the base product. I'd b happy to ship examples which the destros could use as a basis - I think the upstreaming point you make is a good one.

Updated the code formatting, used != 0 on the g_strcmp() call for clarity, and rebased.

As I understand it, NNP generally prohibits security transitions on a process because there is no easy way to verify that the process permissions afterward are a subset of those before. However, there is an exception to this when transitioning from the unconfined state, because security restrictions are never going to increase the permissions of an unconfined process.

I think that's probably what's going on with the Fedora SELinux config. I suspect that if they were to take an approach similar to the one here (confine both xrdp-sesman and Xorg, but with different rulesets, thereby requiring a transition when exec'ing Xorg), then skipping NNP would likewise become necessary.

Please let me know if there's anything else you'd like to see polished further.

The default Fedora SELinux policy is as basic as you can imagine. Essentially the daemons run as process type unconfined_service_t, where they have a few restrictions on what they can do, and the session and the X server run as unconfined_t (even fewer restrictions) which is intended for interactive user sessions only.

Regarding your comment on startwm.sh, on Fedora this runs as unconfined_t as a standard user, which is in-line with how interactive sessions are treated by default.

If particular shops want to introduce a more restrictive policy, by running xrdp through their own use-cases, they are welcome to do so, and can do so by replacing the xrdp-selinux RPM. Shops which don't want to user SELinux, for whatever reason, do not have to install the xrdp-selinux RPM. AAUI, that's from the Fedora packaging guidelines.

Thanks for your changes, and the work youve put into this. I'll have a good look and a think when I've got a bit more time than I have today.

I've got two significantly larger comments in addition to the minor one above.

The first is that AppArmor support is not supported on all our target platforms (some Linux, but also notably FreeBSD), and so needs to be enabled at compile time. That's pretty simple, and I even worked out a patch for it (attached).
patch.txt

The second is a bit more of a problem, possibly a showstopper. I was looking at substituting variables in the apparmor.d instfiles. The substitution is easy enough, and renaming the files to match whatever sbindir is set to is also doable. The problem is that the files themselves are way too specific to Debian/Ubuntu in their current form.

There's a bunch of stuff in there that we have no real control over, including but not limited to:-

• Paths to X servers
• Paths to TLS cert + key
• Paths to stuff in user XDG_DATA_HOME.
• Stuff that the X server does with dbus.

I think these are going to have to be factored out into Distro-specific files somehow (which will not be maintained by us), and it's not at all clear to me how we can do that.

Re AppArmor support on target platforms: Is a configure option really necessary? The AppArmor proc-file check only occurs if HAVE_SYS_PRCTL_H and PR_SET_NO_NEW_PRIVS are defined; presumably this will only be the case on Linux. On Linux systems that are not using AppArmor, the g_file_open_ro() call will fail harmlessly.

(Also, --enable-apparmor would imply that xrdp is using the AppArmor backend library, which is way more integration than I think either of us want)

Re distro-specific bits, yes---the profiles I've submitted are particular to Debian/Ubuntu, as that is what I'm able to provide. Generalizing them to cover other distros will need contributions from those users.

The bits that are outside of our control should, in theory, be addressed by the #include <abstractions/...> lines. I made use of those as much as I could, but there were still a few required permissions that they did not cover.

However, while the profiles may not be as simple as they ought to be, the fact that they (AFAIK) exhaustively list the permissions required for the programs to run is useful. It means that a packager need only delete permissions that are made redundant by newer abstractions (or edit paths on existing permissions to match distro conventions) instead of having to hunt for new, unknown permissions. And permissions relating to XDG_DATA_HOME and DBus shouldn't need to be touched at all, as those elements are standardized across distros by freedesktop.org.

And of course, if users contribute details of the modifications that are needed to adapt the profiles to their systems, then those can be rolled in so the profiles are ready to use out-of -the-box. To give a simple example, where Debian/Ubuntu use /etc/ssl/certs/ssl-cert-snakeoil.pem, another distro might use /etc/certs/snakeoil.pem. That wouldn't even need to be substituted; the profile could safely grant read access to both. (It's unlikely that any system would use both directories, and desire to limit access to only one.)

I believe the configure option is necessary. We've made a decision as a project to make such things explicit, so that xrdp -v is always informative, and so that distros don't have to pollute their packaging scripts by removing unnecessary stuff for them. AppArmor is by no means a universally supported LSM.

I'll try to be a little bit clearer about my profile concerns. We really don't have the effort available on this project to spend time fixing AppArmor profiles which aren't quite right for users who have made changes to their systems without having the competence to consider the consequences. We've all been there as users ourselves, and I'm happy to help many newbies here (as my posting record will testify). It is however important that anything we merge here doesn't impact our existing maintenance burden, which we are actively trying to reduce at the moment (with over 300 outstanding issues).

It follows that anything we ship has to be bullet-proof, either because it's been fully tested by us, or because the distros have decided it's a feature they're happy to enable and make bulletproof themselves.

Please don't misunderstand me - I think what you're doing here is a great addition.

The generalisations will need to be made before we can ship any live profiles. I'm happy for us to ship an example profile for Ubuntu, but I think even the one we have at the moment is too fragile for this.

I think a different approach to handling the "AppArmor support" may be more to your liking. I went the "check the /proc file" route because I wanted something that worked automagically. There were two other approaches I considered and rejected, but they may be worth re-evaluating:

1. A new configuration option on whether to execute the X server with NNP. It would default to TRUE, and have no effect outside of Linux. Users who wish to enable AppArmor confinement can set this to FALSE.

2. stat() the X server path, and iff the file is setuid, then enable NNP. This would not catch e.g. the X server being a shell script that then executes an actual setuid X binary, but it addresses the quasi-common case of /usr/bin/Xorg being setuid and the only available binary.

No. 1 would achieve the aim of explicitness, and no. 2 is an alternate solution to the original problem. Neither of these rely on an AppArmor-specific interface.

That aside, note that distros that don't enable the AppArmor LSM by default can still have it enabled by users (e.g. Debian used to have an apparmor=1 kernel option for this), so a compile-time option that is not used by the packager would require users to recompile xrdp in order to use these profiles. That would present a significant barrier to better security practice, especially in conjunction with a package manager that delivers occasional security updates.

With regards to profile support, I see a few possibilities for how to handle this:

1. Replace apparmord_DATA with noinst_DATA in instfiles/apparmor.d/Makefile.am, so that the profiles don't reach the make install tree;

2. Add a README file in instfiles/apparmor.d/ indicating the development status of the profiles, a request for improvements, and the "use at your own risk" warning;

3. Add the same verbiage as no. 2 to comment boilerplate at the top of both profiles.

Would some/all of these address your concerns?

Having the profiles fully generalized before contributing them is effectively a chicken-and-egg problem. One person could do that only if they regularly work with two or more distros and are able to develop AppArmor profiles from scratch and are specifically interested in xrdp enough to do so. I wouldn't hold my breath. Two or more people could do that, but they need to find each other first---and if the common point of contact (xrdp) doesn't have anything for them, they are more likely to develop their own distro-specific profiles. The opportunity for collaboration will be lost, and the fragmentation won't deliver a better end result.

That's a well-considered post - thanks.

Configuration options; I'm happy with 1). I'd considered 2) as well myself, but discounted it for precisely the reasons you suggest. I find interposing a shell script for an executable is a really handy debugging technique.

As regards the profiles, what you suggest is fine, but the README needs to go at the top-level I think so that it's more prominent. It can probably also be linked from the project README.md so it's findable from the description on Github. Maybe a README.apparmor.md?

I accept the chicken-and-egg problem is a real one. I'm less sure than you are about the benefits or possibilities of collaboration, but then I'm also less knowledgeable about AppArmor in general!

I'm very happy to work with you to get something in here. I've got a few family commitments at the moment which are going to take up quite a lot of my free time. Please don't take any long periods of silence on my part as a lack of interest.

All right, I think we have a way forward. Please let me know if any of my understanding below is at variance with yours.

• I'll remove the AppArmor /proc interface code, and implement a sesman.ini configuration option that will allow users to explicitly disable the use of NNP when invoking the X server.
• I'll write up a README.apparmor.md file explaining the development situation with the AppArmor profiles. As I plan on using the profiles regularly in my own site deployments, it'll probably include a request to ping me on related PRs.
• I can also update the README.md file to link to the aforementioned. If you have any particular wording in mind, let me know.
• Here is a draft Win32 implementation for g_file_open_ex() I've put together (needed so that g_file_open_ro() doesn't just fail on Windows). This comes purely from eyeballing Microsoft docs, and I have no way of testing it, so please let me know if this is at least passable:

  int
  g_file_open_ex(const char *file_name, int aread, int awrite,
                 int acreate, int atrunc)
  {
  #if defined(_WIN32)
      DWORD aflags = 0;
      DWORD dispos = 0;
  
      if (aread)
      {
          aflags |= GENERIC_READ;
      }
      if (awrite)
      {
          aflags |= GENERIC_WRITE;
      }
      if (acreate)
      {
          dispos = atrunc ? CREATE_ALWAYS : OPEN_ALWAYS;
      }
      else
      {
          dispos = atrunc ? TRUNCATE_EXISTING : OPEN_EXISTING;
      }
  
      return (int)CreateFileA(file_name, aflags,
                              FILE_SHARE_READ | FILE_SHARE_WRITE,
                              0, dispos, FILE_ATTRIBUTE_NORMAL, 0);
  #else
      ...
  

I accept the chicken-and-egg problem is a real one. I'm less sure than you are about the benefits or possibilities of collaboration, but then I'm also less knowledgeable about AppArmor in general!

"Collaboration" may have been overselling it a bit, but the key point is that interested newcomers build upon the work that has already been done, rather than [feel they have no choice but to] start from scratch. The only thing worse than someone out there wasting time on redundant work is doing that oneself.

I'm very happy to work with you to get something in here. I've got a few family commitments at the moment which are going to take up quite a lot of my free time. Please don't take any long periods of silence on my part as a lack of interest.

Understood; I have my own "extracurriculars" as well. I'll need some more time to get all this together, but it'll come---this is ultimately my own itch I'm scratching!

I don't think you need to spend time to support Win32. See also Platform Support Tier for the status of Win32. You can focus on Linux (and BSD) rather than taking effort for Win32.

@metalefty: Understood; thanks for the heads-up. I'll keep my changes POSIX-only.

@iskunk - that all sounds fine to me, and along the lines I'm thinking.

You'll need to add a commented-out setting for your variable to sesman/sesman.ini.in. There's also a man page in docs/man/sesman.ini.5.in where you can add more detail - just use the examples which are already there, and feel free to as any questions you may have.

Thanks.

@matt335672, of course. Here is an early draft for those; let me know if this looks OK:

diff --git a/docs/man/sesman.ini.5.in b/docs/man/sesman.ini.5.in
index 366ecfad..ea6694da 100644
--- a/docs/man/sesman.ini.5.in
+++ b/docs/man/sesman.ini.5.in
@@ -293,6 +293,15 @@ To keep compatibility, the following aliases are also available.
 If set to \fB1\fR, \fBtrue\fR or \fByes\fR, require group membership even
 if the group specified in \fBTerminalServerUsers\fR doesn't exist.
 
+.TP
+\fBXorgNoNewPrivileges\fR=\fI[true|false]\fR
+Only applicable on Linux. If set to \fB0\fR, \fBfalse\fR or \fBno\fR, do
+not use the kernel's \fIno_new_privs\fR restriction when invoking the Xorg
+X11 server. The use of \fIno_new_privs\fR is intended to prevent issues due
+to a setuid Xorg executable. However, if a kernel security module (such as
+AppArmor) is used to confine xrdp, \fIno_new_privs\fR may interfere with
+transitions between confinement domains.
+
 .SH "X11 SERVER"
 Following parameters can be used in the \fB[X11rdp]\fR, \fB[Xvnc]\fR and
 \fB[Xorg]\fR sections.
diff --git a/sesman/sesman.ini.in b/sesman/sesman.ini.in
index 278224a1..bc7aad06 100644
--- a/sesman/sesman.ini.in
+++ b/sesman/sesman.ini.in
@@ -38,6 +38,12 @@ RestrictOutboundClipboard=none
 ;   yes: an alias of all
 RestrictInboundClipboard=none
 
+; On Linux systems, the Xorg X11 server is normally invoked using
+; no_new_privs to avoid problems if the executable is suid. This may
+; interfere with the use of security modules such as AppArmor. Leave this
+; unset unless you need to disable it.
+#XorgNoNewPrivileges=true
+
 [Sessions]
 ;; X11DisplayOffset - x11 display number offset
 ; Type: integer

That looks fine to me.

You can preview what the manpage will look like with man /path/to/sesman.ini.5.in, just to check the formatting looks OK. I haven't done that myself.

Due to the ongoing restructuring of sesman, the AppArmor profiles previously presented here will need significant updates. For now, I'd like to get in just the code changes, which I believe everyone here was amenable to. I've rebased my changes to the current tree, and re-worked the commit chain of this PR accordingly. I'll submit new AppArmor profiles for consideration in a future PR, once xrdp has stabilized. Please let me know if there are any issues.

On a related note, a couple of questions for @matt335672:

1. Given that the way sesman starts the user session is being changed, along with many other things, would it be possible to address the issue previously noted in this PR ("There is one major hole in these AppArmor profiles...") ?

   The gist of it is: Don't set up the environment and then execute startwm.sh directly, because the parent process could be under the control of an attacker who can set their own PATH, LD_PRELOAD, program args, etc.---and any AppArmor permission that allows a process the unconfined invocation of a shell script can be leveraged to run an arbitrary, possibly attacker-supplied binary. Instead, execute some helper program that doesn't depend on any environment variables, and takes no program args. That helper, in turn, then does the environment setup, and executes startwm.sh/DefaultWindowManager.

   The AppArmor profile can specify a permission only allowing unconfined execution of the helper with a scrubbed environment. So even though this helper program remains an "escape hatch" to the restrictions imposed on sesman sesexec (?), there's much less of a way for an attacker to abuse it for their own ends.

2. With all the reworking of how PAM is handled, can we now expect both systemd sessions and pam_mkhomedir/fscrypt/etc. to work? (I specifically had an issue with fscrypt that boiled down to pam_open_session() being called in a different process from previous PAM calls, and noted that this behavior was implemented due to the earlier systemd issue. I'm hoping the tug-of-war got resolved.)

Hi @iskunk

As I've mentioned in another PR, CI is now fixed and requires a rebase to get working.

I'll address your questions first.

1. The way sesexec uses PAM is completely standard. All PAM calls related to session management are now made from one process. The only exception is when a user reconnects to a session. A new PAM stack is used to authenticate and authorize the user, and then that PAM stack is thrown away because there's one already in place for the session.
2. I can't see how an additional process can be used to prevent the attack you describe. sesexec would need to pass the name of sesman.ini to the additional process (this is a runtime setting), and this can be crafted to allow a redirection to any file of the attacker's calling. Am I misunderstanding something?

Rebased as requested. (Thanks for straightening out those CI failures!)

1. Bravo! I hope to try making fscrypt work with xrdp again. (Since pam_mkhomedir was a known issue, I'm assuming xrdp now knows not to touch the user homedir until pam_open_session() returns)
2. Yes, an attacker-chosen sesman.ini would be bad. Would it be reasonable to limit the file to somewhere under XRDP_CFG_PATH, or does it need to be completely arbitrary? (sesman/sesexec itself would be limited in where it could read a config file from by AppArmor anyway)

1. pam_open_session() is called as root before any of the xrdp sub-processes attempt to access $HOME for the user. The code is in session_start_wrapped(). pam_open_session is called from auth_start_session().
2. Yes, that would be reasonable, although in general this is a difficult (or even impossible) thing to achieve because of symlinks. Could you expand a bit more on what you're suggesting? It's a pretty good time to change this interface,before next major release.

Yes, that would be reasonable, although in general this is a difficult (or even impossible) thing to achieve because of symlinks. Could you expand a bit more on what you're suggesting?

The idea would be for the helper program to ensure that the config-file path starts with e.g. /etc/xrdp/ and contains no /../ elements. The AppArmor-confined sesman process would have no write access to /etc/xrdp/ nor its parents---even though the process is running as root. So if any path matching the criteria traverses symlinks, then that is presumably because the sysadmin put the symlinks there, not the attacker.

Very happy with this (as already discussed), but we could do with testing your g_file_open() changes.

Gladly! I've rolled in your test code to my first commit, and confirmed that it passes. (I don't have much to add there, not least as these functions are used everywhere, even in the testing code itself...)

The idea would be for the helper program to ensure that the config-file path starts with e.g. /etc/xrdp/ and contains no /../ elements. The AppArmor-confined sesman process would have no write access to /etc/xrdp/ nor its parents---even though the process is running as root. So if any path matching the criteria traverses symlinks, then that is presumably because the sysadmin put the symlinks there, not the attacker.

So to summarise:-

1. If config file path starts with '/' it has to start with $XRDP_CFG_PATH/. Otherwise it's a relative path to $XRDP_CFG_PATH.
2. config file path has no reason to contain '/../'

We can simplify this to only allowing relative paths, and not allowing '..' to appear in the config file path.

I'm still somewhat concerned about a helper program setting a default environment. In general, by the time we get to this stage, the environment will have been set largely by PAM (or other) calls. I can't see any way to generalise this.

The restrictions you've mentioned above for the config could be placed on the startwm.sh invocation instead. Doesn't that cover most of your concerns?

I'm going to merge this, by-the-way. Thanks again. I think we can continue the above conversation here.

So to summarise:-

1. If config file path starts with '/' it has to start with $XRDP_CFG_PATH/. Otherwise it's a relative path to $XRDP_CFG_PATH.

2. config file path has no reason to contain '/../'

We can simplify this to only allowing relative paths, and not allowing '..' to appear in the config file path.

Pretty much, yes. (Though interpreting a relative config-file path as relative to $XRDP_CFG_PATH instead of the current directory may be unexpected by the user.)

I'm still somewhat concerned about a helper program setting a default environment. In general, by the time we get to this stage, the environment will have been set largely by PAM (or other) calls. I can't see any way to generalise this.

Yes, a necessary aspect to this is that the helper needs to throw away whatever environment it gets (or more specifically, when it is invoked by sesman/sesexec, an AppArmor rule will scrub the environment beforehand), so it will need to set up the user environment itself.

Is there not a good way to recreate the environment given by pam_env.so et al. but in a separate, user-owned process? I was afraid that might be the case...

The restrictions you've mentioned above for the config could be placed on the startwm.sh invocation instead. Doesn't that cover most of your concerns?

That approach can also work, since the AppArmor rule can be written to allow (direct) invocation of the script with a scrubbed environment. But then startwm.sh (or whatever other default_wm is configured) needs to set up the environment itself, similarly from scratch. Would that approach be preferable?

I'm going to merge this, by-the-way. Thanks again. I think we can continue the above conversation here.

Much obliged :-) I should have done this (submitted the code changes and left the AA profiles pending) a long time ago.

I don't think there's a good way to reconstruct the PAM environment in another process. I looked into this in quite some detail when I was constructing the updated architecture. PAM implementations rather assume that everything is happening in the one process.

So if sesexec is compromised, the best we can do is contain the damage as much as possible.

Can restricting the window manager script to be under $XRDP_CFG_PATH be done by AppArmor directly? I don't suppose you know what other login managers do in this regard?

Ensuring that the wm script is in the proper directory is doable, but it doesn't get you much, unfortunately. Even if the script is squeaky-clean, it is still a typical shell script, and is thus prone to muckery like the following:

1. Set PATH=/tmp/evilbin:$PATH;
2. Place a few custom programs in this directory, all identically evil: [, test, grep, etc.;
3. Execute the script, and wait for it to run your evil program.

Unless a shell script goes to the trouble of guarding against this---and it's not even clear to me how feasible that is, given the complexity of the shell environment---it's a sitting duck for this line of attack. (And that's not even getting into what you can do with shell functions, LD_PRELOAD, etc.)

That's why scrubbing a potentially compromised environment is critical. Sesexec may run the wm script as root (because root could legitimately be logging in, questionable as that is), and while sesman/sesexec can be confined under AppArmor (limiting what an attacker can do, even as root), the wm script is not. And if the attacker can make the script walk into their chosen program, as root, then it's game over.

There may not be a good solution to this, that doesn't break PAM functionality in one way or another.

The closest parallel I can think of is OpenSSH, with its privilege separation and sandboxing mechanisms. (If you need a refresher, this article presents a good general overview IMO. Not to say that those mechanisms have always been impregnable, of course.)

The startwm.sh issue aside, I think it would be good to officially adopt the Debian practice of running the xrdp daemon as an unprivileged user. Since that daemon is what services the external network connection, the basis of a similar privilege-separation strategy is already in place. It just needs to be taken further.

At present, the 'WM script' can be any executable - it doesn't have to be a script. The code in sesexec just execs whatever it's given.

The problem is, how do you clean up an environment that's potentially compromised at that point? I think we agree that's not solveable in general, but for specialist environments it very much is. I agree that s shell script is not as well suited to this as a specific executable would be. Those environments can provide a suitable executable which knows what the environment would be, and can enforce it before running the script.

Some distros are moving towards using systemd --user to start the whole desktop. If this catches on, the executable can be systemctl, or something that calls it.

The Debian practice is on my short-term TODO list. We've already spoken about this internally.

A security-critical environment could forgo the standard PAM environment setup, do the AppArmor environment scrubbing when executing default_wm, and then have that be a special program (it could be a script, no big deal when the env is clean) that sets up the environment from scratch. But that's too oddball from an administrative perspective for me to expect more than a handful of users to ever do it.

So, while I'll give up on the idea of secure execution of default_wm for the masses, protecting sesman by battening down the xrdp daemon seems like a good fallback position. Re the Debian practice TODO: 👍