pkcs11_ecdsa_sign_sig() does not truncate long digests - fix proposed

[DDvO]

pkcs11_ecdsa_sign_sig() works for digests with sizes up to the key length.
Beyond that, the digest needs to be truncated, as done by OpenSSL in ecdsa_do_sign().
I took over (most of) that truncation code as attached and successfully tested it signing 64 bytes of data with CKM_ECDSA and secp256r1 used for authenticating a TLS endpoint.

p11_ec_ecdsa_sign_truncate.patch.txt

[mtrojnar]

Please use pull requests to submit changes.

[mtrojnar]

You may not "take over" some code from OpenSSL and submit it into a project with incompatible license.

[dengert]

AS a further note pkcs#11-v2.20 12.3.1 EC Signatures says:

"If the length of the hash value is larger than the bit length of n, only the leftmost bits of the hash up to the length of n will be used."

So it is the responsibility of the underlaying PKCS#11 module (or even the smart card) to do the truncation.

What PKCS#11 module are you using? Is it using a hardware device?

[DDvO]

I'm using a CardOS 5.0 card and 5.3.011 middleware. I'm not sure if the hardware is in a beta version, but at least the middleware should be an official release.

@doug, good point that the PKCS#11 middleware or even the device itself should be able to deal with long digests. On the other hand, at least the versions I'm using do not, and there may be also other PKCS#11 modules/devices having difficulties with this. Moreover, too long digests will be eventually need to be truncated anyway, so why not truncate them as soon as possible, which is certainly more efficient than sending possibly lots of needless bytes to the device?

@michal, here is an update of my fix that is independent of the OpenSSL code and much shorter:

    /* Truncate digest if its byte size is longer than needed */
        const EC_GROUP *group = EC_KEY_get0_group(ec);
    BIGNUM *order = BN_new();
    if (group && order && EC_GROUP_get_order(group, order, NULL)) {
        int klen = BN_num_bits(order);
        if (klen < 8*dlen)
            dlen = (klen+7)/8;
    }
    if (order)
        BN_free(order);

This truncation can be seen both as a workaround for certain modules/devices and a general performance improvement. How if I provide a pull request for this?

[dengert]

There is an other issue. PKCS#11 specs say if the HASDH is to long, truncate. But it has no way to tell if the data being passed
is a hash or raw data presented by the user. Truncating non-hashed data may lead to duplicate signatures if the user is not careful
about what they are doing.

Would a better approach be to require the caller to use a hash (or length of data) that is appropriate for the size of the key?

As a side note, I tried to send to an Oberthur PIV card, data that would need to be truncated. The card returns an error.
So the card or OpenSC PKCS#11 did not truncate either.

Have you contacted the CardOS middleware people on the issue?

I have not looked close enough at the libp11 code to see if using EC_KEY_get0_group returns correct info when the key is
used via an engine and the key is on the card. I think it does, but this needs to be checked.)

On 4/19/2016 7:34 AM, David von Oheimb wrote:

I'm using a CardOS 5.0 card and 5.3.011 middleware. I'm not sure if the hardware is in a beta version, but at least the middleware should be an official release.

@doug https://github.com/Doug, good point that the PKCS#11 middleware or even the device itself should be able to deal with long digests. On the other hand, at least the versions I'm using do not,
and there may be also other PKCS#11 modules/devices having difficulties with this. Moreover, too long digests will be eventually need to be truncated anyway, so why not truncate them as soon as
possible, which is certainly more efficient than sending possibly lots of needless bytes to the device?

@michal https://github.com/Michal, here is an update of my fix that is independent of the OpenSSL code and much shorter:

|/* Truncate digest if its byte size is longer than needed _/ const EC_GROUP *group = EC_KEY_get0_group(ec); BIGNUM *order = BN_new(); if (group && order && EC_GROUP_get_order(group, order, NULL)) {
int klen = BN_num_bits(order); if (klen < 8_dlen) dlen = (klen+7)/8; } if (order) BN_free(order); |

This truncation can be seen both as a workaround for certain modules/devices and a general performance improvement. How if I provide a pull request for this?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub #78 (comment)

Douglas E. Engert DEEngert@gmail.com

[DDvO]

I'm confident that by 'hash' the spec actually means any data to be signed. Otherwise, it should have better made the case of non-hash data explicit. For nearly all cases, the data to be signed will be the result of some some hash function. Also for the OpenSSL equivalent of this signing operation,ecdsa_do_sign(), the parameter holding the data to be signed is called 'digest', which suggests/assumes the same.
Moreover, only users how know what they're doing should be calling the plain signature function without hashing before.
Finally, since already libp11 is unable to tell the difference, how can one expect that the module/device should be able to?

Unfortunately, one cannot always assume that a hash of appropriate length has been used. As I wrote initially, I successfully tested this signature with for the use case of authenticating a TLS endpoint. This was done with OpenSSL using the engine_pkcs11 interface where the private key is (only) on the card. (This also answers your last mentioned concern), OpenSSL for some reason provided a 64-byte value for signing, while the key was of secp256r1 form (thus, 32 bytes in length).

Interesting that also the Oberthur PIV card does not handle overlong (hash) data.

Yes, I contacted the CardOS team on the issue (this afternoon); I will let you know on their response.

[mtrojnar]

I don't think truncating the input in lipb11 would have a negative security impact, as the PKCS#11 module is supposed to do it anyway. As @dengert said, "it has no way to tell if the data being passed is a hash or raw data presented by the user". This statement applies both to the PKCS#11 module and to libp11. The user should expect the data to be truncated in the PKCS#11 module, so there is no harm in also doing it in libp11. Am I right?

[mouse07410]

I don't think truncating the input in lipb11 would have a negative security impact...

Since there doesn't seem to be a standard-specified way for the extra bits to be factored into the signature - I'd say it doesn't matter at what point of the processing chain you truncate the input.

Am I right?

I think so.

P.S. I'll be happy to test the change with my working scripts that exercise OpenSC and OpenSSL.

[mouse07410]

Ran my scripts, everything seems fine.

[DDvO]

@mtrojnar, thank you for swiftly taking over also this request for a workaround/speedup w.r.t. digest size.

Now, one can already take advantage of the EC features of CardOS 5.0 cards via PKCS#11 even in case there are delays correcting/updating the official CardOS middleware and/or the alternative OpenSC driver.
As discussed, the workarounds of #78 and #79 may be of help also to other card types.