fix check of broken implementations of GOST ciphersuites

[deemru]

This patch moves the current check of "broken implementations of GOST ciphersuites" to a correct place and adds new GOST 2012 nids to this check.

[deemru]

Because it is a GOST only issue, @beldmit are welcomed.

[beldmit]

The proposed patch seems wrong to me, because

1. GOST ciphersuites match the check SSL_USE_SIGALGS
2. All the known implementations of the new ciphersuites do not have this bug, so checks for the GOST2012 are redundant.

[deemru]

Current issue is for all GOSTs TLS < 1.2 (see the fixed comment in code)

SSL_USE_SIGALGS is valid for TLS 1.2, but GOST2012 (CryptoPro implementations) can work on TLS 1.0

[deemru]

In any case, the place of original check is wrong, because md variable is never filled.

[beldmit]

Have you checked your patch against at least the CryptoPro implementation?

[deemru]

CryptoPro GOST 2012 client authentications have 64 and 128 bytes bare signatures without length fields if TLS 1.2 (sigalgs) does not used.

Currently i am working on GOST engine for OpenSSL 1.1.0 with GOST 2012 support. Everything seems fine except this client authentication code. Even CryptoPro GOST 2001 does not work without this patch because of empty md in EVP_VerifyInit_ex() below.

[beldmit]

And what problems do you have with the existing engine?

[deemru]

I did not try https://github.com/gost-engine/engine (if you mean it), but it does not matter.

That is looks better?

#ifndef OPENSSL_NO_GOST
        if (!SSL_USE_SIGALGS(s)
            && ((PACKET_remaining(pkt) == 64
             && (EVP_PKEY_id(pkey) == NID_id_GostR3410_2001
                 || EVP_PKEY_id(pkey) == NID_id_GostR3410_2012_256))
            || (PACKET_remaining(pkt) == 128
                && EVP_PKEY_id(pkey) == NID_id_GostR3410_2012_512))) {
            len = PACKET_remaining(pkt);
        } else
#endif

[beldmit]

For now we agreed to get more tests for this code, both current implementation and the proposed patch.

[deemru]

Unfortunately, the code for creating the client's signature in all versions of CryptoPro CSP is looks like this:

<...>
if (pCPContext->session->dwTLSVersion == SP_PROT_TLS1_2_CLIENT)
{
    pbSig = (BYTE*)AllocMemory(dwSigLen + 4);
    <...>
    memcpy(pbSig + 4, old, dwSigLen);
    dwSigLen += 4;
}
SecStatus = ssl3_put_message (pCPContext, pbSig, dwSigLen, SSL3_MT_CERTIFICATE_VERIFY);
<...>

As you can see, regardless of an algorithm when using TLS not equal 1.2 a message with a client's signature will be a bare signature without length field.

Fortunately, a CryptoPro server-side code verification of client signatures supports messages with a length field and without.

To test this issue you'll need CryptoPro CSP with 3 certificates with NID_id_GostR3410_2001, NID_id_GostR3410_2012_256, NID_id_GostR3410_2012_512 keys, and an appropriate openssl s_server with client verify.

Then if you run:

csptest -tlsc -server <hostname> -port 4433 -user <certname> -proto 4

-proto 4 stands for TLS 1.0, you can use -proto 5 (TLS 1.1) with same effect: you'll get errors without this patch on all 3 certificates, even with NID_id_GostR3410_2001. And you'll get no errors with this patch.

TLS 1.2 (-proto 6 or by default) works well with and without this patch:

csptest -tlsc -server <hostname> -port 4433 -user <certname> -proto 6

[richsalz]

CLA received, removing tag.

[beldmit]

Seems ok to me.

[richsalz]

Based on @beldmit's review.

[richsalz]

maybe 1.0.2 also?

[deemru]

no patch is needed for 1.0.2:

• NID_id_GostR3410_2001 works correct: https://github.com/openssl/openssl/blob/OpenSSL_1_0_2-stable/ssl/s3_srvr.c#L3047
• there is no GOST 2012 nids at all: https://github.com/openssl/openssl/blob/OpenSSL_1_0_2-stable/crypto/objects/obj_mac.h

[richsalz]

Thanks for the information

[mattcaswell]

This PR is targeting 1.1.0, but presumably the same change needs to be applied to master. However this PR does not apply cleanly to master, so we need another PR for there.

[mattcaswell]

It looks like we could lose this { and everything below moved to the left.

[deemru]

That is right, but i was afraid of massive changes, fixed.

[mattcaswell]

@richsalz are you still ok with this?

[richsalz]

Yes

[richsalz]

Thanks for your patience on this, and work to improve openssl! Merged with commit a892766 in 1.1.0