Buggy ECDSA implementation

[jfinkhaeuser]

This vaguely relates to #49.

There's an issue with the ECDSA implementation, that took me some time to debug. Unfortunately, due to the nature of my contract, I can't submit a fix. But I can provide an explanation.

The TL;DR is, that for SHA-256, you need a key from the P-256 curve, for SHA-384 from the P-384 curve, and for SHA-512 from the P-521 curve. The current implementation allows using SHA-512 with a P-256 curve key, which leads to signatures that may not be verifiable by other implementations.

Now... there are concerns with using NIST's curves. But you can get EC groups with the same output lengths from different groups, so what I'd base this on is the EC_GROUP_order_bits() function - that has to be larger or equal to the SHA output bits (or you match them exactly as I described above, as you wish).

When you use matching sizes, you'll see that the zero-padding you do in the sign() method doesn't produce endless AAAAAA... prefixes, but will typically pad no more than a byte.

For reading up, I'll refer you to RFC7519 Section 8, which refers to encryption algorithms, particularly the ECDSA usage, from RFC7518 Section 3.4, which in turn lists the permissible curve + SHA combinations. It also lists that the curves to be used are from NIST's FIPS PUB 186-4 aka the Digital Signature Standard.

Hope that helps!

[Thalhammer]

Sorry for the late reply.
If I understand correctly, the "bug" is that the library does not protect you against using the wrong input key ?
Or is there also an issue if you use it correctly ?

[jfinkhaeuser]

The bug is in the invalid padding. The trigger are the mismatching keys.

[Thalhammer]

Yeah but the bug regarding invalid padding was fixed in the other issue. So this is only about it not complaining about the user doing something wrong?

[jfinkhaeuser]

The code or the documentation.

I'm not trying to tell you what you should do, mind you. But I also have a quality management background, and my preference is that invalid inputs should not look like they succeed. Currently that's the case.

Personal preference would be to error out and document how to avoid it.

[Thalhammer]

@jfinkhaeuser Of course it should be fixed to error out. But since this is not a hard bug I will fix it once I come around to do so and consider it an improvement.

[Thalhammer]

@jfinkhaeuser
I believe it should no behave correctly. You may check over it though.

[Thalhammer]

Behaviour should be fixed by fae2875.