Do not allow the greeter to send the same password multiple times (Fix GPG smart card access)
ClosedPublic

Authored by jfranklin on Sep 10 2019, 7:29 PM.

Details

Summary

Greetings,

I am working on improving some of the functionality in the screen locker. Specifically, I am attempting to fix this bug reported to the gnupg-devel mailing list. See also a bug that I reported to the Debian package maintainer: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=934185. That bug report has not received a response, so I've come here.

Some background: The libpam-poldi package for Debian allows for 2FA with GnuPG smart cards into a Linux workstation. This functionality is very imporant to me and to my organization. This patch will fix a problem with the conversation between Poldi and KScreenLocker.

To reproduce the bug using libpam-poldi and the screen locker:

  1. arrive at the screen locker
  2. remove and re-insert the GPG smart card
  3. enter a PIN less than 6 characters in length
  4. Bam! The screen locker will stop responding and your /var/log/auth.log file will fill up with "PIN too short" messages

For GPG smart card users, this issue is critical, because it means the machine can become unusable if you accidentally hit enter too early.

I will summarize my solution here:

  1. The libpam-poldi maintainer discovered that a PAM_TEXT_INFO message followed by a PAM_PROMPT_ECHO_OFF from the PAM module would result in an infinite loop in the conversation between kcheckpass and the greeter.
  2. The infinite loop was caused by the fact that the greeter would keep sending the password that was initially entered over and over without prompting the user again, even though the PAM module was requesting another prompt.
  3. Seeing this, I searched for a compact way to ensure that the password would only be passed to kcheckpass once and that passing another password would *require* a prompt.
  4. This was accomplished by clearing the password after sending it from the greeter to kcheckpass.
  5. Now, a subsequent request for another password will return the nullptr, which will abort the current PAM conversation. Naturally, this has the same effect as starting the conversation over again (we're at the screen locker, after all), so we continue waiting for input again. This explains the fall through in the switch statement (see diff).

I worked hard to come up with a solution that would have a minimal impact on the code. I think this patch accomplishes this, and it certainly fixes my immediate problem. It also does not seem to introduce any regressions. I hope testing will reveal these if they are there.

Thanks,
Jason Franklin

Test Plan

I need all the help testing/reviewing this that I can get. It works very well in my set up so far.

Diff Detail

Repository
R133 KScreenLocker
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jfranklin created this revision.Sep 10 2019, 7:29 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 10 2019, 7:29 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
jfranklin requested review of this revision.Sep 10 2019, 7:29 PM

I also didn't point this out in my original post, but I fixed a misuse of pam_end(). The status passed to pam_end() should be the returned status of the last PAM library call. This is noted in the documentation for Linux-PAM.

Friendly ping concerning this issue. Has anyone related to the screen locker project had some time to take a quick look at this?

zzag added a comment.Sep 16 2019, 1:12 PM

This change looks good to me, however I know only general bits of KScreenLocker.

Perhaps someone else from KWin or Plasma could review this patch.

zzag added a comment.Sep 16 2019, 1:25 PM

@jfranklin Ping me if you don't get any feedback in a day or two.

In D23849#532568, @zzag wrote:

@jfranklin Ping me if you don't get any feedback in a day or two.

Hey there @zzag, haven't seen any activity here. Just pinging you per your request. Thanks!

Thanks for the patch Jason. So the GSendStr(nullptr) is called on m_password.isNull() because this aborts the current PAM session? If this is correct, patch looks good to me.

zzag added a comment.Sep 18 2019, 7:45 PM

Thanks for the patch Jason. So the GSendStr(nullptr) is called on m_password.isNull() because this aborts the current PAM session? If this is correct, patch looks good to me.

Yes, current kscreenlocker architecture doesn't allow prompting the user.

Thanks for the patch Jason. So the GSendStr(nullptr) is called on m_password.isNull() because this aborts the current PAM session? If this is correct, patch looks good to me.

That's correct!

The idea is to differentiate between an empty password (possibly sent by the user) and a null password (sent by the greeter). This allows for the conversation to abort when the greeter says so (by sending nullptr), yet it still allows the user to send an empty password.

Ok, you need this for 5.17?

romangg accepted this revision.Sep 18 2019, 8:01 PM

Or let's just get it in. It's a small change and we still have the beta phase to test it.

This revision is now accepted and ready to land.Sep 18 2019, 8:01 PM

Or let's just get it in. It's a small change and we still have the beta phase to test it.

Fantastic. Yep, sooner is better in our case.

+1. Do you wanna commit this, or should I?

@jfranklin can we use Jason Franklin <jason.franklin@quoininc.com> as your email address for the git authorship information?

+1. Do you wanna commit this, or should I?

Are you referring to me? I assume not, just making sure...

@jfranklin can we use Jason Franklin <jason.franklin@quoininc.com> as your email address for the git authorship information?

That's perfect.

This revision was automatically updated to reflect the committed changes.

Thanks so much for the patch, Jason! Feel free to keep on submitting more if you encounter any further issues.