[MIR] lightdm-remote-session-freerdp

[Michael Terry]

lightdm-remote-session-freerdp is a new package from Canonical to provide some configuration files needed and scripts required to login to a full screen RDP session using LightDM and FreeRDP, used for Landscape thin client support.

I'll keep this description abbreviated, since I'll just do the MIR myself in verbose fashion.

[Michael Terry (mterry)]

Nits:
* Should have a bug subscriber

Notes:
* Small, stupidly simple package
* Builds fine
* New package
* All dependencies in main, except for libpam-freerdp which has a separate MIR
* Canonical will maintain

Approved from my end, but I am going to have a security person review, because this includes a PAM config file.

[Jamie Strandboge (jdstrand)]

Security review:
 * Hardening options enabled, but not with PIE. Builds without errors or warnings. Lintian clean, no setuid, fscaps or privileged operations via sudo/su/pkexec. No dbus, initscripts/upstart jobs or cron jobs. Simple C program with a pam configuration file
 * code audit of socket-sucker.c:
  * handles environ fine
  * does not check the return value of snprintf and may not be nul terminated. Specifically, this:
snprintf(serv_addr.sun_path, sizeof(serv_addr.sun_path), "%s/%s", home, ".freerdp-socket");
Should be changed to:
if (snprintf(serv_addr.sun_path, sizeof(serv_addr.sun_path) - 1, "%s/%s", home, ".freerdp-socket") > sizeof(serv_addr.sun_path) - 1) {
    return -1;
}
  * not checking return code early enough for read() which could mean the write call writes something unintended
  * not checking return code of write() at all.

Tyler will comment on the pam configuration since he is looking at libpam-freerdp. Otherwise, conditional ACK provided the above code changes are made and it is compiled with PIE.

[Tyler Hicks (tyhicks)]

TBH, we may need to Steve Langasek to take a look at the PAM module config. I still have trouble making sense out of them at times.

To keep the conversation going, I do have one question. It looks like you probably started with /etc/pam.d/lightdm-greeter (or something similar) and modified it a bit. Any reason why you decided to remove the includes for common-auth and common-session?

[Michael Terry (mterry)]

BTW, Jamie's concerns were met by a new upload.

Steve, could you take a look at the PAM config file in this package for us?

[Ted Gould (ted)]

On Thu, 2012-08-30 at 07:57 +0000, Tyler Hicks wrote:
> TBH, we may need to Steve Langasek to take a look at the PAM module
> config. I still have trouble making sense out of them at times.

Okay.

> To keep the conversation going, I do have one question. It looks like
> you probably started with /etc/pam.d/lightdm-greeter (or something
> similar) and modified it a bit. Any reason why you decided to remove the
> includes for common-auth and common-session?

My reason there was quite simply KISS. It seemed like the majority of
that was to allow for customizable things like multi-factor auth to be
loaded and the such, all of which we don't need for this feature. I'm
happy to be wrong there, but that's how I understood it.

[Steve Langasek (vorlon)]

I notice the following things about the provided configuration:

- No account or passwd stanzas are specified. This means it will use the defaults from /etc/pam.d/other; that's generally a good thing.
- common-auth and common-session are not included (as commented above). This makes sense given the intent to provide freerdp authentication, but it's a very non-PAM-ish way to do it. Why, for instance, is this not done as a pam-auth-update profile for the freerdp module, which enables freerdp as one of the available methods for authenticating to a single lightdm service? Looking at the source, apparently the answer is that we want to use freerdp authentication only for a freerdp session, so it is indeed a different service from lightdm' standard usage; so that's a perfectly fine answer, and shouldn't be a blocker for main IMHO.
- gnome-keyring and selinux modules, used in the standard lightdm config, aren't mentioned here. I guess gnome-keyring doesn't matter at all if the session just launches an rdp client; but shouldn't the selinux policy still be applied (if selinux is in use)?

[Ted Gould (ted)]

On Sun, 2012-09-02 at 10:21 +0000, Steve Langasek wrote:
> - common-auth and common-session are not included (as commented
> above). This makes sense given the intent to provide freerdp
> authentication, but it's a very non-PAM-ish way to do it. Why, for
> instance, is this not done as a pam-auth-update profile for the
> freerdp module, which enables freerdp as one of the available methods
> for authenticating to a single lightdm service? Looking at the
> source, apparently the answer is that we want to use freerdp
> authentication only for a freerdp session, so it is indeed a different
> service from lightdm' standard usage; so that's a perfectly fine
> answer, and shouldn't be a blocker for main IMHO.

Yes, we don't want to use it for local account authentication. This is
why I thought using the common* path should not be included. I'd be
happy to add it if you think it should be there blocker or not.

> - gnome-keyring and selinux modules, used in the standard lightdm
> config, aren't mentioned here. I guess gnome-keyring doesn't matter
> at all if the session just launches an rdp client; but shouldn't the
> selinux policy still be applied (if selinux is in use)?

Sure, makes sense. We are running the guest session in a locked down
wrapper that provides some amount of policy. AFAIK this is apparmor
only. Assuming that the lockdown of the guest session was done for
SELinux do we have to worry that they'd conflict?

[Michael Terry (mterry)]

<slangasek> mterry, supporting selinux for this service is definitely a "should" rather than a "must"
<mterry> slangasek, OK, so you're fine with it is as in terms of MIR?
<slangasek> mterry: yes. would you mind filing a bug about the missing selinux handling?

I've filed bug 1046371 for selinux support. So now I believe Jamie, Steve, and I are happy. Marking MIR fix-committed.

[Matthias Klose (doko)]

Override component to main
lightdm-remote-session-freerdp 0.3-0ubuntu1 in quantal: universe/misc -> main
lightdm-remote-session-freerdp 0.3-0ubuntu1 in quantal amd64: universe/misc/extra -> main
lightdm-remote-session-freerdp 0.3-0ubuntu1 in quantal armel: universe/misc/extra -> main
lightdm-remote-session-freerdp 0.3-0ubuntu1 in quantal armhf: universe/misc/extra -> main
lightdm-remote-session-freerdp 0.3-0ubuntu1 in quantal i386: universe/misc/extra -> main
lightdm-remote-session-freerdp 0.3-0ubuntu1 in quantal powerpc: universe/misc/extra -> main
6 publications overridden.