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.
** Changed in: libpam-freerdp (Ubuntu)
Assignee: Tyler Hicks (tyhicks) => (unassigned)
--
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
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
