Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When processing secpass login, failed logins are not recorded #3226

Closed
jnorell opened this issue Jan 30, 2020 · 18 comments
Closed

When processing secpass login, failed logins are not recorded #3226

jnorell opened this issue Jan 30, 2020 · 18 comments
Labels
bug Undesired behaviour logging Logging issue resolved A fixed issue
Milestone

Comments

@jnorell
Copy link

jnorell commented Jan 30, 2020

I would like to have fail2ban monitor login failures and block clients after a threshold.

Currently I find only successful logins are logged, not failed logins, so the first part of the rfe is to log all login failures. (Note if you have Account Locking enabled, then login failures for valid accounts are logged - not helpful for catching most brute force attempts.) If setting debug level logging you do see the username of invalid login attempts as well, but this doesn't work for fail2ban, as it does not log the actual failure, the debug level log is the attempt whether successful or not. (And it seems login failures should always be logged, not as a 'debugging' level message.)

The other part needed is to include the ip address of the client in the log message, so fail2ban can match and block the client.

@cigamit cigamit added enhancement General tag for an enhancement logging Logging issue bug Undesired behaviour and removed enhancement General tag for an enhancement labels Jan 30, 2020
@cigamit cigamit changed the title better logging of login failures When processing secpass login, failed logins are not recorded Feb 2, 2020
@cigamit cigamit added the resolved A fixed issue label Feb 2, 2020
@cigamit cigamit added this to the v1.2.9 milestone Feb 2, 2020
@cigamit
Copy link
Member

cigamit commented Feb 2, 2020

Audited the login code and found that secpass did not log login subsequent attempts after the account was locked out.

@cigamit
Copy link
Member

cigamit commented Feb 2, 2020

This is resolved now.

@cigamit cigamit closed this as completed Feb 2, 2020
@jnorell
Copy link
Author

jnorell commented Feb 3, 2020

I'm glad you found and fixed a related issue in the logging, but this was intended to be a request for enhancement, not a bug report, and it certainly does not address the issue I was requesting. Please re-read the request and consider reopening.

@netniV
Copy link
Member

netniV commented Feb 3, 2020

Rather that suggesting to simply re-read, it may be more efficient to point to the part that you actually want implementing that seems to have been missed. I'm reading between the lines and I think you are saying that if there is an attempt to login with an account that does not exist, that is not logged?

@netniV
Copy link
Member

netniV commented Feb 3, 2020

Also, if you actually used the github templates that are provided for creating new issues, you would have filled in the enhancement template, which would have made it obvious it was an enhancement request, not just a bug report which the majority of posts are.

@cigamit
Copy link
Member

cigamit commented Feb 3, 2020

I believe he is wanting the IP address in the log also. We would have to attempt to determine exactly what the IP is though, in case the Cacti server is sitting behind a proxy or Cloudflare.

@cigamit cigamit reopened this Feb 3, 2020
@jnorell
Copy link
Author

jnorell commented Feb 3, 2020

@netniV: no problem, hopefully I'll catch the clarification you requested by using the mentioned template:

Is your feature request related to a problem? Please describe.
I would like to have fail2ban monitor login failures and block clients after a threshold.

