[MIR] libpam-freerdp

[Michael Terry]

libpam-freerdp is a new package from Canonical to log into PAM using a remote server, 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)]

Blockers:
* Has several TODO items which are important (like using stdin instead of command line)
* open_session doesn't do anything.

Nits:
* Should have a bug subscriber

Notes:
* Small, simple package
* Builds fine
* New package
* All dependencies in main
* Canonical will maintain

This will also need a security review, once it is actually featureful.

[Michael Terry (mterry)]

Also, what's the story with unit tests?

[Ted Gould (ted)]

On Tue, 2012-08-21 at 21:56 +0000, Michael Terry wrote:
> Also, what's the story with unit tests?

Really, it's looking like they'll have to be manual because PAM is so
hard coded to be "unbreakable". Basically you'll have to be root to be
able to install a PAM session that we can then use for testing. I'm
planning up writing the testing I'm doing, but I don't think we'll have
"unit tests" in the common usage of the term.

[Michael Terry (mterry)]

OK, seems fine besides a security pass.

[Michael Terry (mterry)]

(after having uploaded 0.2.0 that is)

[Michael Terry (mterry)]

Just uploaded 0.3.0 which should fix a few issues.

[Tyler Hicks (tyhicks)]

I've completed my initial security review of the project. Of course, there is
no CVE history due to the project being new. The project consists of a fairly
simple PAM module and a helper application that uses the libfreerdp API to
authenticate to a remote RDP server.

I've given libpam-freerdp code a shallow security review. This should not be
considered a thorough code audit because I only looked for some of the more
common security issues. Some issues that I found in pam-freerdp.c are:

 * A named socket is created as root, inside of user home directories. There
   are quite a few things that can go wrong when a privileged process is doing
   things inside of a user-controlled directory.

   For example, there is currently a race condition between the call to bind()
   and the call to chmod(). After the bind(), the user could unlink
   $HOME/.freerdp-socket, a symlink could be created in its place that points
   to /etc/passwd, and then the chmod() would make /etc/passwd owned by the
   user.

   Please move all of the socket-based code after the fork() and set*id() calls.

 * The return values of malloc(), strdup(), mlock() and snprintf() are not
   checked.

 * Memory containing a copy of PAM_AUTHTOK should be memset() with 0's prior to
   munlock()/free().

 * It is a good idea to clear the environment, using clearenv(), when dropping
   privileges after fork().

 * It is a good idea to also drop all extra supplementary groups, using
   setgroups(1, &pwdent->pw_gid), when dropping privileges after fork().

 * The handling of session_pid doesn't look right to me. Do we really want to
   blindly kill a PID that we stored in a global variable at some point in the
   past? I think there are probably PID wrap around issues here.

 * The use of static global variables is not recommended by the PAM
   documentation. According to PAM_SET_DATA(3), "PAM modules may be dynamically
   loadable objects. In general such files should not contain static
   variables."

   Could everything in pam_sm_open_session() be moved to pam_sm_authenticate()
   to get rid of the need for the globals? If not, pam_set_data() should
   probably be used.

The issues that I found in freerdp-auth-check.c are:

 * pam-freerdp.c mlocks password buffers, but freerdp-auth-check.c doesn't.

 * The freerdp ignore_certificate settings is set to true, which presumably
   disables certificate verification. I can't find any useful libfreerdp
   documentation, so I say 'presumably'. Since we're sending login credentials
   here, we really need to make proper use of encrypted protocols when
   possible.

   This setting should be set to false and it should be verified that TLS
   connections are attempted to be negotiated by default.

Security NAK while these items are being addressed. Please let me know if you
have any questions or if I misunderstood anything.

[Ted Gould (ted)]

On Thu, 2012-08-30 at 07:19 +0000, Tyler Hicks wrote:
> * A named socket is created as root, inside of user home directories. There
> are quite a few things that can go wrong when a privileged process is doing
> things inside of a user-controlled directory.
>
> For example, there is currently a race condition between the call to bind()
> and the call to chmod(). After the bind(), the user could unlink
> $HOME/.freerdp-socket, a symlink could be created in its place that points
> to /etc/passwd, and then the chmod() would make /etc/passwd owned by the
> user.
>
> Please move all of the socket-based code after the fork() and set*id() calls.

The problem with moving that code after the fork() is that we create a
race condition with the starting of the session. The session expects
that file will exist, and LightDM is basically calling open_session()
and then executing the script. So there's a chance that the fork'd
process won't have started in time to have created the file.

The only way that I could see fixing this is to set up a pipe between
the forked process and the module which then blocks until the pipe has
data. My plan is to take that approach.

> * The return values of malloc(), strdup(), mlock() and snprintf() are not
> checked.

Will fix.

> * Memory containing a copy of PAM_AUTHTOK should be memset() with 0's prior to
> munlock()/free().

Just to be clear, the only case I can find of this is the prompt value,
is that the case you're referring to?

> * It is a good idea to clear the environment, using clearenv(), when dropping
> privileges after fork().

Okay. Will do.

