yubikey-val icon indicating copy to clipboard operation
yubikey-val copied to clipboard

Invalid signature for some bad OTP requests, but not all

Open cyli opened this issue 11 years ago • 3 comments

I originally opened this issue in the yubico-dotnet-client repo.

I was testing invalid OTPs, and often got an exception saying that the server signature did not match the key.

As @klali helpfully pointed out, this is because, in the server, if the parameters provided are missing or malformed then the response is sent back immediately with a bad status, without having loaded the key first. Hence the whole response is signed with an empty key, and hence the signature is wrong.

https://code.google.com/p/yubikey-val-server-php/wiki/GettingStartedWritingClients suggests that clients check the length of the OTP, but not for ModHex characters in QWERTY or Dvorak format (and the reference clients all prevent the other checks from failing, such as a malformed nonce or missing parameters). But the server does specifically check for the ModHex characters in QWERTY and Dvorak, so there can be a mismatch if I enter in random characters that are part of neither alphabet, since the reference clients (and clients conforming to the suggestions on the wiki) won't catch it.

Would it make sense for the ModHex alphabet check to be done after the key is loaded, since all the other validation failures that can happen before the key is loaded are prevented by checks/implementation of the reference clients? This way all bad otp responses that the client receives will be signed correctly, and there won't be the unexpected bad signature failure (which would suggest some type of MITM or other more serious problem).

cyli avatar Mar 28 '13 08:03 cyli

This sounds like a good idea. A patch would be appreciated, then we can look at how it would actually be implemented. Thanks for the report.

jas4711 avatar Jun 25 '13 10:06 jas4711

Will do, thanks for the response!

Edit: caveat: will TRY, as I don't actually know PHP. :)

cyli avatar Jun 25 '13 16:06 cyli

We believe this should be implemented by not sending the 'h=' parameter if the server doesn't have a key available.

/Simon

jas4711 avatar Jun 26 '14 09:06 jas4711