fix off-by-one in update_client_pin_if_verified()#559
Open
DanielCohenHillel wants to merge 1 commit intosolokeys:masterfrom
Open
fix off-by-one in update_client_pin_if_verified()#559DanielCohenHillel wants to merge 1 commit intosolokeys:masterfrom
DanielCohenHillel wants to merge 1 commit intosolokeys:masterfrom
Conversation
The code that validated new PIN length checks if new PIN is between 4 and 63 bytes long, instead of between 4 and 64 as it should. This could allow an attacker to decrypt the 64-th byte of an encrypted message without knowing the shared secret key, assuming he can bypass the HMAC memcmp check (which is not too likely). Reasoning: The authenticator decrypts the given PIN, and a user/attacker can tell if the given decrypted PIN length is correct or not from the returned error, this is a padding oracle. If the check were if the new pin is between was between 4 and 64 this wouln't be a problem, since 64 is a multiple of the block size (16), and since the message is padded to block multiples, all 64-byte long new PINs would be accepted. The next possible length is 80 bytes, and the only way an 80-bytes new PIN message would be accepted is if the last 16 bytes are zero, which is impossibly unlikely (2 ^ -16 random chance), so the padding oracle won't give an attacker anything. Since the actual check is if the PIN is between 4 and 63, if you send a 64 byte new PIN message, it would be accepted only if the last byte was decrypted to zero, and fail otherwise. Since the authenticator uses CBC mode of operation, it is possible the brute- force the last byte of the previous block (i.e. the 48-th byte) until the last byte is zero, then you know the last byte is zero, so the decrypted last byte is the encrypted 48-th byte.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The code that validated new PIN length checks if the new PIN is between 4 and 63
bytes long, instead of between 4 and 64 as it should. This could allow an
attacker to decrypt the 64-th byte of an encrypted message without knowing
the shared secret key, assuming he can bypass the HMAC memcmp check (which is
not too likely).
Reasoning:
The authenticator decrypts the given PIN, and a user/attacker can tell if the
given decrypted PIN length is correct or not from the returned error, this is
a padding oracle.
If the check were if the new pin is between was between 4 and 64 this wouln't
be a problem, since 64 is a multiple of the block size (16), and since the
message is padded to block multiples, all 64-byte long new PINs would be
accepted. The next possible length is 80 bytes, and the only way an 80-bytes
new PIN message would be accepted is if the last 16 bytes are zero, which is
impossibly unlikely (2 ^ -16 random chance), so the padding oracle won't give
an attacker anything.
Since the actual check is if the PIN is between 4 and 63, if you send a 64
byte new PIN message, it would be accepted only if the last byte was
decrypted to zero, and fail otherwise.
Since the authenticator uses CBC mode of operation, it is possible the brute-
force the last byte of the previous block (i.e. the 48-th byte) until the
last byte is zero, then you know the last byte is zero, so the decrypted last
byte is the encrypted 48-th byte.