I will answer each of your questions below after providing more context and some notes of my own.
First and foremost, the patch works by distinguishing between a null
password string and an empty password string. A user may only enter the
latter, and, thus, the former may be used to indicate some kind of
problem in the data-exchange between the greeter and kcheckpass.
In this case, we send the password from the greeter to kcheckpass once
(sufficient for authentication) and then nullify it. If kcheckpass asks
for the password again before indicating "ready for authentication", a
null password will then be passed, and the PAM conversation will be
aborted by kcheckpass.
Much more could be done to improve the screen locker above and beyond
this patch, but I chose this method because:
1. It is not a breaking change for other PAM modules.
2. It directly fixes the problem with Poldi and is well-tested by
myself and my colleagues.
3. It has a minimal impact/footprint on the code.
On 9/27/19 1:44 PM, Maximiliano Curia wrote:
> In particular, the fallthrough ends up calling the same code on abort
> and on ready, which sets the m_ready variable and sends a USR1 signal on
> direct mode, what's the idea here?
This actually makes sense. If a conversation is aborted (i.e., cut
short) without a lock out or failure, there is nothing for the locker to
do but start over. That is, return to the state of waiting for user
input. This explains the fallthrough.
> This ends up with one or two USR1 signals being sent to kcheckpass,
> can you please explain why is this needed so that kcheckpass lets pam
> process the smartcard input?
Remember that this is on the greeter side! Kcheckpass has already
aborted the conversation at this point because the password was null.
The goal is simply to treat an aborted conversation as though we are
just returning to the state of waiting for user input. This is a very
simple way of accomplishing this.
> Why does PAM_data.abort need to be zeroed?
PAM_data is static data. If this is not zeroed, it will retain its
value and future errors will be interpreted as though the conversation
were aborted.
Thus, we zero it as soon as we handle it.
> Previously, the pam_error was hidden and pam_end was called with
> PAM_SUCCESS. Was that wrong?
It is my understanding, from reading the Linux-PAM documentation, that
this is always wrong.
The signature of pam_end() is:
int pam_end(pam_handle_t *pamh, int pam_status);
and the docs clearly state (see "man pam_end"):
The pam_status argument should be set to the value returned to the
application by the last PAM library call.
I would go so far as to say that this should be fixed everywhere, but I
decided to limit the scope of my patch and only fix it in blocks that I
actually touched.
--
Jason Franklin
signature.asc
Description: OpenPGP digital signature
