Partial logins logged & audited as failures?

[Vincent Brillault]

Hi,

Here is a small patch that tries to fix the problem mentioned below. Looking a
bit into history, it seems that the custom logs part of `auth_log` (within
`ifdef`) was missed out when the `partial` parameter was added to `auth_log`
(15b05cfa17592da7470d7bd4b2de063188697471 in openssh-portable).

The patch is modifying a bit the existing `ifdef` logic to remove the
duplicated `if (authenticated == 0 && !authctxt->postponed`, which I
understand was flawed (missing the partial check). I'm not sure if it's
better, because it makes it a bit more complicated to follow all cases
(depending on the defined variables), but at the same time it simplifies the
if statements. Let me know what's best.

I'm not too sure why the `if (authenticated)` part for `WITH_AIXAUTHENTICATE`
is within `CUSTOM_FAILED_LOGIN`, as it's not logging a failed login, but I
didn't want to change this logic...

Please let me know if I'm missing something,
Vincent Brillault

On 04/05/2020 10:56, Vincent Brillault wrote:
> Hi,
> 
> Trying to understand why some spurious `There was 1 failed login attempt since
> the last successful logins`, that seems to appear on every single login, I
> think there is a bug in auth.c's auth_log with the handling of partial logins:
> https://github.com/openssh/openssh-portable/blob/c697e46c314aa94574af0d393d80f23e0ebc9748/auth.c#L355-L372
> 
> If I read this code correctly, when auth_log is called with authenticated=0
> and partial=1 without authctxt->postponed being set (which is normal on
> partial authentications) then:
> - if method is password, keyboard-interactive or challenge-response (not sure
> why the others are not considered?), record_failed_login is called
> - audit_event is called with an event from audit_classify_auth which always
> seems to return a failure events (or unknown).
> 
> So it seems that partial authentications are considered as failures :/
> 
> The simplest fix for me seems to be to return before L355 if partial or
> authctxt->postponed are set (maybe after checking that there isn't a logic
> flow and authenticated was set?).
> 
> Am I missing something?
> Thanks in advance,
> Vincent Brillault
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-auth_log-dont-log-partial-successes-as-failures.patch
Type: text/x-patch
Size: 2166 bytes
Desc: not available
URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20200524/fa893d1f/attachment.bin>