frankmorgner / OpenSCToken

Use OpenSC in macOS CryptoTokenKit.
GNU General Public License v3.0
75 stars 14 forks source link

Add missing break #34

Closed ElMostafaIdrassi closed 3 years ago

ElMostafaIdrassi commented 3 years ago

For keys of type SC_PKCS15_TYPE_PRKEY_EC, there is no break, meaning the execution continues to the next labeled statement, here default, and return nil is always executed.

Signed-off-by: El Mostafa IDRASSI el-mostafa.idrassi@prestalab.net

frankmorgner commented 3 years ago

Thank you. Does this mean that you've actually tested some of the ECDSA algorithms via CTK?

ElMostafaIdrassi commented 3 years ago

Yes, and here are some interesting notes.

From the official Apple source code :

@constant kSecKeyAlgorithmECDSASignatureDigestX962SHA1
ECDSA algorithm, signature is in DER x9.62 encoding, input data is message digest created by SHA1 algorithm.

There are 2 main things to consider here :

1- When signData is called with kSecKeyAlgorithmECDSASignatureDigestX962SHA1, the input is expected to be an SHA-1 digest and the card is expected to only sign that digest. This meant that I had to modify the algorithmToFlags() function for that matter, from mapping kSecKeyAlgorithmECDSASignatureDigestX962SHA1 to SC_ALGORITHM_ECDSA_HASH_SHA1 to mapping it to SC_ALGORITHM_ECDSA_RAW | SC_ALGORITHM_ECDSA_HASH_NONE : this told the card not to perform hashing, and rather, sign the digest directly. I am not sure whether this mapping will work for other cards though.

2 - The resulting ECDSA signature is expected to be in DER. I will create a pull request for this one soon, since the current implementation only returns the raw (r,s) signature.

frankmorgner commented 3 years ago

Nice find! Your change sounds reasonable, would you mind creating an other PR with that fix?

ElMostafaIdrassi commented 3 years ago

Thanks! Yes, I've create the PR #35 which tackles encoding the signature into ASN.1. Concerning the mapping of the kSecKeyAlgorithmECDSASignatureDigestX962SHAX algorithms, I'm still not sure, although, IMO, mapping them all to SC_ALGORITHM_ECDSA_RAW | SC_ALGORITHM_ECDSA_HASH_NONE is the way to go.