ecdsa: prehash must receive zero-pads on left

[sorah]

This is a fix for the issue I mentioned at #534 (comment). I believe the present implementation of prehash_to_field_bytes can't interop with OpenSSL (at least but should be the same for other crypto libraries)...

Description

prehash_to_field_bytes was zero-padding on the right of the byte sequence but this must be done on the left because the output is evaluated as a integer encoded in big-endian, and its integer representation should be stable regardless of sequence length.

This behavior is defined on various documents including RFC6979 Section 2.3.2., SEC 1 Section 2.3.8., NIST FIPS 186-4 Appendix C.2.1.

• https://datatracker.ietf.org/doc/html/rfc6979#section-2.3.2
• https://www.secg.org/sec1-v2.pdf
• https://nvlpubs.nist.gov/nistpubs/fips/nist.fips.186-4.pdf

Verification

I used the following code to test signing with ecdsa crate and verify with OpenSSL:

use digest::{Digest, FixedOutput};
use ecdsa::signature::hazmat::PrehashSigner;
use elliptic_curve::sec1::ToEncodedPoint;
use openssl::nid::Nid;

const DATA: &str = "data to sign";

fn main() {
    let pkey = p384::SecretKey::random(&mut rand::thread_rng());
    let signing_key = ecdsa::SigningKey::from(&pkey);

    let digest = sha2::Sha256::digest(DATA);
    let signature = signing_key.sign_prehash(&digest).unwrap();

    //---------//

    let point_sec1 = pkey.public_key().to_encoded_point(false).to_bytes();
    let signature_der = signature.to_der().to_bytes();

    //---------//

    let mut ossl_bn_ctx = openssl::bn::BigNumContext::new().unwrap();
    let ossl_curve = openssl::ec::EcGroup::from_curve_name(Nid::SECP384R1).unwrap();
    let ossl_point =
        openssl::ec::EcPoint::from_bytes(&ossl_curve, &point_sec1, &mut ossl_bn_ctx).unwrap();
    let ossl_public_key = openssl::ec::EcKey::from_public_key(&ossl_curve, &ossl_point).unwrap();

    let ossl_signature = openssl::ecdsa::EcdsaSig::from_der(&signature_der).unwrap();
    assert!(ossl_signature.verify(&digest, &ossl_public_key).unwrap());
}

this patch fixes the crate not to fail the above code.

[aumetra]

LGTM

[aumetra]

Maybe we should add the test parameters from RFC 6979 (in a separate PR)? At least for one of the curves. That would have caught this problem (I think?)

Never mind, misread the source. But having a few tests against these test vectors and some parameters, signatures, etc. from other implementations would be beneficial regardless

[sorah]

I was thinking to add a test to catch this but curious how to test this problem appropriately without help of other libs. A simple roundtrip test doesn’t help for this, so as the rfc makes the process determinable embedding the expected output vector and compare?

[aumetra]

So this issue you fixed should occur whenever you use the _prehash methods with a digest that is smaller than the field size (ie. SHA-1 with P256, SHA-256 with P384, etc.) since the padding has to be applied when signing and verifying.

So a possible test would be to, for example, import p384 as a development dependency and have a test that tries to verify a signature generated by OpenSSL via the verify_prehash function that uses SHA-256 as a digest (you could put the private/signing key and the signature in files inside the test/ directory).
This test should fail with the version currently published on crates.io but work with your changes.

RFC 6979 test vectors wouldn't detect this unfortunately since they expect the digest that was used to hash the message and the digest that is used to derive k to be the same, which isn't the case here.
In your MRE, the message is hashed with SHA-256 but p384 internally uses SHA-384 to derive factor k (I forgot about this when I originally made my comment; see here which references the associated type defined here).

I'm not sure whether this is even the place for a test like this since the RFC 6979 tests aren't here either, instead they are implemented alongside the concrete curve implementations (here).

CC @tarcieri

[tarcieri]

Nice catch, thanks!

Regarding testing: I'd strongly recommend against circular dependencies, particularly involving pre-1.0 crates. We already have some in this repo for the ed25519 crate and it's a giant pain from a maintainability perspective.

I'd suggest adding some unit tests in the ecdsa crate itself and integration tests downstream in the p256/p384 crates. There are already some RFC6979 tests here, for example:

https://github.com/RustCrypto/elliptic-curves/blob/f3ae7d5/p256/src/ecdsa.rs#L91-L113

[sorah]

Indeed, having circular dependencies would be painful; Do you think that this PR needs a unit test? I think I can work on p256/p384 integration test...

But writing a unit test would take a time for me because currently ecdsa crate has no unit test & I'm not confident enough to prepare curve structs and others for unit testing from scratch (with aligning to your intention and design decision), so I want to leave this for you.

[tarcieri]

Integration tests would be a lot more valuable for assessing correctness.

I can add a unit/regression test separately if you'd rather work on integration tests instead.

[sorah]

ok, I'll try working on integration test in a separate PR (as p256/p384 crate reside in another repo)

[sorah]

Submitted RustCrypto/elliptic-curves#666

[tarcieri]

Thank you!

[tarcieri]

Released as ecdsa v0.14.8