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 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.
> * pam-freerdp.c mlocks password buffers, but freerdp-auth-check.c
> doesn't.
Will fix.
> * 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.
Yes, sorry. That was a work around to not having a home directory yet
that wasn't removed.
--Ted
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1039634
Title:
[MIR] libpam-freerdp
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/libpam-freerdp/+bug/1039634/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
