pam-u2f icon indicating copy to clipboard operation
pam-u2f copied to clipboard

authtok: add support for decrypting an auth token

Open zhihengq opened this issue 1 year ago • 12 comments

Add support for storing an encrypted password in u2f_keys. During authentication, the password is decrypted and passed to PAM for other PAM modules.

Problem

When logging in with pam_u2f as a single factor, gnome-keyring cannot be auto-unlocked. This is because no password was provided during authentication, but gnome-keyring requires the user password to decrypt its database. Another user reported the same problem in #283.

Solution

This PR allows pamu2fcfg to optionally store a password for each authenticator. During authentication, pam_u2f retrieves the password and pass it to PAM as PAM_AUTHTOK. Subsequent PAM modules such as pam_gnome_keyring will use the password in PAM_AUTHTOK to auto-unlock the keyring.

Passwords are encrypted with AES-256-GCM and stored in the u2f_key file, along with an HMAC salt. The AES encryption key is derived using the FIDO2 HMAC-SECRET extension.

New arguments added:

  • pam_u2f:
    • allowauthtok: an optional argument that enables this feature.
  • pamu2fcfg:
    • -a, --authtok: an optional flag to prompt the user for a password to be stored.
    • -p, --pam-arguments=STRING: an optional argument that specifies the arguments passed to pam_u2f. This is used to ensure users are authenticated in the same way when getting the HMAC secret.

Next steps

Update the documentation for this feature.

zhihengq avatar Apr 30 '23 08:04 zhihengq

Hi,

First of all, thank you for the contribution!

I have given this a brief look and some thinking and wanted to write those down. As pointed out in the linked issue, there's a set of complexities here I'm worried about:

  • On a password change, the user would have to encrypt the new secret for each of their registered authenticators.
  • If the UV requirements changes in the service configuration or authfile, the same problem applies.

The first source of problems could potentially be partially alleviated by implementing pam_sm_chauthtok(), but would bring in a lot of new code. There's no hooks for us to detect the second problem and it could be worth considering to e.g. always require some form of UV for this feature as a workaround.

In any case, I think we'll have to carefully consider these consequences, how to deal with them, and whether the added complexity is worthwhile before being able to move forward. Any thoughts or suggestions?

LDVG avatar May 08 '23 07:05 LDVG

I don't think we should implement pam_sm_chauthtok() because:

  1. For pam_sm_chauthtok() to work, we need the authenticator to be present to encrypt the new password. And I don't feel that requiring the authenticator is reasonable when the user simply wants to change their password.
  2. The auth token that the user wants to set might not be the account password. One such case is when using full-disk encryption, a user might want to set the gnome-keyring password to be the same as the disk encryption password to achieve auto-unlocking. In this case, changing the account password shouldn't overwrite the auth token saved in the authfile.

Since we also can't verify if the current auth token still works with gnome-keyring (or any other service that the auth token is for), there is not much we can do here.

For UV requirement change, we can only detect this case during the next authentication by failing to validate the AEAD tag. At this point the encrypted auth token is already lost. The best we could do is to skip the auth token and report this error to the user.

In both of the cases, the user has to manually set up the auth token again with pamu2fcfg. The solution is definitely not ideal, but I can't come up with anything better. We could potentially change pamu2fcfg to support updating existing auth tokens, but I'm not sure if the benefit is worth the added complexity. That being said, I still think this feature would be helpful to some users, since entering password for gnome-keyring defeats the purpose of using YubiKeys in a passwordless setup.

zhihengq avatar May 11 '23 07:05 zhihengq

Thank you for the feedback.

  1. For pam_sm_chauthtok() to work, we need the authenticator to be present to encrypt the new password. And I don't feel that requiring the authenticator is reasonable when the user simply wants to change their password.

Marking the module as optional could help for this, I suppose.

  1. The auth token that the user wants to set might not be the account password. One such case is when using full-disk encryption, a user might want to set the gnome-keyring password to be the same as the disk encryption password to achieve auto-unlocking. In this case, changing the account password shouldn't overwrite the auth token saved in the authfile.

Since we also can't verify if the current auth token still works with gnome-keyring (or any other service that the auth token is for), there is not much we can do here.

Good point. For some reason, this use-case escaped me and, as you say, is probably sufficient reason to not do something like that -- at least not without somehow knowing that the user wants to wrap their login credentials.

For UV requirement change, we can only detect this case during the next authentication by failing to validate the AEAD tag. At this point the encrypted auth token is already lost. The best we could do is to skip the auth token and report this error to the user.

Ack.

