google / google-authenticator-libpam

Apache License 2.0
1.75k stars 280 forks source link

Emergency codes not working if first digit is zero #213

Closed ruedigerkupper closed 2 years ago

ruedigerkupper commented 2 years ago

Upon creating the secret, a set of 8-digit emergency codes is generated that can be used (in top-down order) for logging in. These 8-digit emergency codes will not work if the first digit is zero.

There may be a general problem with leading zeros: It appears that even a 6-digit code will be rejected if it starts with zero – but it will be accepted on the second try (please confirm).

But 8-digit codes starting with zero will never be accepted.

ThomasHabets commented 2 years ago

For 8 digit scratch codes you are correct. This is documented in the code, too, and no scratch codes with a leading zero should ever be created.

Both at creation time and check time scratch codes have to be "8 digits" (with leading zeroes removed).

It appears that even a 6-digit code will be rejected if it starts with zero – but it will be accepted on the second try (please confirm).

I doubt this. There's nothing special about the first attempt, so if it failed the first time it should fail the second time. Do you have a reproducible case for this, and where you confrm that it does not happen when leading digit is not zero?

ruedigerkupper commented 2 years ago

For 8 digit scratch codes you are correct. This is documented in the code.

Ah, I see. Would be good having this in the man page!

Both at creation time and check time scratch codes have to be "8 digits" (with leading zeroes removed).

I think the misunderstanding here is in the word ”code“. A code to my undrstanding is an arbitrary string (here a string of digits) and there's no reason why any character should be excluded. If it is however implied that this string of digits must comprise a valid integer number (or will be temporarily stored like this), it is obvious that it can't start in 0.

The behavior is a little confusing, since available OTP generators happily will generate codes starting in 0. So the usual 6-digit codes may very well start in 0. Can you explain why 8-digit codes are handled differently from 6-digit codes by this library?

It appears that even a 6-digit code will be rejected if it starts with zero – but it will be accepted on the second try (please confirm).

I doubt this. There's nothing special about the first attempt, so if it failed the first time it should fail the second time. Do you have a reproducible case for this, and where you confrm that it does not happen when leading digit is not zero?

This may actually be caused by time differences between our server and the hardware tokens we use for generating OTP. If so the problem should be tje same with all 6-digit codes, I'll check that!

Thanks!

ThomasHabets commented 2 years ago

I think the misunderstanding here is in the word ”code“. A code to my undrstanding is an arbitrary string (here a string of digits) and there's no reason why any character should be excluded

I agree. But the only way you can encounter this is if you go in and hand-hack the state file. Doing that is not documented, so arguably doesn't need mention in the manpage either.

The behavior is a little confusing, since available OTP generators happily will generate codes starting in 0. So the usual 6-digit codes may very well start in 0.

This code is like a decade old. I think we would already know if 10% of codes just plain don't work. So non-emergency codes I'm going to assert are just fine.

Can you explain why 8-digit codes are handled differently from 6-digit codes by this library?

Dunno. That predates me. Laziness, probably.

The important thing is to unambiguously and reliably only check the correct path. I guess removing 8-digit codes starting with 0 helps make it impossible to get it wrong. Belts and suspenders. No matter what the user does they can't exercise the wrong code path.

So that's another reason.

I don't really see a problem with this program not accepting codes it would never have generated.

ruedigerkupper commented 2 years ago

I think the misunderstanding here is in the word ”code“. A code to my undrstanding is an arbitrary string (here a string of digits) and there's no reason why any character should be excluded

I agree. But the only way you can encounter this is if you go in and hand-hack the state file. Doing that is not documented, so arguably doesn't need mention in the manpage either.

Arguably yes. But there is literally no other way of generating new backup codes than "hand-hacking the state file". We need our users to have valid baclup codes at any time. Unless there's an official way of doing this, we are simply out of other options.

So my problem essentially is a duplicate of #141 (open since 2019).

Can you explain why 8-digit codes are handled differently from 6-digit codes by this library?

Dunno. That predates me. Laziness, probably.

I don't really see a problem with this program not accepting codes it would never have generated.

Agreed. But I am sure we are not the only ones forced doing it unless #141 gets implemented.