Skip to content
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

Add EdDSA/Minisign signatures to #397 (sign-hash/sign-file CTAP command) #583

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

stevenwdv
Copy link

@stevenwdv stevenwdv commented Oct 20, 2021

This adds credential algorithm detection to #397 to also support EdDSA signatures. It also adds a trusted comment field to the request with key 3. If provided with an EdDSA credential, a global signature on the main signature + trusted comment is included in the response with key 2. The command now accepts 64-byte (512-bit) hashes in addition to 32-byte hashes. See #575 for more about Minisign and an earlier version using a FIDO2 extension instead of a custom CTAP command.

Potentially-breaking change from #397: EdDSA credentials used with sign-hash are not incorrectly treated as ES256 anymore. CTAP structures are backwards-compatible.
Update: now only accepts credentials with RP ID starting with solo-sign-hash:.

See solokeys/solo1-cli#137 for the client PR.

Radoslav Gerganov and others added 3 commits October 18, 2021 16:24
This patch adds new CTAP2 vendor command with command value 0x50. The
command arguments are credentialId and user specified SHA256 hash. It
returns a DER encoded signature of the given hash, using the key
which corresponds to the specified credentialId.

Example request:
{1: <sha256_hash>,
 2: {"id": <credential_id>, "type": "public-key"},
 3: [pinAuth]}

Example response:
{1: <der_signature>}

Issue: solokeys#395
…ith support for arbitrary-length hashes up to 64B and a trusted comment in the EdDSA case.

Also fixed existing bug: get_credential_id_size(SH.cred.type) should be get_credential_id_size(&SH.cred)
Also now cose_alg is checked. I'm not sure yet if it is safe to use arbitrary-length hashes with verify_pin_auth_ex. Maybe a min length should be set?
memcpy(data, data1, len1);
memcpy(data + len1, data2, len2);
memcpy(data, data1, len1);
if (len2)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to check, memcpy with zero length is fine

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally yes, but I call it with data2 == NULL, which would be UB even with len2 == 0. I could call it passing data1 twice, but I think this is cleaner.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the check should be if (data2 != NULL)

Copy link
Author

@stevenwdv stevenwdv Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's also possible, but personally I think it looks weird because this would make the following a legal call:

crypto_ed25519_sign(some_data, some_len, NULL, 100, buf)

Although to be fair, with the current conditional the following is legal:

crypto_ed25519_sign(some_data, some_len, (uint8_t*)123, 0, buf)

We could have if (data2 || len2) (or maybe if (data && len2) if we want to be lenient), but in any case it will only matter with invalid parameters.
Or maybe I'm missing an advantage of if (data2)?

fido2/ctap.c Show resolved Hide resolved
fido2/ctap.c Outdated Show resolved Hide resolved
@stevenwdv stevenwdv marked this pull request as ready for review November 2, 2021 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants