Log a message for failed logins

[matt335672]

Fixes #1724 and #1946

Apologies for dropping this one. I was hoping to pick it up as a rework of the SCP code, but this is a much larger job than I first realised.

The following message is now logged as in sesman.log for unrecognised users or invalid passwords:-

Username or password error for user: <user>

This is very much a quick fix. If preferred. I can add more detailed logging to the PAM code, or I could pull the existing PAM detail from xrdp/xrdp_mm.c into some sort of library module and use it to generate extra info in sesman.

Thoughts?

[metalefty]

Maybe related to #909.

[matt335672]

Also #1921 on the discussions page.

I think the approach should be:-

• provide detailed information in sesman log
• more information to users via the xrdp login dialog, but not enough to compromise security.

We've currently got some PAM code in xrdp_mm.c which is used for logging in gateway situations (i.e. using pamusername= and pampassword= in xrdp.ini). That code could be moved to sesman, so we can get more detailed info in the sesman log. sesman can then return a simpler code to xrdp (and the user) for display purposes.

Does that sound reasonable?

[metalefty]

Yes, it looks reasonable to me. Thanks!

Also, the PR is definitely simple, and there's no reason to object.

[MikoyChinese]

Why not append the client ip to log? s->client_ip, It will be useful to ban ip from attack.

    {
        LOG(LOG_LEVEL_INFO, "Username or password error for user: %s from %s",
            s->username, s->client_ip);
        scp_v0s_deny_connection(c);
    }

[matt335672]

I did think about this, and was very much in two minds.

On the positive side, it's useful information for log scraping utilities.
On the negative side, this string may well change in the future, so I didn't want to introduce user dependencies on it.

The string is created by a call to g_write_ip_address() from the xrdp daemon and then sent to sesman. I'm hoping to rework that part of the protocol between xrdp and sesman to just send the actual connection information which will then get formatted by sesman. At the moment sesman has to jump through some rather ugly hoops to parse the data from xrdp.

What's your take on it? I can see you being rather annoyed if you're provided information which could change between releases. On the other hand you may prefer that to nothing at all. If you've got a strong preference to have the info I'll happily add it in.

[timriker]

On servers with public IPs, it would be a very good idea to setup things like fail2ban that recognize failed logins and then ban the source IP for a reasonable time. This would need a message like the above in #1947 (comment)
that includes the failure state, and the IP. It would be nice to have the user they tried as well to correlate with pam logs, but for fail2ban the date, failure state, and IP are the critical pieces.
Pick a format and stick to it. With the rework, the data can come through different paths, or even be output from different code, but keep the format the same. Something like:
"Login failure for %s from %s"
would be great.

[matt335672]

@timriker - could you take a look at #1976 for me, and comment?

Thanks.

[timriker]

@timriker - could you take a look at #1976 for me, and comment?

@matt335672 - commented there. That's looking great.