> * It is a good idea to also drop all extra supplementary groups, using
> setgroups(1, &pwdent->pw_gid), when dropping privileges after fork().

K.

> * The handling of session_pid doesn't look right to me. Do we really want to
> blindly kill a PID that we stored in a global variable at some point in the
> past? I think there are probably PID wrap around issues here.

I see where there could be issues there, but I'm unsure of how else to
handle the case of something like this:

open_session();
do_something(&error);
if (error) {
   close_session();
}

Where if we don't go into the session, and we don't read the socket, the
fork'ed process would stay around forever.

> * The use of static global variables is not recommended by the PAM
> documentation. According to PAM_SET_DATA(3), "PAM modules may be dynamically
> loadable objects. In general such files should not contain static
> variables."
>
> Could everything in pam_sm_open_session() be moved to pam_sm_authenticate()
> to get rid of the need for the globals? If not, pam_set_data() should
> probably be used.

Yes, unfortunately the things like AUTHTOK get cleared by PAM and can
only used between modules, not between calls. And we can't use
set_data() for things like the domain where it's not a value that PAM
supports internally. LightDM doesn't support prompting for these values
in open_session() so we have to save the values between authenticate()
and open_session(). But, if other callers use the PAM module we will
prompt as the...

[Tyler Hicks (tyhicks)]

On 2012-08-30 14:09:21, Ted Gould wrote:
> On Thu, 2012-08-30 at 07:19 +0000, Tyler Hicks wrote:
> > * Memory containing a copy of PAM_AUTHTOK should be memset() with 0's prior to
> > munlock()/free().
>
> Just to be clear, the only case I can find of this is the prompt value,
> is that the case you're referring to?

It looks like I missed some of the existing memset() calls when doing my
review yesterday. It is being done correctly in most places. The
promptval is the only place that I see missing a memset.

> > * The handling of session_pid doesn't look right to me. Do we really want to
> > blindly kill a PID that we stored in a global variable at some point in the
> > past? I think there are probably PID wrap around issues here.
>
> I see where there could be issues there, but I'm unsure of how else to
> handle the case of something like this:
>
> open_session();
> do_something(&error);
> if (error) {
> close_session();
> }
>
> Where if we don't go into the session, and we don't read the socket, the
> fork'ed process would stay around forever.

I'll have to give this some more thought. Can we at least issue the kill
after dropping privileges? Doing it when privileged makes it scary.

>
> > * The use of static global variables is not recommended by the PAM
> > documentation. According to PAM_SET_DATA(3), "PAM modules may be dynamically
> > loadable objects. In general such files should not contain static
> > variables."
> >
> > Could everything in pam_sm_open_session() be moved to pam_sm_authenticate()
> > to get rid of the need for the globals? If not, pam_set_data() should
> > probably be used.
>
> Yes, unfortunately the things like AUTHTOK get cleared by PAM and can
> only used between modules, not between calls. And we can't use
> set_data() for things like the domain where it's not a value that PAM
> supports internally. LightDM doesn't support prompting for these values
> in open_session() so we have to save the values between authenticate()
> and open_session(). But, if other callers use the PAM module we will
> prompt as the same calls are used. We're also checking the values of
> the globals to ensure they've been set and they're initialized. So
> while it isn't beautiful, I don't think there's a security issue here.

You're right, there is no security issue. It was just something that I
came across while reading PAM docs. This specific issue isn't a
blocker, IMO.

Thanks!

[Ted Gould (ted)]

On Thu, 2012-08-30 at 17:46 +0000, Tyler Hicks wrote:
> > > * The handling of session_pid doesn't look right to me. Do we really want to
> > > blindly kill a PID that we stored in a global variable at some point in the
> > > past? I think there are probably PID wrap around issues here.
> >
> > I see where there could be issues there, but I'm unsure of how else to
> > handle the case of something like this:
> >
> > open_session();
> > do_something(&error);
> > if (error) {
> > close_session();
> > }
> >
> > Where if we don't go into the session, and we don't read the socket, the
> > fork'ed process would stay around forever.
>
> I'll have to give this some more thought. Can we at least issue the kill
> after dropping privileges? Doing it when privileged makes it scary.

Done.

The only one I've had issue with is it seems I don't have permissions to
do a setgroups(). Either my user or the guest user. So I've removed
that from the merge proposal. Not sure what I could be doing wrong
there other than it's just not allowed by default on Ubuntu.

[Jamie Strandboge (jdstrand)]

Did this not fail because you added setgroups() after you already called setuid()?

[Ted Gould (ted)]

On Thu, 2012-08-30 at 20:54 +0000, Jamie Strandboge wrote:
> Did this not fail because you added setgroups() after you already called
> setuid()?

I tried it both ways, both before and after, and setgroups() failed both
times. I tried putting it before the setuid() as well figuring that
perhaps dropping privs would be the problem, but that didn't seem to be
it either.

[Jamie Strandboge (jdstrand)]

It has always been my understanding that the order to permanently drop privileges from root is:
* setgroups()
* setgid()
* setuid()

