Given that we have the protocol and don't use the legacy conv any more
there is no need to go through exit code mapping. By not using exit code
we can start reusing one kcheckpass for multiple auth request and turn it
into a kind of daemon, which is a requirement for enabling seccomp in
kscreenlocker_greet.
Details
- Reviewers
romangg - Group Reviewers
Plasma - Commits
- R133:d666fe879d46: [greeter] Send the auth result through the server instead return value
Tested with the new test helper
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.
ping! This is security relevant code. I don't want to push without anybody looking at it!
Since there seem to be not many others I'll try to give you reviews for this and your following patches, but first I've to understand this stuff.
Tell me if the following is right:
Until now the greeter waited for the exit code of the forked process with the pid m_pid. The child process executes the kcheckpass binary via execlp for pam/etcshadow password validation and exits immediately. The parent process now reads the requests and results from kcheckpass via socket m_fd. It gets notified when kcheckpass has written something to the socket thanks to m_notifier.
First kcheckpass's pam/etcshadows part asks for the provided password, i.e. transmits ConvGetNormal / ConvGetHidden, so KCheckPass::handleVerify writes back the provided password to the socket. kcheckpass can apparently without having to wait directly read it from the socket. Pam/etcshadow compares it with the set user password and (until now) exits with the respective success or fail code.
With your change instead of setting an exit code conv_server writes back one more time to the socket with the result (i.e. only the enum signaling it with empty prompt).
EDIT: Follow up question: In the current version at which point in the execution is the reapVerify method called at all? handleVerify is only connected to the activated method of QSocketNotifier and I assume that kcheckpass writes to the socket in the end and handleVerify is called one last time with GRecvInt( &ret ) being false. But I don't see any more writes to the socket in kcheckpass.c and for example checkpass_shadow.c after the initial query for the provided password.
greeter/authenticator.cpp | ||
---|---|---|
221 | When you're at it: This seems to do the exact same thing as in ConvGetNormal case. Code duplication? | |
270 | When we have already received ConvPutAuthSucceeded this would mean that we first emitted succeeded and later failed, which could lead to problems. Just delete the whole reapVerify method and end m_notifier as well as close m_fd in handleVerify. |
yes, the exit code was the auth result. exit code 0 means auth success, everything else failure.
The child process executes the kcheckpass binary via execlp for pam/etcshadow password validation and exits immediately. The parent process now reads the requests and results from kcheckpass via socket m_fd. It gets notified when kcheckpass has written something to the socket thanks to m_notifier.
yes
First kcheckpass's pam/etcshadows part asks for the provided password, i.e. transmits ConvGetNormal / ConvGetHidden, so KCheckPass::handleVerify writes back the provided password to the socket. kcheckpass can apparently without having to wait directly read it from the socket. Pam/etcshadow compares it with the set user password and (until now) exits with the respective success or fail code.
yes
With your change instead of setting an exit code conv_server writes back one more time to the socket with the result (i.e. only the enum signaling it with empty prompt).
yes
EDIT: Follow up question: In the current version at which point in the execution is the reapVerify method called at all? handleVerify is only connected to the activated method of QSocketNotifier and I assume that kcheckpass writes to the socket in the end and handleVerify is called one last time with GRecvInt( &ret ) being false. But I don't see any more writes to the socket in kcheckpass.c and for example checkpass_shadow.c after the initial query for the provided password.
reapVerify seems to me to be only if the communication horribly breaks. I did not fully understand the code and what it was doing. Part is to wait for the auth result, part is further error checking.
In the long run I want to replace this by a nice QProcess, but one step at a time. This change is actually only a preparation step for the follow up which has the long running kcheckpass.
greeter/authenticator.cpp | ||
---|---|---|
221 | not unlikely. That code is old and got carried around. |
I don't think that's the case, because otherwise greeter could've never read in the exit code of kcheckpass on current master.
It's more like this: In kcheckpass.c the method conv_server() first sends GSendInt(), which applies to one of the cases in handleVerify(). Then (in the cases ConvGetNormal and ConvGetHidden) conv_server() sends again a string via GSendStr. For checkpass_shadow.c this is the 0 string, which then falls through to reapVerify().
I mean in the new code. In the old one: yes it was relevant to handle the authentication.
kcheckpass/kcheckpass.c | ||
---|---|---|
203 | This line (and GSendArr(len, prompt) above) writes (in case of shadow auth 0) to the socket, which should lead to reapVerify() being called and the socket being closed before you can read in ConvPutAuthSucceeded* after Authenticate(..) finished. |
Tested on X and Wayland.
kcheckpass/kcheckpass.c | ||
---|---|---|
203 | I tested it with debug lines, and it works as intended. I assume handleVerify() isn't called a second time, because the fd wasn't written on in between by the greeter yet. |