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!
--
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