Describe the solution you'd like
I would like cacti to log all login attempts, whether successful or not, and include the IP address of the client. (As @cigamit noted, there are several cases to consider, direct clients as well as cdn's/proxies which provide the ip in several common ways.)

Describe alternatives you've considered
Not sure there is one directly, I suppose integrating with the web server which is privy to the actual requests/responses (eg. mod_security) could achieve this. It would be nicer to have cacti do the logging directly, as more people could (would?) take advantage of it, rather than the small subset who both use apache and would take the time to configure mod_security for this.

Additional context
Currently login attempts are only logged at DEBUG log level, it seems like that should be done at a much lower level (the equivalent of syslog NOTICE level; it probably would fall in POLLER_VERBOSITY_LOW as it seems more significant than logging normal 'Results').

Thanks!

@TheWitness
Copy link
Member

I'd rather classify this as a bug since without the logging, you won't know someone is trying to hack in. We should just log the fake account that is being used to get as well.

@cigamit
Copy link
Member

cigamit commented Feb 4, 2020

Making another change shortly.

cigamit added a commit that referenced this issue Feb 4, 2020
* When processing secpass login, failed logins are not recorded
* This change adds Cacti log function calls
* Some code cleanup and improve internal logging
@cigamit
Copy link
Member

cigamit commented Feb 4, 2020

@jnorell, I misread your logging request as Cacti's internal user_log table. On second read, I see you are scraping the Cacti log. That is done now. If you are happy close this.

@jnorell
Copy link
Author

jnorell commented Feb 4, 2020

Thanks for the quick action, @cigamit. Looks like the username would indeed be logged, any chance the client IP(s) info could be included as well?

Items to include would be at minimum $_SERVER['REMOTE_ADDR'] , and maybe a configurable HTTP header (common ones are X-Client-IP, X-Forwarded-For, CF-Connecting-IP or True-Client-IP)? I know the last bit is more work/configuration, and I could live without it personally (I'll just use mod_remoteip where needed), but just knowing a username being guessed at isn't actionable (to a firewall level) without the ip address.

Thanks!

cigamit added a commit that referenced this issue Feb 4, 2020
* When processing secpass login, failed logins are not recorded
* Adding Client Address
@cigamit
Copy link
Member

cigamit commented Feb 4, 2020

Done.

@cigamit
Copy link
Member

cigamit commented Feb 4, 2020

We have a function that goes through NAT addresses first, and then goes into the more generic addresses. Please review it for completeness.

@cigamit
Copy link
Member

cigamit commented Feb 4, 2020

Here is the function in it's current state. A pull request would be good.

function get_client_addr($client_addr = false) {
	if (isset($_SERVER['X-Forwarded-For'])) {
		$client_addr = $_SERVER['X-Forwarded-For'];
	} elseif (isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {
		$client_addr = $_SERVER['HTTP_X_FORWARDED_FOR'];
	} elseif (isset($_SERVER['HTTP_FORWARDED_FOR'])) {
		$client_addr = $_SERVER['HTTP_FORWARDED_FOR'];
	} elseif (isset($_SERVER['HTTP_FORWARDED'])) {
		$client_addr = $_SERVER['HTTP_FORWARDED'];
	} elseif (isset($_SERVER['REMOTE_ADDR'])) {
		$client_addr = $_SERVER['REMOTE_ADDR'];
	} elseif (isset($_SERVER['HTTP_CLIENT_IP'])) {
		$client_addr = $_SERVER['HTTP_CLIENT_IP'];
	}

	return $client_addr;
}

@cigamit
Copy link
Member

cigamit commented Feb 4, 2020

Looks like we are missing a few. Seems like a minor change.

cigamit added a commit that referenced this issue Feb 4, 2020
Adding a few additional header matches
@cigamit
Copy link
Member

cigamit commented Feb 4, 2020

Double check my math. I just added what was new.

cigamit added a commit that referenced this issue Feb 4, 2020
Adding a few more possible headers.
@jnorell
Copy link
Author

jnorell commented Feb 4, 2020

3 comments on the latest get_client_addr()

All http headers which show up in $_SERVER should have names starting with HTTP_, so eg. $_SERVER['HTTP_X_FORWARDED_FOR'] rather than just $_SERVER['X-Forwarded-For'].

All values in $_SERVER['HTTP_*'] are untrusted input from the http client, and must be validated and/or sanitized prior to use to avoid security issues. Eg. if an X-Forwarded-For: surprise! header is present, it appears that is what will be logged and inserted directly into the database (using a prepared statement, no sql injection, but I didn't check for how the db entry might be subsequently used).

There needs to be a way to specify what source(s) of addresses should be used. The $_SERVER['HTTP_*'] variables are set by http headers and are untrusted inputs (sent by the client) unless known to be otherwise, but there is no way to automatically determine what the correct address is given multiple options, it must be configured. A sane default might be a best guess like the current function, with some clear note in the documentation that if you need/expect the IP to be unforgeable it must be configured. Or you might choose to default to always use $_SERVER['REMOTE_ADDR'] as it is safe (unforgeable), though often inaccurate.

Eg. in the current form, on a site which has clients directly access it a client can send a X-Forwarded-For header and that is the ip address which will be logged (and hence blocked if using fail2ban for that), not the correct one in $_SERVER['REMOTE_ADDR'].

Thanks,
Jesse

jnorell added a commit to jnorell/cacti that referenced this issue Feb 4, 2020
This change is 100% untested, but demonstrates the discussed changes at Cacti#3226 (comment) except for the (very important) issue of being able to configure what the correct source of remote client ip is in the local server environment.
@cigamit
Copy link
Member

cigamit commented Feb 6, 2020

Added some comments in the commit. So, in summary:

  1. Remote ADDR should be the default if no others found
  2. Anything in the FORWARD category can be a comma delimited list of IP's, so we should do a strpos() on the comma, explode, valid, implode if good.
  3. No need to add some nebulous, and presently undefined function (the pseudo-code you commented out)
  4. Otherwise, it's perfect.

@netniV netniV closed this as completed Feb 10, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Undesired behaviour logging Logging issue resolved A fixed issue
Projects
None yet
Development

No branches or pull requests

4 participants