Improve error reporting on authentication/authorization failure

[matt335672]

This discussion is prompted by this conversation. It's not directly relevant to the main issue, but I think raises an important point.

At the moment the SCP V0 interface returns a single value representing the success or failure of the session creation process. As @danielperna84 points out, this makes it difficult to work out why a login may not be succeeding, which in turn can lead to increased issues being reported here which don't need to be.

My main concern is that presenting too much information to a user (for example a PAM code) is bad from a security perspective as we're revealing more information about the users on the system than we need to.

I was wondering if we simply returned an enumerated type from SCP V0 along with the DISPLAY number we could be a little more helpful without compromising security. Here's a suggested list:-

• Success
• Authentication failure (i.e. invalid credentials)
• Authorization failure (e.g. user isn't allowed to log on this way, or at this time)
• Other failure (system configuration, software error, etc)

All of these can be handled by the existing auth mechanisms, although PAM is used by most of our users.

Is this revealing too much information, or too little, or is it about right?

[danielperna84]

To me your list seems about right. Providing more detail in the other case would be helpful for debugging, but it would probably reveal critical details about how the system is configured. And in such failures an admin has to look into the logs anyways.

If I might add one more idea to this conversation before setteling on doing this the simple way and have more work later on: as PAM is able to communicate back and forth, wouldn't it make sense to properly support PAM_PROMPT_ECHO_OFF, PAM_PROMPT_ECHO_ON, PAM_ERROR_MSG and PAM_TEXT_INFO? The first two could allow PAM-based MFA (there have been issues about TOTP after all), and the last two seem to be the methods that should be used to communicate with the user.

Regarding the two prompts I assume that could collide with the current implementation for providing the credentials as a user. But I guess it would be neat if that would be supported. With my PAM written in Python I actually fetch the TOTP-secret for the user logging in, then in startwm.sh I display a prompt using zenity to check his TOTP, and depending on the input continue or exit to abort the login (which technically has already succeeded at that point). An ugly workaround, but it does the job for now.

The other two message types seem to be an alternative (or addition) to the initial idea of this discussion. I agree that the user doesn't have to see the specific error-code. But if the PAM decides to display an error- or infomessage to the user (which it could do when not used in xrdp), then it would be the duty of the PAM to handle security / privacy, and xrdp would just provide the framework to display what the PAM has to say.

That being said, I got a feeling that this kind of bi-directional communication between PAM and the login-screen could be very tricky to implement. Hence I'd understand if this part was rejected. In that case a general message based on the PAMs returncode would be the next best thing. 👍

[matt335672]

You're right about the trickiness of this - we need to make some architectural changes to really sort this out.

The largest issue is that xrdp-sesman isn't forked when calling into the PAM stack. This means an callback to the user (which can take an arbitrary amount of time to resolve) would prevent other users from logging on (i.e. a denial-of-service). Just forking isn't really an option as there are also restrictions imposed upon the PAM stack by systemd which we haven't currently addressed (see #1684) which have a bearing on this.

It's certainly fixable, but there are workarounds and there are more urgent issues requiring effort.

For now I think we'll go for the simpler solution!

[danielperna84]

Thanks for clarifying the problems. And yes, the simpler solution would still be a great improvement. I just wanted to share my thoughts while we're at it. Thanks for taking the time to talk about this. 😇

[matt335672]

See also #1947 which is peripherally related to this discussion.

[matt335672]

See #2472 which seeks to improve error reporting in this area.