Re: (ITS#9055) contrib/slapd-modules/passwd/totp improvements

[gv@members.scinet.supercomputing.org]

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.

> 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;

-- 
Greg Veldman