weidai11/cryptopp

ECDSA verification succeeds when it should fail

guidovranken opened this issue · 7 comments

guidovranken commented

I think the following should not pass ECDSA verification (invalid pubkey), but it does. Tested on Linux 64 bit, latest master branch checkout.

#include <eccrypto.h>
#include <ecp.h>
#include <oids.h>
int main(void)
{
    ::CryptoPP::ECDSA<::CryptoPP::ECP, ::CryptoPP::SHA256>::PublicKey publicKey;
    publicKey.Initialize(
            ::CryptoPP::ASN1::secp256k1(),
            CryptoPP::ECP::Point(
                CryptoPP::Integer("83326269377737301187045338455478996967104803243941757917076354219390730898031"),
                CryptoPP::Integer("108911706275326467973600132368983151825997206660859431906025905780521963107049")
                ));

    ::CryptoPP::ECDSA<::CryptoPP::ECP, ::CryptoPP::SHA256>::Verifier verifier(publicKey);

    const CryptoPP::Integer R("58459610944154385406267492095069703630366579530687393858060946682600281547621");
    const CryptoPP::Integer S("166425580247629610000042226331335729376739920034019158271667950393311552549306");

    uint8_t ct[] = {0x31, 0x32, 0x33, 0x34, 0x30, 0x30};
    const size_t siglen = verifier.SignatureLength();
    uint8_t signature[siglen];

    R.Encode(signature + 0, siglen / 2);
    S.Encode(signature + (siglen / 2), siglen / 2);
    printf("%d\n", verifier.VerifyMessage(ct, sizeof(ct), signature, siglen));
    return 0;
}
noloader commented

Thanks @guidovranken,

I think the following should not pass ECDSA verification (invalid pubkey)

What is wrong with the key?

When I add a call to Validate after the public key is loaded, it passes validation.

#include "eccrypto.h"
#include "ecp.h"
#include "oids.h"
#include "osrng.h"

#include <iostream>

int main(void)
{
    using namespace CryptoPP;

    ECDSA<ECP, SHA256>::PublicKey publicKey;
    publicKey.Initialize(
        ASN1::secp256k1(),
        CryptoPP::ECP::Point(
            CryptoPP::Integer("83326269377737301187045338455478996967104803243941757917076354219390730898031"),
            CryptoPP::Integer("108911706275326467973600132368983151825997206660859431906025905780521963107049")
            ));

    AutoSeededRandomPool prng;
    std::cout << "Valid: " << !!publicKey.Validate(prng, 3) << std::endl;

    return 0;
}

Crypto++ 5.6.2, 8.0 and Master each validate the public key.

guidovranken commented

Apologies, I was misreading my debug output. The pubkey is ok but the signature S is larger than the curve order and should be rejected as far as I can tell.

noloader commented

but the signature S is larger than the curve order and should be rejected as far as I can tell.

OK, so it looks like S is being reduced prior to Verify. Here's the Verify function from gfpcrypt.h : 304. Verify performs the check.

bool Verify(const DL_GroupParameters<T> &params, const DL_PublicKey<T> &publicKey, const Integer &e, const Integer &r, const Integer &s) const
{
    const Integer &q = params.GetSubgroupOrder();
    if (r>=q || r<1 || s>=q || s<1)
        return false;

    Integer w = s.InverseMod(q);
    Integer u1 = (e * w) % q;
    Integer u2 = (r * w) % q;
    // verify r == (g^u1 * y^u2 mod p) mod q
    return r == params.ConvertElementToInteger(publicKey.CascadeExponentiateBaseAndPublicElement(u1, u2)) % q;
}

Tracing back in the call stack leads to pubkey.h : 1729 and VerifyAndRestart. S is reduced when it is initialized ma.m_s, which is a PK_MessageAccumulator-based class:

bool VerifyAndRestart(PK_MessageAccumulator &messageAccumulator) const
{
    this->GetMaterial().DoQuickSanityCheck();

    PK_MessageAccumulatorBase &ma = static_cast<PK_MessageAccumulatorBase &>(messageAccumulator);
    const DL_ElgamalLikeSignatureAlgorithm<T> &alg = this->GetSignatureAlgorithm();
    const DL_GroupParameters<T> &params = this->GetAbstractGroupParameters();
    const DL_PublicKey<T> &key = this->GetKeyInterface();

    SecByteBlock representative(this->MessageRepresentativeLength());
    this->GetMessageEncodingInterface().ComputeMessageRepresentative(NullRNG(), ma.m_recoverableMessage, ma.m_recoverableMessage.size(),
        ma.AccessHash(), this->GetHashIdentifier(), ma.m_empty,
        representative, this->MessageRepresentativeBitLength());
    ma.m_empty = true;
    Integer e(representative, representative.size());

    Integer r(ma.m_semisignature, ma.m_semisignature.size());
    return alg.Verify(params, key, e, r, ma.m_s);
}

I don't see an easy way around this.

