New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in Two Step Enrollment Key #1347

Closed
hemantprasad opened this Issue Dec 19, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@hemantprasad
Copy link

hemantprasad commented Dec 19, 2018

Upon Token Enrollment, once the user scans the QR Code, privacyIDEA Authenticator generates the key.
privaycyIDEA server is supposed to validate the entire key as a whole.

But there is bug, wherein if the key generated in Authenticator is 12 character long, privacyIDEA accepts any character in the end.

For example if authenticator generates the key as AAA-BBB-CCC-DDD, but if user enters the key in privacyIDEA window as AAA-BBB-CCC-DDE its accepted and OTP works.

@fredreichbier

This comment has been minimized.

Copy link
Member

fredreichbier commented Dec 19, 2018

Thank you for the bug report! I reproduced the behavior with the current master. Seems like our checksum algorithm is faulty somehow.

@fredreichbier fredreichbier added the bug label Dec 19, 2018

@fredreichbier fredreichbier added this to the 3.0 Code Cleanup milestone Dec 19, 2018

cornelinux added a commit that referenced this issue Dec 20, 2018

@cornelinux

This comment has been minimized.

Copy link
Member

cornelinux commented Dec 20, 2018

I added a test for the client component check and everything works fine. Obviously it is due to a strange character alignment that in this corner case the base32 encoding encodes more than the payload.

In the tests you can see, that the payload is the same. This "collision" only happens with the last character. (Actually I never liked base32 encoding).

This looks good to me.

(It is important to note, that if you want to show this on a trade fair - either use a different client component length or change a character in the middle ;-)

@cornelinux cornelinux referenced this issue Dec 20, 2018

Merged

Add check for client_component #1351

0 of 1 task complete
@fredreichbier

This comment has been minimized.

Copy link
Member

fredreichbier commented Jan 2, 2019

Interesting! I was curious, so looked into this some more. For the record, this effect happens because of the payload length of 12 bytes (= 8 bytes client component + 4 bytes checksum), i.e. 96 bits. During encoding, base32 maps groups of 5 bits to one character. If the payload length is not divisible by 5, the payload is filled with zeros. When decoding, every character is converted back to a group of 5 bits. For the very last group, only the most significant bit matters. So replacing A (= 00000) with B (= 00001) doesn't change the decoded payload, but replacing A (= 00000) with Q (= 10000) changes the payload -- but then the checksum doesn't match anymore :)

>>> decode_base32check("TIXQW4ydvn2aos4cj6ta")
'03ab74074b824fa6'
>>> decode_base32check("TIXQW4ydvn2aos4cj6tq")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "privacyidea/lib/utils.py", line 315, in decode_base32check
    raise ParameterError("Malformed base32check data: Incorrect checksum")
privacyidea.lib.error.ParameterError: ERR905: Malformed base32check data: Incorrect checksum

So this is really only a problem when showing 2step enrollment at trade fairs :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment