Patches for the PIN authentication work, as detailed in https://live.gnome.org/GnomeOS/Design/Whiteboards/ScreenLock/PinAuthentication
Created attachment 63941 [details] [review] Use PAM to change passwords Modifying /etc/shadow directly can be wrong in many configurations, and will fail to update other tokens (such as keyring passwords). Instead, use a small helper to interact with PAM (as PAM functions are not safe to use in process)
Created attachment 63942 [details] [review] Add PAM module for handling PIN authentication PIN authentication is implementing by decrypting the real password from a file in the user DB directory, using the PIN to derive a key using PBKDF2.
Created attachment 63943 [details] [review] Add a mechanism to set the PIN Add DBus and libaccountsservice API to set the PIN for users, as well as pam-password-helper support to talk to pam_pin.so
I did not test the DBus API, but I did test the PAM module, by manually modifying /etc/pam.d/password-auth. To set a PIN, pam_pin.so must be set to be after pam_unix.so, and the latter must be changed from sufficient to required. Using passwd is not possible, because pam_pin.so must run as root. To use a PIN to login, pam_pin.so must be optional before pam_unix.so. It must not be sufficient, because it does no actual check on its own, it merely decrypts the password.
Created attachment 63944 [details] [review] Add PAM module for handling PIN authentication PIN authentication is implementing by decrypting the real password from a file in the user DB directory, using the PIN to derive a key using PBKDF2.
hey great! I'm looking over your various GDM changes now, I'll look after this afterward. Thanks for all the work you've put into these features.
Comment on attachment 63941 [details] [review] Use PAM to change passwords Review of attachment 63941 [details] [review]: ----------------------------------------------------------------- In general this makes sense. ::: src/user.c @@ +1483,1 @@ > argv[2] = strings[0]; Passing the password on the command line is a security hole. See bug #55000. Additionally this string is crypt()'ed by the caller of SetPassword() so I don't think it'll work to pass it into PAM. Obviously passing a crypted password around won't work for non-local users and stuff. So that is something that needs to be changed as well. See bug #55002.
Ok with the command line thing, it was made to avoid setting up pipes, but it should be ok to accept it on stdin. As for crypt(), I removed that part from the library, so SetPassword passes it unencrypted. I need it because PAM wants the real password, but if it's a security problem we can look at GcrSecretExchange.
Created attachment 72158 [details] [review] pam-helper: use pipes instead of command line arguments to set the password Command line arguments are by default accessible to all users through /proc, which leaves a window for an attacker to retrieve a password while it is changed. Using a pipe avoids this problem. https://bugs.freedesktop.org/show_bug.cgi?id=51833
Created attachment 72159 [details] [review] Use GcrSecretExchange to pass the password from the client to the daemon GcrSecretExchange, from gcr, provides a way for two programs to communicate secret data without the risk of eavesdropping. We use it to pass the password, which would otherwise travel in plain-text, from the UI to the daemon. The two step protocol is implemented by two methods, BeginSetPassword and ContinueSetPassword. Due to the nature of the PIN implementation, and given that password and hint were tied before, ContinueSetPassword is actually a setter for all those three at once. For backwards compatibility, SetPassword is retained and continues to take the plain-text password, but it is deprecated. SetPIN is removed because it never appeared in a stable release.
Comment on attachment 63941 [details] [review] Use PAM to change passwords Review of attachment 63941 [details] [review]: ----------------------------------------------------------------- ::: src/pam-password-helper.c @@ +26,5 @@ > +#include <sys/types.h> > +#include <security/pam_appl.h> > +#include <security/pam_misc.h> > + > +char *password; static char *password; @@ +43,5 @@ > + into each question, and hope that configuration is > + correct. */ > + for (i = 0; i < num_msg; i++) { > + resps[i].resp = strdup(password); > + resps[i].resp_retcode = 0; Should fail if the message type isn't a password (echo off) request. @@ +66,5 @@ > + return 1; > + } > + > + username = argv[1]; > + password = argv[2]; should squash the later patch that uses pipes with this one. @@ +68,5 @@ > + > + username = argv[1]; > + password = argv[2]; > + > + res = pam_start ("accountsservice", username, you need to include the pam service file in this patch.
Comment on attachment 63943 [details] [review] Add a mechanism to set the PIN Review of attachment 63943 [details] [review]: ----------------------------------------------------------------- ::: src/pam-password-helper.c @@ +45,4 @@ > into each question, and hope that configuration is > correct. */ > for (i = 0; i < num_msg; i++) { > + if (strcmp((*msg)[i].msg, "PIN") == 0) I really don't like how we're using translatable messages as machine identifiers. Honestly, I'd rather if the PIN verification worked via some non-pam side channel, and the PAM module would just block until it got a go ahead before setting the AUTHTOK to the user's password and dropping to the next module in the stack. That's more work, though, and I know last time I brought it up you didn't like it much. Another, less invasive idea is to have the pam module set PAM_AUTHTOK_TYPE to "PIN" and have this code check it with pam_get_item
Comment on attachment 63944 [details] [review] Add PAM module for handling PIN authentication Review of attachment 63944 [details] [review]: ----------------------------------------------------------------- I've started to look over this, but want to look deeper. I will look more in a bit. ::: data/accountsservice @@ +14,5 @@ > +session required pam_namespace.so > +session optional pam_keyinit.so force revoke > +session include password-auth > +session include postlogin > + Why are there session modules in this file? Are we allowing PIN for initial login too? Different distros have different service file formats so we'll probably need to do what we do with GDM and have it configure time pluggable. ::: src/pam-pin/pam-module.c @@ +325,5 @@ > + char *ciphertext; > + size_t ciphertext_len; > + int result; > + > + result = pam_get_user (handle, &username, "Username: "); This could probably be NULL since I think we pass username in unconditionally now anyway. @@ +329,5 @@ > + result = pam_get_user (handle, &username, "Username: "); > + if (result != PAM_SUCCESS) > + return result; > + > + filename = g_strdup_printf (USERDIR "/%s.encrypted-password", username); Right we have icons/[username] and users/[username] . a new file should probably try to be consistent.
(In reply to comment #12) > Comment on attachment 63943 [details] [review] [review] > Add a mechanism to set the PIN > > Review of attachment 63943 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: src/pam-password-helper.c > @@ +45,4 @@ > > into each question, and hope that configuration is > > correct. */ > > for (i = 0; i < num_msg; i++) { > > + if (strcmp((*msg)[i].msg, "PIN") == 0) > > I really don't like how we're using translatable messages as machine > identifiers. Honestly, I'd rather if the PIN verification worked via some > non-pam side channel, and the PAM module would just block until it got a go > ahead before setting the AUTHTOK to the user's password and dropping to the > next module in the stack. That's more work, though, and I know last time I > brought it up you didn't like it much. > > Another, less invasive idea is to have the pam module set PAM_AUTHTOK_TYPE > to "PIN" and have this code check it with pam_get_item Sorry, but I really dislike the idea of a middle man sitting on system dbus. It makes sense for fprint, because there the daemon is accessing the HW directly and has all the knowledge to report the result on its own. Here, the daemon would be setting up a private dbus connection to the shell, grab the PIN, then wait for another private connection from the PAM module to pass it on. And we can't have a direct connection from the module to the shell, because you can't pass the identifier on, and you can't use a well-known system bus name. PAM_AUTHTOK_TYPE seems as wrong as the prompt label (they're both translatable strings), and I need new API in gdm to get PAM_AUTHTOK_TYPE down to the shell. (In reply to comment #13) > Comment on attachment 63944 [details] [review] [review] > Add PAM module for handling PIN authentication > > Review of attachment 63944 [details] [review] [review]: > ----------------------------------------------------------------- > > I've started to look over this, but want to look deeper. I will look more > in a bit. > > ::: data/accountsservice > @@ +14,5 @@ > > +session required pam_namespace.so > > +session optional pam_keyinit.so force revoke > > +session include password-auth > > +session include postlogin > > + > > Why are there session modules in this file? Are we allowing PIN for initial > login too? > > Different distros have different service file formats so we'll probably need > to do what we do with GDM and have it configure time pluggable. Right. What about a configure switch to choose the stack to use instead, and have someone else (GDM?) provide the stack files? If any, it would make consistent what happens when you change the password through accountsservice and when you're forced to change it at login. > ::: src/pam-pin/pam-module.c > @@ +325,5 @@ > > + char *ciphertext; > > + size_t ciphertext_len; > > + int result; > > + > > + result = pam_get_user (handle, &username, "Username: "); > > This could probably be NULL since I think we pass username in > unconditionally now anyway. I wrote this with the idea of using it from passwd too, because control-center spawns that for the own password. But now that I look at this, pam-pin requires root privileges, so it must go through accountsservice, whereas control-center needs to bypass it to update the keyring password. Ugh...
(In reply to comment #14) > (In reply to comment #12) > > Comment on attachment 63943 [details] [review] [review] [review] > > Add a mechanism to set the PIN > > > > Review of attachment 63943 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: src/pam-password-helper.c > > @@ +45,4 @@ > > > into each question, and hope that configuration is > > > correct. */ > > > for (i = 0; i < num_msg; i++) { > > > + if (strcmp((*msg)[i].msg, "PIN") == 0) > > > > I really don't like how we're using translatable messages as machine > > identifiers. Honestly, I'd rather if the PIN verification worked via some > > non-pam side channel, and the PAM module would just block until it got a go > > ahead before setting the AUTHTOK to the user's password and dropping to the > > next module in the stack. That's more work, though, and I know last time I > > brought it up you didn't like it much. > > > > Another, less invasive idea is to have the pam module set PAM_AUTHTOK_TYPE > > to "PIN" and have this code check it with pam_get_item > > Sorry, but I really dislike the idea of a middle man sitting on system dbus. You're doing the work, so I'm not going to press the issue, too much. But I'll press a little ;-) : > It makes sense for fprint, because there the daemon is accessing the HW > directly and has all the knowledge to report the result on its own. The point of it isn't "needs hardware access like fprintd". The point is, "avoid shoe horning the mechanism into a PAM interface that isn't rich enough to support it." It's not the end of the world, either way, mind you... > Here, > the daemon would be setting up a private dbus connection to the shell, grab > the PIN, then wait for another private connection from the PAM module to > pass it on. > And we can't have a direct connection from the module to the shell, because > you can't pass the identifier on, and you can't use a well-known system bus > name. I would imagine it could go something like this: -Shell creates a PIN pad on screen. -Shell starts user verification with "gdm-pin" pam service. The PAM module calls WaitForPassword() on org.freedesktop.AccountService.PINVerifier that makes it block until the user enters a PIN -User enters PIN with their finger on the shell pin pad -Shell calls EnterPIN() on org.freedesktop.AccountService.PINVerifier with the PIN code entered by the user. -accountsservice verifies the PIN and returns the password from WaitForPassword() call -PAM module sets the password as the AUTHTOK for the next module in the stack and then finishes The PAM module/PAM interaction would be very thin. Just some scaffolding so we can stack other modules under. Anyway, that's how I'd do it. > PAM_AUTHTOK_TYPE seems as wrong as the prompt label (they're both > translatable strings), and I need new API in gdm to get PAM_AUTHTOK_TYPE > down to the shell. It is documented as translatable, but it can't really be translated properly because it's a single word that gets plugged into sentences, so it's not really used in practice. Granted, it's not a great fit either. PAM doesn't have a good fit, which is why i'd rather use side-channel communication. > Right. What about a configure switch to choose the stack to use instead, and > have someone else (GDM?) provide the stack files? Right now, we do have some dependencies on GDM in accountsservice (for instance autologin), but the plan as always been to flip that, and move those things into accountsservice and change GDM to use accountservice. So, if you want to consolidate PAM files, I think i'd rather do it the other direction. Alternatively, we could put the PAM module in the GDM source. Then everything is together. > But now that I look at > this, pam-pin requires root privileges, so it must go through > accountsservice, whereas control-center needs to bypass it to update the > keyring password. Ugh... /usr/bin/passwd is setuid and I'm pretty sure pam_gnome_keyring already updates the password when you run passwd, so I think things should be okay?
Attachment 63944 [details] made me go eek. Is there a reason why the PIN should be passed in clear from the shell's screensaver to accountsservice? I would use accountsservice to set the "PIN" by passing it encrypted from the UI to be saved (that's the way normal session passwords are passed, the UI would be responsible for ensuring the password is sane), and have a simple pam module[1] to handle verification in a separate stack (a-la gdm-fingerprint/gdm-smartcard/etc.) I might be missing something blatant here. It would mean making 3 of the 5 patches posted so far obsolete. [1]: Ideally, the separate PAM module would be pam_unix fork to use a different shadow file, but that might be more work than it's worth.
(In reply to comment #16) > I would use accountsservice to set the "PIN" by passing it encrypted from > the UI to be saved (that's the way normal session passwords are passed, the > UI would be responsible for ensuring the password is sane). I assume my encrypted you mean hashed with crypt(). That doesn't work because it's one way, and we need access to the plain text pin to decrypt the password to propagate it to other modules in the stack. But thanks to Stef, we do have GcrSecretExchange which does diffie-hellman key exchange to securely pass the password around. That's clearly the right way forward. Giovanni has done work torward that end in comment 10
Right, the cleartext SetPassword is just to avoid squashing the Gcr changes togheter, while keeping every commit testable. gnome-control-center must move to BeginSetPassword/ContinueSetPassword as soon as possible after this commit lands. If the security risk of unencrypted SetPassword is unacceptable, I'm fine with its removal. It might even be possible to rewrite act_user_set_password in terms of set_multiple_passwords. Accessing the plain text password in accountsservice is not only needed for PIN handling, it is needed for all cases where the password is not stored in /etc/shadow (as the crypt() path used usermod, not PAM). Using a side channel in place of PAM misses the usual point: gdm-fingerprint does not prompt the user. Smartcard (pam_pkcs11) uses a standard PAM prompt for the card PIN. We cannot just fork pam_unix in pam_pin, because we want to be able to retrieve the full Unix password from the PIN. This is later given to pam_gnome_keyring for automatic unlock. Anyway, I rebased the series to follow a more logical succession.
Forgot to add, the rebased series does not include the side channel, because I'm not yet convinced of its benefit. It would be possible to add it as a new commit in any case.
Created attachment 72741 [details] [review] Use PAM to change passwords
Created attachment 72743 [details] [review] Use GcrSecretExchange to pass the password from the client to the daemon
Created attachment 72744 [details] [review] Add PAM module for handling PIN authentication
Created attachment 72745 [details] [review] Add a mechanism to set the PIN
(In reply to comment #18) > Right, the cleartext SetPassword is just to avoid squashing the Gcr changes > togheter, while keeping every commit testable. I'd say we should either drop SetPassword or fix it if we need backward compatibility (by say frobbing the spwd struct ourself instead of using usermod). I don't think we should change the semantics of a function in an incompatibile way right before we replace it. > Using a side channel in place of PAM misses the usual point: gdm-fingerprint > does not prompt the user. Smartcard (pam_pkcs11) uses a standard PAM prompt > for the card PIN. PAM doesn't have a way to ask software to display a software pin pad though. Smartcard "pin" isn't actually a pin, it's just a passphrase. We don't need to show a pin pad for it, so the PAM interface is sufficient. This PIN pad feature is special because it's asking for information in a way PAM doesn't support asking. Anyway, i'm not going to argue this anymore. > Anyway, I rebased the series to follow a more logical succession. Thanks.
Whats the next step here ?
I may have found a solution that might solve the "identifier looks like a translatable string" problem: we can add a prefix to the identifier that is valid Unicode and is handled fine by legacy apps that don't recognize it, but doesn't logically make sense in that context. The right character is U+FEFF ZERO WIDTH NO-BREAK SPACE (\357\273\277), better known as the byte order mark. This character will not appear at the beginning of any translated string, because we use UTF-8, but applications not recognizing it will ignore it, because it's a zero width space. Then, we can look for it at the shell level, and interpret the rest of the string as a machine identifier if found.
Why do we need to look at the prompt string at all? can we just assume if it's coming from the pin service it's going to expect a pin code response?
I was thinking to have pam_pin inside gdm-password, not as a separate service (it returns PAM_AUTHINFO_UNAVAIL and gets skipped if the PIN is not configured). But even with a separate service, you need to handle all possible prompts because one account module might require a password change (so you run the password stack and end up with a prompt from pam_unix).
We should really come to a conclusion here, one way or other
Well i'm not happy with it as it is, but i'm not going to block it either. if we need to do this for now to get the feature in we can, I guess.
Created attachment 74904 [details] [review] Remove deprecated and unsafe SetPassword
Ok, with the last patch, plus some minor fixes here and there, I finally managed to test the whole stack in a VM. Now, for the appropriate integration we need: 1) To add password optional pam_pin.so to password-auth/system-auth/common-password This needs to be done downstream (i.e. in Fedora we would change authconfig), because: - gnome-control-center should launch passwd directly for the same user case (to handle gnome-keyring, and to avoid polkit) - pam-pin must be after pam-unix to see the right password, but password-auth by default ends with pam_deny, so if pam_pin is after the whole substack it is never reached Alternatively, we could make pam_pin work when configured before pam_unix, and make gnome-control-center spawn something better than passwd. 2) To have an UI for setting the PIN in gnome-control-center. This is still missing a complete design, but gnomebug 693920 has an initial implementation. 3) To fix gdm-password to add auth [success=ok new_authtok_reqd=ok ignore=ignore authinfo_unavail=ignore default=bad] pam_pin.so before calling out to password-auth/common-auth or to have a gdm-pin with auth required pam_pin.so before password-auth/common-auth In the latter case, we also need a selector in gnome-shell (current code shows a PIN pad when asked, no matter from where)
Turns out I was wrong, pam_pin can be added at the beginning of accountsservice just fine. which reduces the required changes to accountsservice and gdm. Next: to make the pam password helper usable by gnome-control-center...
Created attachment 74912 [details] [review] Use PAM to change passwords
Created attachment 74913 [details] [review] Use GcrSecretExchange to pass the password from the client to the daemon
Created attachment 74914 [details] [review] Add PAM module for handling PIN authentication
Created attachment 74915 [details] [review] Add a mechanism to set the PIN
Created attachment 74916 [details] [review] Remove deprecated and unsafe SetPassword
Created attachment 74917 [details] [review] password-helper: allow running as non-root
Created attachment 74919 [details] [review] password-helper: allow running as non-root Now with a giant hack around libgcrypt brokenness!
Giovanni, thanks for this work! I'm from Ubuntuland, looking at this patch. I have a couple potential patches on top of your work for your consideration: 1) When checking for machine-id, also check legacy location /var/lib/dbus/machine-id for those platforms that haven't switched to /etc/machine-id (like Ubuntu). Something like adding the following to the existing check: && !g_file_get_contents ("/var/lib/dbus/machine-id", &machine_id, &machine_id_len, NULL) 2) In /etc/pam.d/accountsservice, Ubuntu uses 'common-password' instead of 'password-auth'. It would be nice if there were a way to specify the substack name at configure time or similar such workaround that doesn't involve patching. 3) I ran into a problem with sudo not working when pam_pid was used. It was because pam_pid was initializing libgcrypt one way, and openldap (pulled in from pam_pkcs11) needed it to be another way. The easiest solution I saw was just to initialize libgcrypt the same way. Which involves calling gnutls_global_init. To do so, replace the contents of your init_libgcrypt() with: /* gnutls_global_init () will initialize gcrypt for us a certain way. We initialize gnutls instead of gcrypt because other pam modules may use gnutls and we need to initialize gcrypt the same way. */ gnutls_global_init (); And add the following right after AM_PATH_LIBGCRYPT in configure.ac (along with using the related vars in src/pam-pin/Makefile.am): PKG_CHECK_MODULES(LIBGNUTLS, gnutls, [enable_pin_authentication=yes], [enable_pin_authentication=no]) Besides those issues, it works fine. Thanks again! (Oh, I should mention for anyone else wanting to play with this, that you should make sure pam_unix is configured with the try_first_pass flag, else it won't be able to see the password that pam_pin sets for it.)
When unlocking gnome-screensaver, the encrypted-password files need to be user-readable in my testing. How does that work with pam_unix normally? A setuid helper binary to read its shadow file?
Yes, pam_unix uses /sbin/unix_chkpwd if euid != 0, but note that pam_pin is designed explicitly with one use case in mind: accountsservice and gdm, and both run the PAM stack as root. Anything else is unsafe (5 digits numeric pins are quite easy to brute-force), so I'm personally against a setuid binary. (For the record, I thought that ubuntu moved the screensaver to lightdm, thus deprecating gnome-screensaver and PAM-as-non-root forever)
Yes, it's deprecated. We've been planning to move the screen lock to lightdm for a long time, but it hasn't happened yet. Not working in gnome-screensaver is fine; just wanted to make sure it was expected behavior.
Another thought. Is there a particular reason to drop SetPassword from the DBus API? It'd be nice to keep it in for existing consumers. When used, it could just unset any current PIN.
The reason for dropping SetPassword is that, for bug #55002, we want to go through PAM to change passwords (as that handles enterprise setups), but we can't do that with a crypt-ed password.
Fair enough. I guess I just was expecting those two fixes to be decoupled. The change to drop that method seems like it needs more surrounding work. Like I imagine we would want to increment the SONAME of libaccountsservice if we're dropping symbols from its library. Maybe version the DBus API too, like systemd does and offer org.freedesktop.Accounts.User2 or some such.
Hi Matthias, Hi Giovanni, whats the current status of this patch series? I'm looking into PIN handling for my GNOME tablet and stumbled upon this important bug report. Any progress in the last 4 years?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/accountsservice/accountsservice/issues/25.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.