@mouse07410, any thoughts?

Here's the external checks that can be added to check r and s:

#include "eccrypto.h"
#include "ecp.h"
#include "oids.h"
#include "osrng.h"

#include <iostream>

int main(void)
{
    using namespace CryptoPP;

    ECDSA<ECP, SHA256>::PublicKey publicKey;
    publicKey.Initialize(
        ASN1::secp256k1(),
        CryptoPP::ECP::Point(
            CryptoPP::Integer("83326269377737301187045338455478996967104803243941757917076354219390730898031"),
            CryptoPP::Integer("108911706275326467973600132368983151825997206660859431906025905780521963107049")
            ));

    AutoSeededRandomPool prng;
    std::cout << "Group: " << !!publicKey.GetAbstractGroupParameters().Validate(prng, 3) << std::endl;
    std::cout << "Key: " << !!publicKey.Validate(prng, 3) << std::endl;

    ECDSA<ECP, SHA256>::Verifier verifier(publicKey);

    const CryptoPP::Integer R("58459610944154385406267492095069703630366579530687393858060946682600281547621");
    const CryptoPP::Integer S("166425580247629610000042226331335729376739920034019158271667950393311552549306");

    if (R >= publicKey.GetGroupParameters().GetSubgroupOrder())
        std::cout << "R is too large" << std::endl;
    else
        std::cout << "R is OK" << std::endl;

    if (S >= publicKey.GetGroupParameters().GetSubgroupOrder())
        std::cout << "S is too large" << std::endl;
    else
        std::cout << "S is OK" << std::endl;

    uint8_t ct[] = {0x31, 0x32, 0x33, 0x34, 0x30, 0x30};
    const size_t siglen = verifier.SignatureLength();
    uint8_t signature[siglen];

    R.Encode(signature + 0, siglen / 2);
    S.Encode(signature + (siglen / 2), siglen / 2);
    
    std::cout << "Verify: " << !!verifier.VerifyMessage(ct, sizeof(ct), signature, siglen) << std::endl;

    return 0;
}
mouse07410 commented

In ECDSA in general, if the hash size is greater than the P, it's truncated.

Usually it's done when the signature is created - but I see no obvious reason why Verify shouldn't do that. Though I'd be less comfortable with truncation on Verify.

Haven't checked the code - it should be truncation rather than modular reduction.

noloader commented

Yeah, it looks like DL_VerifierBase lops-off s:

void InputSignature(PK_MessageAccumulator &messageAccumulator, const byte *signature, size_t signatureLength) const
{
    CRYPTOPP_UNUSED(signature); CRYPTOPP_UNUSED(signatureLength);
    PK_MessageAccumulatorBase &ma = static_cast<PK_MessageAccumulatorBase &>(messageAccumulator);
    const DL_ElgamalLikeSignatureAlgorithm<T> &alg = this->GetSignatureAlgorithm();
    const DL_GroupParameters<T> &params = this->GetAbstractGroupParameters();

    const size_t rLen = alg.RLen(params);
    ma.m_semisignature.Assign(signature, rLen);
    ma.m_s.Decode(signature+rLen, alg.SLen(params));

    this->GetMessageEncodingInterface().ProcessSemisignature(ma.AccessHash(), ma.m_semisignature, ma.m_semisignature.size());
}

I wonder if we can safely perform a check that verifies signatureLength == rLen + sLen.

noloader commented

I wonder if we can safely perform a check that verifies signatureLength == rLen + sLen.

The answer to this question appears to be no. Running cryptest.exe v shows the NIST test vectors violate the condition.

This is a kind of unexpected result. When I change:

const size_t rLen = alg.RLen(params);
ma.m_semisignature.Assign(signature, rLen);
ma.m_s.Decode(signature+rLen, alg.SLen(params));

To:

ma.m_semisignature.Assign(signature, rLen);
ma.m_s.Decode(signature+rLen, signatureLength - rLen);
std::cout << " O:  " << std::hex << params.GetSubgroupOrder() << std::endl;
std::cout << " S: " << std::hex << ma.m_s << std::endl;

Then the result is:

 O: fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141h
 S: 6ff18a52dcc0336f7af62400a6dd9b810732baf1ff758000d6f613a556eb31bah

So S is getting lopped-off before InputSignature. I suspect it is happening when the PK_MessageAccumulator is being filled. I'll dig a little further this afternoon when I get some time.

noloader commented

So S is getting lopped-off before InputSignature.

Doh... S is being lopped-off when it is encoded:

const size_t siglen = verifier.SignatureLength();
uint8_t signature[siglen];

R.Encode(signature + 0, siglen / 2);
S.Encode(signature + (siglen / 2), siglen / 2);

S.MinEncodedSize() will return a value larger than siglen / 2. When we add the following to the test program:

std::cout << "Min size: " << S.MinEncodedSize() << std::endl;
std::cout << "s length: " << siglen / 2 << std::endl;

It results in:

Min size: 21
s length: 20