After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 764669 - set PAM_TTY to the login vt when initialising pam
set PAM_TTY to the login vt when initialising pam
Status: RESOLVED FIXED
Product: gdm
Classification: Core
Component: general
3.20.x
Other Linux
: Normal normal
: ---
Assigned To: GDM maintainers
GDM maintainers
Depends on:
Blocks:
 
 
Reported: 2016-04-06 08:17 UTC by darkxst
Modified: 2016-04-13 03:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gdm-session: set PAM_TTY when initialising pam so Debian/Ubuntu can detect if login is coming from a local tty (3.44 KB, patch)
2016-04-11 13:32 UTC, darkxst
none Details | Review
gdm-session: set PAM_TTY when initialising pam so Debian/Ubuntu can detect if login is coming from a local tty (3.13 KB, patch)
2016-04-11 13:47 UTC, darkxst
none Details | Review
gdm-session: set PAM_TTY when initialising pam so Debian/Ubuntu can detect if login is coming from a local tty (3.25 KB, patch)
2016-04-12 07:16 UTC, darkxst
committed Details | Review
gdm-session: require a password for for remote logins (985 bytes, patch)
2016-04-12 07:16 UTC, darkxst
committed Details | Review

Description darkxst 2016-04-06 08:17:03 UTC
The user on the Ubuntu GNOME Live CD's does not have a password, which means is a user hits the login screen (logs out for e.g) they are not able to log back these days, since the "sign In" is not reactive. We do have autologin set on the first boot only, but I suspect people who create a persistant USB live session will also have this problem.

It would be nice if the login screen would respect that AccountsService.UserPasswordMode.NONE and not ask for a password in that case. The lock screen already supports this, but not the login screen.
Comment 1 Ray Strode [halfline] 2016-04-07 20:32:09 UTC
sounds like a pam misconfiguration. pam shouldn't be asking for a password at all if you've used passwd -d on the account.
Comment 2 darkxst 2016-04-11 11:01:29 UTC
so turns out Debian/Ubuntu have patched pam_unix to provide an "nullok_secure" argument which checks that the login is coming from a tty listed in /etc/securetty (based on the PAM_TTY property).

However gdm-session-worker doesnt set PAM_TTY until much later (well after pam_authenticate is called), so that fails. Would it be possible/acceptable to have gdm set this to the login_vt before calling pam_authenticate?
Comment 3 darkxst 2016-04-11 13:32:29 UTC
Created attachment 325725 [details] [review]
gdm-session: set PAM_TTY when initialising pam so Debian/Ubuntu can detect if login is coming from a local tty

For review only, I still need to test this one. Though not even sure I can test with git master 3.20 might be the best I can do
Comment 4 Ray Strode [halfline] 2016-04-11 13:40:33 UTC
Review of attachment 325725 [details] [review]:

looks mostly fine. There are a few issues that will keep it from working, and i have some minor style nits.

::: daemon/gdm-session-worker.c
@@ +969,3 @@
+        char tty_string[256];
+        struct vt_stat vt_state = { 0 };
+

too many new lines here

@@ +988,3 @@
+        return TRUE;
+
+fail:

maybe call this out: , and drop the duplicated close and return call above.  then just return got_login_vt; or something like that.

@@ +1078,3 @@
         g_debug ("GdmSessionWorker: state SETUP_COMPLETE");
         worker->priv->state = GDM_SESSION_WORKER_STATE_SETUP_COMPLETE;
 

I think you need to call get_login_vt here.

