ITS#10313 Tighten counter tracking modification

Try to make sure the counter/timer value we used hasn't been used up in
the meantime. Also if the update cannot be committed, do not say whether
the provided OTP was correct, this would open up an oracle for malicious
clients to brute force a token they could use later/elsewhere.

diff --git a/servers/slapd/overlays/otp.c b/servers/slapd/overlays/otp.c
index e2ffab03e8f543004385fb5273fe2e862d44a80d..772c9d2d982db02124901a46fe35861afbf2936f 100644 (file)
--- a/servers/slapd/overlays/otp.c
+++ b/servers/slapd/overlays/otp.c
@@ -649,7 +649,7 @@ otp_bind_response( Operation *op, SlapReply *rs )
 }
 
 static long
-otp_hotp( Operation *op, Entry *token )
+otp_hotp( Operation *op, Entry *token, BerValue *old_value )
 {
        char outbuf[MAX_DIGITS + 1];
        Entry *params = NULL;
@@ -669,6 +669,9 @@ otp_hotp( Operation *op, Entry *token )
                                a->a_vals[0].bv_val );
                goto done;
        }
+       if ( a ) {
+               ber_bvreplace( old_value, &a->a_vals[0] );
+       }
 
        a = attr_find( token->e_attrs, ad_oathHOTPParams );
        if ( !a ||
@@ -753,7 +756,7 @@ done:
 }
 
 static long
-otp_totp( Operation *op, Entry *token, long *drift )
+otp_totp( Operation *op, Entry *token, BerValue *old_value, long *drift )
 {
        char outbuf[MAX_DIGITS + 1];
        Entry *params = NULL;
@@ -773,6 +776,9 @@ otp_totp( Operation *op, Entry *token, long *drift )
                                a->a_vals[0].bv_val );
                goto done;
        }
+       if ( a ) {
+               ber_bvreplace( old_value, &a->a_vals[0] );
+       }
 
        a = attr_find( token->e_attrs, ad_oathTOTPParams );
        if ( !a ||
@@ -885,7 +891,7 @@ otp_check_and_update( Operation *op, BerValue *totpdn, BerValue *hotpdn,
                Entry *token )
 {
        AttributeDescription *ad = NULL, *drift_ad = NULL;
-       BerValue ndn;
+       BerValue ndn, old_value[2] = { BER_BVNULL, BER_BVNULL };
        long t = -1, drift = 0;
        int rc = LDAP_INVALID_CREDENTIALS;
 
@@ -895,7 +901,7 @@ otp_check_and_update( Operation *op, BerValue *totpdn, BerValue *hotpdn,
                        ndn = *totpdn;
                        ad = ad_oathTOTPLastTimeStep;
                        drift_ad = ad_oathTOTPTimeStepDrift;
-                       t = otp_totp( op, token, &drift );
+                       t = otp_totp( op, token, &old_value[0], &drift );
                        be_entry_release_r( op, token );
                        token = NULL;
                }
@@ -905,7 +911,7 @@ otp_check_and_update( Operation *op, BerValue *totpdn, BerValue *hotpdn,
                                        0, &token ) == LDAP_SUCCESS ) {
                        ndn = *hotpdn;
                        ad = ad_oathHOTPCounter;
-                       t = otp_hotp( op, token );
+                       t = otp_hotp( op, token, &old_value[0] );
                        be_entry_release_r( op, token );
                        token = NULL;
                }
@@ -917,7 +923,7 @@ otp_check_and_update( Operation *op, BerValue *totpdn, BerValue *hotpdn,
                char outbuf[32], drift_buf[32];
                Operation op2;
                Opheader oh;
-               Modifications mod[2], *m = mod;
+               Modifications mod[3], *m = mod;
                SlapReply rs2 = { REP_RESULT };
                slap_callback cb = { .sc_response = &slap_null_cb };
                BerValue bv[2], bv_drift[2];
@@ -926,15 +932,27 @@ otp_check_and_update( Operation *op, BerValue *totpdn, BerValue *hotpdn,
                bv[0].bv_len = snprintf( bv[0].bv_val, sizeof(outbuf), "%ld", t );
                BER_BVZERO( &bv[1] );
 
+               /* Limit races by removing old counter by value */
+               if ( !BER_BVISNULL( &old_value[0] ) ) {
+                       m->sml_numvals = 1;
+                       m->sml_values = old_value;
+                       m->sml_nvalues = NULL;
+                       m->sml_desc = ad;
+                       m->sml_op = LDAP_MOD_DELETE;
+                       m->sml_flags = SLAP_MOD_INTERNAL;
+                       m->sml_next = (m+1);
+                       m++;
+               }
+
                m->sml_numvals = 1;
                m->sml_values = bv;
                m->sml_nvalues = NULL;
                m->sml_desc = ad;
-               m->sml_op = LDAP_MOD_REPLACE;
+               m->sml_op = LDAP_MOD_ADD;
                m->sml_flags = SLAP_MOD_INTERNAL;
 
                if ( drift_ad ) {
-                       m->sml_next = &mod[1];
+                       m->sml_next = (m+1);
 
                        bv_drift[0].bv_val = drift_buf;
                        bv_drift[0].bv_len = snprintf(
@@ -971,13 +989,20 @@ otp_check_and_update( Operation *op, BerValue *totpdn, BerValue *hotpdn,
 
                rc = op2.o_bd->be_modify( &op2, &rs2 );
                if ( rs2.sr_err != LDAP_SUCCESS ) {
-                       rc = LDAP_OTHER;
+                       Debug( LDAP_DEBUG_ANY, "%s otp_check_and_update: "
+                               "failed to invalidate provided token rc=%d: %s\n",
+                               op->o_log_prefix, rc, rs2.sr_text ? rs2.sr_text : "" );
+                       rc = LDAP_INVALID_CREDENTIALS;
                }
                if ( m->sml_next ) {
                        slap_mods_free( m->sml_next, 1 );
                }
        }
 
+       if ( !BER_BVISNULL( &old_value[0] ) ) {
+               ch_free( old_value[0].bv_val );
+       }
+
        return rc;
 }