[PATCH] Early request for comments: U2F authentication

[Michael Stapelberg]

Thanks for pointing this out. Comments inline:

On Sun, Dec 14, 2014 at 11:42 PM, Klaus Keppler <kk at keppler-it.de> wrote:

> I’ve spent some time (together with Christian and Thomas) hacking on
>> U2F support in OpenSSH, and I’m happy to provide a first patch — it’s
>> not complete, but it should be good enough to get the discussion going
>> :). Please see the two attached files for the patch.
>>
>
> This is great - I'm looking forward to it! :)
>
> I've implemented U2F into another (C-based) application these days. While
> searching for some relevant OpenSSL-specific "help" I stumbled upon your
> OpenSSH patch.
> I think there's a small bug:
>
>  +       if ((err = EVP_VerifyInit(&mdctx, EVP_ecdsa())) != 1) {
>> +               ERR_error_string(ERR_get_error(), errorbuf);
>> +               fatal("EVP_VerifyInit() failed: %s (reason: %s)",
>> +                               errorbuf, ERR_reason_error_string(err));
>>
>
> You should use "EVP_sha256()" instead of "EVP_ecdsa()" here (we have a
> ECDSA signature on the SHA256 hash)
>

If I do that, EVP_VerifyFinal() will result in EVP_R_WRONG_PUBLIC_KEY_TYPE.
Looking at the OpenSSL source, I can see that in crypto/evp/m_sha1.c, the
sha* digests are defined with EVP_PKEY_RSA_method, which requires an RSA
publickey, but we have an ECDSA publickey. The only digest working with
ECDSA publickeys is crypto/evp/m_ecdsa.c AFAICT.


>
>  +       if ((err = EVP_VerifyFinal(&mdctx, walk, restlen, pkey)) == -1) {
>> +               ERR_error_string(ERR_get_error(), errorbuf);
>> +               error("Verifying the U2F registration signature failed:
>> %s (reason: %s)",
>> +                               errorbuf, ERR_reason_error_string(err));
>> +               goto out;
>> +       }
>>
>
> You test EVP_VerifyFinal() only against "-1". This catches OpenSSL library
> errors and such. But if the signature check itself fails, you get "0". So,
> the only valid result here should be "1".
>

You’re correct, good catch.


>
> When you change EVP_ecdsa() to EVP_sha256() above, EVP_VerifyFinal()
> should return "1" on valid data.
>

Unfortunately not. Could you share the code that you have please? Or is it
not yet working?