@@ +1084,3 @@
+                pam_set_item (worker->priv->pam_handle, PAM_TTY, tty_string);
+        } else {
+                worker->priv->password_is_required = TRUE;

This is probably a good idea, but it's orthogonal, so should be a separate commit.

@@ +1113,3 @@
 
+        set_pam_login_vt (worker);
+

this line should be removed
Comment 5 darkxst 2016-04-11 13:47:22 UTC
Created attachment 325726 [details] [review]
gdm-session: set PAM_TTY when initialising pam so Debian/Ubuntu can detect if login is coming from a local tty
Comment 6 Jasper St. Pierre (not reading bugmail) 2016-04-11 17:20:01 UTC
Review of attachment 325726 [details] [review]:

::: daemon/gdm-session-worker.c
@@ +965,2 @@
 static gboolean
+get_login_vt (GdmSessionWorker *worker)

i don't like naming things "get" that don't return their values. i would prefer "ensure_login_vt"

@@ +967,3 @@
+{
+        int fd;
+        char tty_string[256];

unused?

@@ +970,3 @@
+        struct vt_stat vt_state = { 0 };
+
+

extra newline

@@ +971,3 @@
+
+
+        fd = open ("/dev/tty0", O_RDWR | O_NOCTTY);

shouldn't this be the magic "/dev/tty" that returns the one for the current process?

@@ +1081,3 @@
+        /* set PAM_TTY with login_vt */
+        if (display_is_local) {
+                get_login_vt (worker);

you never check the return value?

@@ +1082,3 @@
+        if (display_is_local) {
+                get_login_vt (worker);
+                g_snprintf (tty_string, 256, "/dev/tty%d", worker->priv->login_vt);

i hope we have a tty_string in the local scope? i don't see anywhere that existing code would have used it though.
Comment 7 Ray Strode [halfline] 2016-04-11 18:00:10 UTC
(In reply to Jasper St. Pierre from comment #6)
> Review of attachment 325726 [details] [review] [review]:
> 
> ::: daemon/gdm-session-worker.c
> @@ +965,2 @@
>  static gboolean
> +get_login_vt (GdmSessionWorker *worker)
> 
> i don't like naming things "get" that don't return their values. i would
> prefer "ensure_login_vt"
I actually told him to use get_login_vt over IRC. I don't like ensure, since it's not reserving the vt, just querying it.  I don't like query because it clashes with the completely different, but also used VT_OPENQRY.

Maybe fetch_login_vt ?

> +        fd = open ("/dev/tty0", O_RDWR | O_NOCTTY);
> 
> shouldn't this be the magic "/dev/tty" that returns the one for the current
> process?

No.  /dev/tty is the controlling tty of the process, it doesn't necessarily have anything to do with console ttys / the vt subsystem.  Most daemons don't have controlling ttys.  Actually the O_NOCTTY in that call above is to prevent the worker from gaining a controlling tty when opening /dev/tty0.

/dev/tty0 is the right device to use to query the current vt.  Of course, a better approach would be to pass in the vt, since we should already know it.  That takes a lot more plumbing though, and this is good enough.
Comment 8 darkxst 2016-04-12 05:56:51 UTC
(In reply to Ray Strode [halfline] from comment #7)
> (In reply to Jasper St. Pierre from comment #6)
> > Review of attachment 325726 [details] [review] [review] [review]:
> > 
> > ::: daemon/gdm-session-worker.c
> > @@ +965,2 @@
> >  static gboolean
> > +get_login_vt (GdmSessionWorker *worker)
> > 
> > i don't like naming things "get" that don't return their values. i would
> > prefer "ensure_login_vt"
> I actually told him to use get_login_vt over IRC. I don't like ensure, since
> it's not reserving the vt, just querying it.  I don't like query because it
> clashes with the completely different, but also used VT_OPENQRY.
> 
> Maybe fetch_login_vt ?
well it is the login_vt, so technically its already reserved since gdm is already running on it. so maybe ensure is ok?
Comment 9 darkxst 2016-04-12 07:16:16 UTC
Created attachment 325766 [details] [review]
gdm-session: set PAM_TTY when initialising pam so Debian/Ubuntu can detect if login is coming from a local tty
Comment 10 darkxst 2016-04-12 07:16:25 UTC
Created attachment 325767 [details] [review]
gdm-session: require a password for for remote logins

To ensure that passwordless logins dont work over XDMCP
Comment 11 Ray Strode [halfline] 2016-04-12 14:15:12 UTC
Review of attachment 325766 [details] [review]:

sure, looks fine to me.

::: daemon/gdm-session-worker.c
@@ +1076,3 @@
         worker->priv->state = GDM_SESSION_WORKER_STATE_SETUP_COMPLETE;
 
+        /* set PAM_TTY with login_vt */

this comment doesn't add anything, so i would drop it.  or if you want a comment it should say:

/* Temporarily set PAM_TTY to the currently active vt (the one used by the login screen).
   Note, PAM_TTY will get set to the user's VT right before the user session is opened. */

or something like that.
Comment 12 Ray Strode [halfline] 2016-04-12 14:23:19 UTC
Review of attachment 325767 [details] [review]:

this is fine, but you should give some justification in the commit message.

How about: 

Many remote services refuse passwordless logins, so administrators may find it surprising that the login screen over xdmcp doesn't require a password on the account.
This commit toggles to that default.

Also adding a statement to hammer home how bad of an idea xdmcp is would be nice:

Note, this change doesn't really make XDMCP more secure. It is inherently insecure over an untrusted network, since it's a completely plain text protocol.
Comment 13 darkxst 2016-04-13 03:50:02 UTC
Attachment 325767 [details] pushed as 25e8677 - gdm-session: require a password for for remote logins