Bug 28377

Created attachment 36055 [details] [review]
patch

Initial patch

Review of attachment 36055 [details] [review]:

Overall makes sense.  We should probably figure out some sort of story for OSes that don't have login-session-id.  Might make sense to talk to Halton or Brian.

::: src/ck-manager.c
@@ +1657,3 @@
+enum {
+        P_STRING,
+        P_BOOLEAN,

Should be PROP_ not P_ to match the surrounding conventions

@@ +1701,3 @@
+                        g_debug ("Skipping NULL parameter");
+                        goto cont;
+                }

Can it be NULL?  I don't think dbus allows NULL

@@ +1712,3 @@
+                                const char *val;
+                                val = g_value_get_string (prop_val);
+                                if (value) {

you use if (foo != NULL) in other parts of the code, not if (foo)

@@ +1713,3 @@
+                                val = g_value_get_string (prop_val);
+                                if (value) {
+                                        *value = g_strdup (val);

A more concise way to write this whole block would be:

if (value != NULL) {
      *value = g_value_dup_string (prop_val);
}

@@ +1722,3 @@
+                                val = g_value_get_boolean (prop_val);
+                                if (value) {
+                                        *(gboolean *)value = val;

Won't this cause strict aliasing warnings? Maybe the last argument of this function should be variadic instead of gpointer *.

@@ +1742,3 @@
+                        g_value_unset (prop_val);
+                        g_free (prop_val);
+                }

You could probably save the duplicated code here by just adding && ret == FALSE to your for loop conditional.

@@ +1760,3 @@
+        /* If any local session exists for the given login session id
+           then that means a trusted party has vouched for the
+           original login */

Should it be *any* or "The one that was created with OpenSessionWithParameters" ?

@@ +1776,3 @@
+                                      "is-local", &is_local,
+                                      NULL);
+                        if (sessid != NULL && strcmp (sessid, login_session_id) == 0 && is_local) {

maybe just use g_strcmp0 here?

On Solaris, I believe we never set login-session-id since there is not a straightforward similar interface as /proc/%u/sessionid when not using Linux, and ConsoleKit/GDM seemed to work well enough without needing to set it before.

However, if now ConsoleKit/GDM depend on it being set to determine if sessions are local or not, could you explain how it is supposed to be set?  Looking at Linux manpages, sessionid is the process id of the session leader.  Is that what this value should be set to? 

Thanks,

Brian

ConsoleKit used to talk to the X server and if the X server was running on the local machine then it would treat sessions run on that X server as local.

The problem with that heuristic is, remote users (logged in via ssh) can start local X servers.  We don't want remote users to be able to initiate local sessions.

This means we need a way to know "is this session local or not" that doesn't use the X server.

ConsoleKit has two ways of starting sessions

OpenSessionWithParameters
and
OpenSession

The latter can be run by anyone and the former can be run only by root users.  We get is-local passed to us with OpenSessionWithParameters and can trust it, but for OpenSession its up to us to figure it out.

One way to do that is by asking "Is the session it was started from local" ?  If the answer to that question is yes, then is-local is yes for the newly opened session as well.

How do we figure out which session this session was started from, though?  We can't look at parent pid since process hierarchies are mutable.  We can't look at enviornment variables, since they're spoofable.  We need some mechanism to tie two sessions together that is infallable.  On linux we can use /proc/%u/sessionid ..  If one ck session is started from another they'll have the same sessionid.  We need something like that for solaris as well, if it's available, or come up with some alternate game plan.

I notice on Solaris that the proc structure has the following:

            pid_tpr_pgid;            /* process group id */
            pid_tpr_sid;             /* session id */

I cannot find much documentation via Google about how /proc/pid/sessionid works.  Is it the same as one of the above?  If so, then modifying  the
ck_unix_pid_get_login_session_id function in ck-sysdeps-solaris.c to simply call stat2proc() and returning the right value might work.

Though I'm not sure how to test this.  I can't even see the upstream bugzilla bug since I don't have a Red Hat bugzilla account and it won't let me see the bug without logging in first.  If a test case could be provided which shows how to test this, then that would help.

Hi Brian,

That's a different kind of session id.  that's a session id in the "setsid()"-stevens since of the word.

What we need is some way to positively identify that a session was initiated from a local console.

As far as a test case goes, just start an X server from an ssh login, export DISPLAY=:display-number and run ck-launch-session

ck-list-sessions will show the session as local instead of remote.

one thing to note... sessionid is -1 on linux systems configured without audit

Created attachment 36637 [details] [review]
Only set sessions to be is-local=true if set by a trusted party

clearing ck-security group since the commit is public.

(In reply to comment #0)
> This results in some issues like those discussed in:
> https://bugzilla.redhat.com/show_bug.cgi?id=585952

Sidenote: This bug is still referenced as private.

I'm still unsure what we should do to get is-local and active set to TRUE after this commit, so meanwhile I've had to revert the entire commit for consolekit package in Gentoo:

http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/sys-auth/consolekit/files/consolekit-0.4.2-revert.patch?revision=1.2&view=markup

Can we please get this someway documented? :-/

For anyone that is struggling with what is required for a session to be considered local when using the OpenSession method (i,e. with login and ck-launch-session) after this change I have commented on it here: http://bugs.gentoo.org/show_bug.cgi?id=336634

Fwiw I experienced the same bug on Debian (see http://bugs.debian.org/598150)

Summary:

* X session started via gdm: OK
* X session started via slim/startx and running ck-launch-session: NOK
* X session started from console with working ck session (via pam connector): OK

I've tried reverting the patch and it worked fine, so I guess the logins (either through slim or through startx) aren't set as trusted for unknown reason. I was asked to add:

session optional        pam_loginuid.so

to my pam setup, but it didn't work.

I've assigned this CVE-2010-4664 and opened up the Red Hat bugs.