Note setgid() sets all of: saved gid, egid and gid and setuid() sets all of saved uid, euid and uid.

I did this locally and it worked fine. setgroups() seems to be able to be called after setgid() but not after setuid().

[Jamie Strandboge (jdstrand)]

(and by locally I mean in a test program, not your pam module).

[Ted Gould (ted)]

On Thu, 2012-08-30 at 22:10 +0000, Jamie Strandboge wrote:
> I did this locally and it worked fine. setgroups() seems to be able to
> be called after setgid() but not after setuid().

Okay, so I figured out my issue, but I'm unsure of how to handle it. I
am testing this using pamtester, which runs as my user. And my user
can't call setgroups() because, well, I'm not root. If I run pamtester
under sudo, then it works.

So, it would seem that a valid error from setgroups is a permission
error. Because if you can't set them, you likely aren't in a position
to really screw with things. Would that be okay?

[Ted Gould (ted)]

Let's not be theoretical, this is exactly what I'm thinking:

http://bazaar.launchpad.net/~ted/libpam-freerdp/security-concerns/view/head:/src/pam-freerdp.c#L240

[Jamie Strandboge (jdstrand)]

Why was this marked to 'Fix Committed'? There is still conversation surrounding it.

[Michael Terry (mterry)]

Jenkins did that. I'm betting ted tied his branch to this bug, so Jenkins automatically marked this Fix Committed when the branch was pushed. Ted, perhaps don't tie MIR bugs with their special status meanings to branches.

[Ted Gould (ted)]

On Fri, 2012-08-31 at 15:13 +0000, Jamie Strandboge wrote:
> Why was this marked to 'Fix Committed'? There is still conversation
> surrounding it.

The current state of the merge proposal was set to approved, so the
autolander landed it. It set it to committed when it landed it. If
there are more issues I'll make another branch.

[Ted Gould (ted)]

On Fri, 2012-08-31 at 15:32 +0000, Michael Terry wrote:
> Ted, perhaps don't tie MIR bugs with their special status
> meanings to branches.

We're discussing it on IRC now. I think the autolander shouldn't change
the status on (Ubuntu) bugs. Those should be managed by the distro team
(using methods of their choosing) and not PS tools.

[Tyler Hicks (tyhicks)]

Hi Ted - Thanks for working on addressing the security issues!

After reviewing revision 30 in the upstream libpam-freerdp project, a privileged kill() of session_pid still exists at the beginning of pam_sm_open_session(). All other issues seem to be addressed. Thanks!

Also, have you confirmed that freerdp-auth-check.c's use of the libfreerdp API attempts to negotiate encrypted connections (TLS, preferably) to the RDP server by default? I'm still not finding any documentation of the API.

[Ted Gould (ted)]

On Mon, 2012-09-03 at 21:34 +0000, Tyler Hicks wrote:
> After reviewing revision 30 in the upstream libpam-freerdp project, a
> privileged kill() of session_pid still exists at the beginning of
> pam_sm_open_session(). All other issues seem to be addressed. Thanks!

Merge proposal posted:

https://code.launchpad.net/~ted/libpam-freerdp/unpriviledged-kill/+merge/122680

> Also, have you confirmed that freerdp-auth-check.c's use of the
> libfreerdp API attempts to negotiate encrypted connections (TLS,
> preferably) to the RDP server by default? I'm still not finding any
> documentation of the API.

So, it seems that what it does is use what the server asks it to. If
the server has TLS enabled it definitely uses it, and gives cert errors
(we had a misconfigured cert on a test server), but when we disabled TLS
server side it is happy. So does that satisfy "by default" ?

[Tyler Hicks (tyhicks)]

On 2012-09-04 13:43:55, Ted Gould wrote:
> On Mon, 2012-09-03 at 21:34 +0000, Tyler Hicks wrote:
> > After reviewing revision 30 in the upstream libpam-freerdp project, a
> > privileged kill() of session_pid still exists at the beginning of
> > pam_sm_open_session(). All other issues seem to be addressed. Thanks!
>
> Merge proposal posted:
>
> https://code.launchpad.net/~ted/libpam-freerdp/unpriviledged-
> kill/+merge/122680

Looks good.

> > Also, have you confirmed that freerdp-auth-check.c's use of the
> > libfreerdp API attempts to negotiate encrypted connections (TLS,
> > preferably) to the RDP server by default? I'm still not finding any
> > documentation of the API.
>
> So, it seems that what it does is use what the server asks it to. If
> the server has TLS enabled it definitely uses it, and gives cert errors
> (we had a misconfigured cert on a test server), but when we disabled TLS
> server side it is happy. So does that satisfy "by default" ?

Yes. When I said "by default", I meant "does freerdp-auth-check try to
use the most secure authentication option supported by the server?".

I'm happy from a security standpoint. Thanks for fixing everything so
quickly!

ACK

[Michael Terry (mterry)]

I just uploaded new upstream release 0.4.0, which fixes Jamie's and Tyler's concern. So I'll mark this Fix Committed. Thanks all!

[Matthias Klose (doko)]

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