On Fri, Jul 19, 2019 at 04:34:44PM +0000, gv@members.scinet.supercomputing.org wrote:

On Fri, Jul 19, 2019 at 05:58:37PM +0200, Ond??ej Kuzn??k wrote:

...

BTW, why did you choose to check that the user has already logged in rather than "(told == 0 || t - 1 > told)"?

There are two circumstances where a user may transmit a code from the previous time window:

1. The time window rolls over as the code is being typed in or transmitted over the network.

2. The clock on the user's prover is off.

I made this condition because on the very first attempt at TOTP auth, I wanted to be more strict about accepting expired codes to ensure that the clock on the user's prover is in fact in sync with the server. Since this implementation has no mechanism to adjust for clock skew, if both clocks weren't in sync that would cause many issues for the user down the road, and the possibility of a false negative on the first attempt seemed a small price to pay to ensure they were.

OK, probably worth mentioning then.

...

It might be better to check both the token and password independently and then return success iff both of them succeeded to avoid timing leaks of OTP success/failure?

I do. The issue Quanah raised is in relation to the change in chk_totp(). This is only called directly for the {TOTP1}, {TOTP256}, and {TOTP512} schemes (e.g. TOTP only and no password). If a password is in play, the checks use the new chk_totp_and_pw() function, which does validate each piece independently:

    if (chk_totp(&passwd_otp, &cred_otp, mech, text) == LUTIL_PASSWD_OK
                    && lutil_passwd(&passwd_pass, &cred_pass, NULL, text)
                    == LUTIL_PASSWD_OK)
            rc = LUTIL_PASSWD_OK;

This only checks the password if OTP check passed, right? So if checking the password takes a measurable amount of time, an attacker can see if they hit the right OTP token without it being voided.

Regards,