In both of the cases, the user has to manually set up the auth token again with pamu2fcfg. The solution is definitely not ideal, but I can't come up with anything better. We could potentially change pamu2fcfg to support updating existing auth tokens, but I'm not sure if the benefit is worth the added complexity.

Agreed. At least for a potential first stab at this, the preference would be to keep it as simple as possible and manually dealing with the problem using pamu2fcfg would probably be the way to go (as cumbersome as that may be).

I have not yet had time to take a closer look at your implementation or try it out for myself. I hope to be able to do so soon.

LDVG avatar May 16 '23 13:05 LDVG

hi, did test out this pr and it seems to address exactly the remaining issue I have (full debian login with fido2/u2f token) what is preventing this pr from being merged? May I help in some way?

note: did successfully test this:

  1. install this patched version instead of system pam on my debian 12
  2. modify /etc/pam.d/common-auth to contain auth [success=ok new_authtok_reqd=ok ignore=ignore default=ignore] pam_u2f.so userpresence=1 userverification=0 pinverification=1 allowauthtok (do not use sufficient, it will prevent gnome-keyring from being unlocked)
  3. generate the yubikey file in ~/.config/Yubico/u2f_keys withoutput from pamu2fcfg -a (enter PIN, press button, enter login password, press button)
  4. reboot, login using GDM with PIN and yubikey only

akallabeth avatar May 07 '24 19:05 akallabeth

@LDVG I think I addressed your previous concerns. Could you take another look? Thanks!

zhihengq avatar May 08 '24 06:05 zhihengq

@zhihengq only thing I found a bit lacking is the debug output. maybe add 2 more lines to tell if

  1. the authtoken will be used (not the config setting, but if it is actually tried to decrypt)
  2. if the authtoken was successfully decrypted

akallabeth avatar May 08 '24 07:05 akallabeth

as an idea (most likely for a follow up pull) maybe it would also be possible to implement pam_sm_chauthtok to update the data in the mapping on password change?

akallabeth avatar May 08 '24 18:05 akallabeth

@zhihengq only thing I found a bit lacking is the debug output. maybe add 2 more lines to tell if

1. the `authtoken` will be used (not the config setting, but if it is actually tried to decrypt)
2. if the `authtoken` was successfully decrypted

That's a great idea! Could you help me put it in? Or I can do that when I have some time.

as an idea (most likely for a follow up pull) maybe it would also be possible to implement pam_sm_chauthtok to update the data in the mapping on password change?

I'm not exactly sure how to make it work. Changing the auth token requires presence of the fido2 device (and possibly human interaction) to encrypt the new token. What happens if the fido2 device is not present?

zhihengq avatar May 17 '24 04:05 zhihengq

1. the `authtoken` will be used (not the config setting, but if it is actually tried to decrypt)
2. if the `authtoken` was successfully decrypted

That's a great idea! Could you help me put it in? Or I can do that when I have some time.

ok, will push to your branch.

as an idea (most likely for a follow up pull) maybe it would also be possible to implement pam_sm_chauthtok to update the data in the mapping on password change?

I'm not exactly sure how to make it work. Changing the auth token requires presence of the fido2 device (and possibly human interaction) to encrypt the new token. What happens if the fido2 device is not present?

well, same as other PAM modules I guess, depending on configuration? e.g. if it is required changing fails, if it is optional changing succeeds even if the token did not?

akallabeth avatar May 17 '24 06:05 akallabeth

ok, can´t push to your branch, so here is the patch:

patch.zip

akallabeth avatar May 17 '24 06:05 akallabeth

@LDVG anything left that is blocking this pr? using it successfully on a couple of systems already so would love to get this upstream.

akallabeth avatar Aug 08 '24 13:08 akallabeth

Hi, thanks for the ping.

While we believe this is a good experimental start if people are finding it useful, this PR is rather big both in terms of LOC and the number of configuration knobs it introduces. Coupled with UX quirks related to e.g. changing your password and having to pass module arguments to pamu2fcfg, that we might want to figure out before shipping something like this, the maintenance cost of this is rather high -- and we have consequently given this feature a low priority.

Ideally, we'd like to first understand how we can minimize the footprint of this feature. For example, in terms of code size, this may require first refactoring some parts of pam-u2f (e.g. to move assertion logic so that pamu2fcfg can reuse it). As I alluded to in my early reviews of this PR, one way of going about it could be to split this PR into smaller parts in which we can discuss each problem at a time and test things separately.

Again, thanks for the contributions and interest in pam-u2f!

LDVG avatar Aug 09 '24 05:08 LDVG