sesman: auth session before fork

[jsorg71]

So, the home directory can be created by "pam_mkhomedir".

[proski]

+1

[metalefty]

The change looks good. But I want you to include "why" in commit message for future source code reading. "Why" is always more important than "What". In this change, to create home directory, right? It's OK to do force push in unmerged PRs.

[ksteinb]

Correct session management is necessary for more things than just to create homedirectory. Here some reasons:

-> Create Homedirectory on demand
-> Correctly setup umask (pam_umask)
-> Correct setup of selinux contexrt
-> Handling of limits (pam_limits)

and so on .....

[proski]

I'm a big fan of good comments in the code, however...

The reader of the code would not see that auth_start_session was called elsewhere. So a comment in the code would not be very useful. We cannot annotate every line of code with an explanation why it's there and not elsewhere.

Those who want to dig deeper should be able to find the commit and it's comment. That would provide information about the change and the reason behind it.

Of course, if somebody can come with a useful comment about the auth_start_session that is not duplicating the code, I'll be glad to see it. But the lack of a comment should not be a blocker.

[metalefty]

I think most of correct session management procedures can be done in PAM.
For example, suggested in #688. This fixes things done in PAM wasn't working.

Of course I don't intend to block merging this PR. We can add comments whenever later.

[ksteinb]

#688 describes the correct setup of the pam configuration, but that doesn't help as long as auth_start_session is called in the wrong place.

So just write as comment something like:

"Calling auth_start_session in the correct place, otherwise pam session management fails"

[jsorg71]

Does it also fix the issue if auth_start_session is moved above wait_for_xserver?
I'd rather do that.
There is not matching auth_start_session / auth_stop_session now.

[jsorg71]

After looking closer, I think it's right now. auth_start_session and auth_stop_session are called in same process now.

Before this PR

session_start_fork()
g_fork()
    child - g_fork()
        child auth_start_session(), g_fork()
            child - g_exec(window manager)
        parent - g_waitpid(), auth_stop_session(), g_exit()
    parent - g_fork()
        child - g_exec(X11 backend)
    parent - sesman svc(wait for window manager to exit, then kill X11 backend)
parent - exit session_start_fork()

After this PR

session_start_fork()
g_fork()
    child - auth_start_session(), g_fork()
        child g_fork()
            child - g_exec(window manager)
        parent - g_waitpid(), auth_stop_session(), g_exit()
    parent - g_fork()
        child - g_exec(X11 backend)
    parent - sesman svc(wait for window manager to exit, then kill X11 backend)
parent - exit session_start_fork()

[rolnas]

This patch don't work with pam_krb5 on Debian 8.
I moved auth_start_session higher to the same process as auth_userpass.
That works, but some auth_end closes session too early (but retain_after_close=true on pam_krb5 solves temporary).
I'm attaching patch and log files from successful session (pam_krb5 with debug=true).
auth_start_session2.txt
auth_start_session2_log.txt

[moobyfr]

@rolnas can you add your pam.d/common-* files to understand the problem ?
I'm using pam_krb5 or pam_ldap with xrdp since several years without problem.

[ksteinb]

Am 16.03.2017 um 18:29 schrieb Blindauer Emmanuel:

@rolnas <https://github.com/rolnas> can you add your pam.d/common-* files to understand the problem ? I'm using pam_krb5 or pam_ldap with xrdp since several years without problem.

the problem is _not_ authorization as you need for pam_ldap or pam_krb5. It is session management. For example pam_limits was not working (pam_limits has _only_ the session module). Sincerly, Klaus

…

-- Rechnerbetriebsgruppe / IT, Fakultät für Physik Klaus Steinberger FAX: +49 89 28914280 Tel: +49 89 28914287

[jsorg71]

I create #696 that can replace this PR. Hopefully it works for everyone.

[setharnold]

Use CVE-2017-6967.
Thanks

[mrkz]

Hello, I would like to confirm if PR #704 and #697 are the fix to this CVE issue?
Thanks!

[jsorg71]

It's just #704 for the fix. Can someone else confirm it;s fixed?

[moobyfr]

I confirm it. I thing the pam process is now ok: before, if pam_krb5 was used, the environement variable set by the module was't available in the user session an the file created by the module was owned by root. Now the variable is set and file owned by user.