'attempt to multiply with overflow' at totp-rs-5.0.1/src/lib.rs:452:21

[carl-wallace]

check_current panics on skew values larger than 127 as shown in this snip:

use totp_rs::{Algorithm, TOTP};
fn main() {
    let works = TOTP::new(Algorithm::SHA256, 8, 127, 30, vec![0x01; 48]).unwrap();
    assert!(works.check_current(works.generate_current().unwrap().as_str()).unwrap());

    let does_not_work = TOTP::new(Algorithm::SHA256, 8, 128, 30, vec![0x01; 48]).unwrap();
    let value = does_not_work.generate_current().unwrap();

    // check_current causes thread 'main' panicked at 'attempt to multiply with overflow' at totp-rs-5.0.1/src/lib.rs:452:21
    assert!(does_not_work.check_current(value.as_str()).unwrap());
}

Cargo.toml featured these dependencies:

[dependencies]
totp-rs = "5.0.1"

[constantoine]

Hi!

Yes it does overflow, because I didn't really intend on allowing as much skew. Is this a requirement you currently are having issues with?

[carl-wallace]

The report was primarily to highlight a missing boundary check (or data type change where the panic occurs) so a panic could be avoided.

Re: using an i8 (or half of one) as a skew, it does complicate the use of tokens in some one-time usage scenarios due to the length of the step that is necessary to compensate. Larger steps increase the hang time one must wait for a fresh value, smaller skews shorten the period during which one must act before expiration. For example, if a TOTP value were requested by a user and released via email by an approval role at some time later it'd be nice to avoid the need for the user to act more or less immediately. The liboath library uses the same data type for skew (or window in their parlance) and step, giving more flexibility. This scenario can be addressed otherwise, of course.

[constantoine]

Gotcha. I fixed the overflow so now it can have up to 255 skew periods, but I feel like if one needs to go further than u8 periods, time-based otp might not be the best authentication period. Moreover, changing the skew type would require a new major version, which is not ideal imo

[carl-wallace]

Thanks.