Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EVP_DigestSign fails when called second time in 3.2.0 #23075

Closed
orignal opened this issue Dec 18, 2023 · 23 comments
Closed

EVP_DigestSign fails when called second time in 3.2.0 #23075

orignal opened this issue Dec 18, 2023 · 23 comments
Assignees
Labels
branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 triaged: documentation The issue/pr deals with documentation (errors)

Comments

@orignal
Copy link

orignal commented Dec 18, 2023

I need sign series of message with EdDSA.
I create EVP_PKEY from a private key, then initialize EVP_MD_CTX using EVP_DigestSignInit and then call EVP_DigestSign multiple times for each message.
It worked fine in 3.1.0 and EVP_DigestSign fails when called second time in 3.2.0.
See the code below, I'm trying to sign and verify result using a test vector from RFC 8032.
It's successive after the first call and fails after the second call.

```
#include <cassert>
#include <inttypes.h>
#include <string.h>

#include <openssl/evp.h>

int main ()
{
    // ED25519 test vector TEST 1024 from RFC 8032
    uint8_t key[32],  msg[1024], sig[64];
    BIGNUM * input = BN_new();
    BN_hex2bn(&input, "f5e5767cf153319517630f226876b86c8160cc583bc013744c6bf255f5cc0ee5");
    BN_bn2bin(input, key);
    BN_hex2bn(&input, 
    "08b8b2b733424243760fe426a4b54908632110a66c2f6591eabd3345e3e4eb98"
    "fa6e264bf09efe12ee50f8f54e9f77b1e355f6c50544e23fb1433ddf73be84d8"
    "79de7c0046dc4996d9e773f4bc9efe5738829adb26c81b37c93a1b270b20329d"
    "658675fc6ea534e0810a4432826bf58c941efb65d57a338bbd2e26640f89ffbc"
    "1a858efcb8550ee3a5e1998bd177e93a7363c344fe6b199ee5d02e82d522c4fe"
    "ba15452f80288a821a579116ec6dad2b3b310da903401aa62100ab5d1a36553e"
    "06203b33890cc9b832f79ef80560ccb9a39ce767967ed628c6ad573cb116dbef"
    "efd75499da96bd68a8a97b928a8bbc103b6621fcde2beca1231d206be6cd9ec7"
    "aff6f6c94fcd7204ed3455c68c83f4a41da4af2b74ef5c53f1d8ac70bdcb7ed1"
    "85ce81bd84359d44254d95629e9855a94a7c1958d1f8ada5d0532ed8a5aa3fb2"
    "d17ba70eb6248e594e1a2297acbbb39d502f1a8c6eb6f1ce22b3de1a1f40cc24"
    "554119a831a9aad6079cad88425de6bde1a9187ebb6092cf67bf2b13fd65f270"
    "88d78b7e883c8759d2c4f5c65adb7553878ad575f9fad878e80a0c9ba63bcbcc"
    "2732e69485bbc9c90bfbd62481d9089beccf80cfe2df16a2cf65bd92dd597b07"
    "07e0917af48bbb75fed413d238f5555a7a569d80c3414a8d0859dc65a46128ba"
    "b27af87a71314f318c782b23ebfe808b82b0ce26401d2e22f04d83d1255dc51a"
    "ddd3b75a2b1ae0784504df543af8969be3ea7082ff7fc9888c144da2af58429e"
    "c96031dbcad3dad9af0dcbaaaf268cb8fcffead94f3c7ca495e056a9b47acdb7"
    "51fb73e666c6c655ade8297297d07ad1ba5e43f1bca32301651339e22904cc8c"
    "42f58c30c04aafdb038dda0847dd988dcda6f3bfd15c4b4c4525004aa06eeff8"
    "ca61783aacec57fb3d1f92b0fe2fd1a85f6724517b65e614ad6808d6f6ee34df"
    "f7310fdc82aebfd904b01e1dc54b2927094b2db68d6f903b68401adebf5a7e08"
    "d78ff4ef5d63653a65040cf9bfd4aca7984a74d37145986780fc0b16ac451649"
    "de6188a7dbdf191f64b5fc5e2ab47b57f7f7276cd419c17a3ca8e1b939ae49e4"
    "88acba6b965610b5480109c8b17b80e1b7b750dfc7598d5d5011fd2dcc5600a3"
    "2ef5b52a1ecc820e308aa342721aac0943bf6686b64b2579376504ccc493d97e"
    "6aed3fb0f9cd71a43dd497f01f17c0e2cb3797aa2a2f256656168e6c496afc5f"
    "b93246f6b1116398a346f1a641f3b041e989f7914f90cc2c7fff357876e506b5"
    "0d334ba77c225bc307ba537152f3f1610e4eafe595f6d9d90d11faa933a15ef1"
    "369546868a7f3a45a96768d40fd9d03412c091c6315cf4fde7cb68606937380d"
    "b2eaaa707b4c4185c32eddcdd306705e4dc1ffc872eeee475a64dfac86aba41c"
    "0618983f8741c5ef68d3a101e8a3b8cac60c905c15fc910840b94c00a0b9d0"
    );
    BN_bn2bin(input, msg);
    BN_hex2bn(&input, 
    "0aab4c900501b3e24d7cdf4663326a3a87df5e4843b2cbdb67cbf6e460fec350"
    "aa5371b1508f9f4528ecea23c436d94b5e8fcd4f681e30a6ac00a9704a188a03");
    BN_bn2bin(input, sig);

    EVP_PKEY * pkey = EVP_PKEY_new_raw_private_key (EVP_PKEY_ED25519, NULL, key, 32);
    assert (pkey != NULL);

    EVP_MD_CTX * ctx = EVP_MD_CTX_create ();
    assert (ctx != NULL);
    assert (EVP_DigestSignInit (ctx, NULL, NULL, NULL, pkey));

    // try to sign and compare with expected signature
    size_t l = 64; uint8_t s[64];
    assert (EVP_DigestSign (ctx, s, &l, msg, 1023));
    assert (memcmp (s, sig, 64) == 0);
    // success

    // try again
    assert (EVP_DigestSign (ctx, s, &l, msg, 1023));
    // failed
}
```
@orignal orignal added the issue: bug report The issue was opened to report a bug label Dec 18, 2023
@nhorman
Copy link
Contributor

nhorman commented Dec 18, 2023

Looks like this was introduced as part of commit 3fc2b7d , in which EVP_MD_CTX's were marked as finalized to prevent them from being reused without re-initalization. The proper call flow would be to insert a new call to EVP_DigestSignInit between the two EVP_DigestSign calls.

This may constitute a regression though

@nhorman nhorman added severity: regression The issue/pr is a regression from previous released version branch: 3.2 Merge to openssl-3.2 labels Dec 18, 2023
@mattcaswell mattcaswell added triaged: bug The issue/pr is/fixes a bug hold: need otc decision The OTC needs to make a decision and removed issue: bug report The issue was opened to report a bug labels Dec 18, 2023
@mattcaswell
Copy link
Member

mattcaswell commented Dec 18, 2023

Hmmm. This was introduced as a deliberate decision to force incorrect reuse of a ctx without re-initialising them. It's unclear though whether this is really a regression or incorrect use of the API. Adding an OTC hold to discuss this.

OTC Question: Is it a regression that we now require the EVP_MD_CTX to be reinitialised between EVP_DigestSign calls?

@t8m
Copy link
Member

t8m commented Dec 18, 2023

Did it produce correct signatures? I.e. the same as if DigestSignInit() was called in between?

@nhorman
Copy link
Contributor

nhorman commented Dec 18, 2023

@t8m , it didn't get that far. The output message from the second call is correct, but only because it was correct from the first call and unmodified in the second. The second call errored out before making any modifications, due to the first call setting EVP_MD_CTX_FLAG_FINALISED in the ctx flags

@orignal
Copy link
Author

orignal commented Dec 18, 2023

What's a rationale to reinitialize the context for each signature if the key is not changing?

@t8m
Copy link
Member

t8m commented Dec 18, 2023

@nhorman I mean not with the 3.2.0 but with previous versions where there was no check for that flag.

@t8m
Copy link
Member

t8m commented Dec 18, 2023

What's a rationale to reinitialize the context for each signature if the key is not changing?

The expected sequence with hash-then-sign algorithms which allow streaming is

EVP_DigestSignInit->EVP_DigestSignUpdate->...->EVP_DigestSignFinal

The expected sequence with algorithms which do not allow streaming is

EVP_DigestSignInit->EVP_DigestSign

However this sequence can be used with streaming algorithms as well and it leaves the digest in the finalized state. It cannot get into the initialized state without calling EVP_DigestSignInit again.

That it worked before to call EVP_DigestSign multiple times was just by chance.

@t8m t8m added the branch: master Merge to master branch label Dec 18, 2023
@orignal
Copy link
Author

orignal commented Dec 18, 2023

I was under impression that EVP_DigestSign is just a single call for Init/Update/Final, e.g. EVP_DigestSignInit should be called inside EVP_DigestSign if necessary.

@nhorman
Copy link
Contributor

nhorman commented Dec 18, 2023

@t8m ah, I can try this afternoon, but I expect it would, though seemingly only by happenstance, given that the input buffers are unchanged, and the output buffer is re-written

@orignal
Copy link
Author

orignal commented Dec 18, 2023

Second call with different input causes wrong signature, that's how we have found the problem.

@nhorman
Copy link
Contributor

nhorman commented Dec 18, 2023

@orignal so you're saying that in 3.1, it was still broken, just in a different way? I.e. it completed successfully, but produced incorrect outputs? If so, then this seems like this isn't a regression, just a correction to properly report the error

@orignal
Copy link
Author

orignal commented Dec 18, 2023

@orignal so you're saying that in 3.1, it was still broken, just in a different way?

No, everything was fine in 3.1. The problem exists in 3.2 only. Even if you don't check the return code of EVP_DigestSign, it still produces wrong signature being called second time.

@t8m
Copy link
Member

t8m commented Dec 19, 2023

It does not produce any signature in 3.2.0, it just returns failure.

algitbot pushed a commit to alpinelinux/aports that referenced this issue Dec 19, 2023
Add version constraint openssl-dev<3.2.0 due to
openssl/openssl#23075
@t8m
Copy link
Member

t8m commented Jan 9, 2024

OTC: We need to investigate whether the original code produced valid signatures in the subsequent EVP_DigestSign() calls.

@levitte
Copy link
Member

levitte commented Jan 9, 2024

The documentation for EVP_DigestSign() has this sentence:

In the event of a failure EVP_DigestSign() cannot be called again without reinitialising the EVP_MD_CTX.

Doesn't this suggest that if there was no error, it can be called again without reinitializing?

Note: further down in the documentation, there is this:

The call to EVP_DigestSignFinal() internally finalizes a copy of the digest context. This means that calls to EVP_DigestSignUpdate() and EVP_DigestSignFinal() can be called later to digest and sign additional data.

It's documented that EVP_DigestSign() essentially finishes off the same way as EVP_DigestSignFinal(), so the docs leave it a bit unclear that there's now a need to call EVP_DigestSignInit() again.

@t8m t8m assigned t8m and levitte and unassigned t8m Jan 30, 2024
@t8m t8m removed the hold: need otc decision The OTC needs to make a decision label Jan 30, 2024
@t8m
Copy link
Member

t8m commented Jan 30, 2024

Removing the OTC hold until the investigation is done.

@slontis
Copy link
Member

slontis commented Feb 22, 2024

I performed these Tests:
In 3.1 the test described above was behaving in a deterministic way.
Note that I reset the signature between calls to make sure and checked that the result was the same the second time.
I also tried writing tests for DSA and ECDSA to see how they worked also.

For X25519 multiple calls to EVP_DigestSign() resulted in the same signature output being produced.
For ECDSA multiple calls to EVP_DigestSign() resulted in different output being produced (internally it uses the same md_ctx for the update). This was verified by forcing the k to be deterministic (all zeros), and checking that the signature of the second call to EVP_DigestSign() was the same as passing the input repeated once to the first call to EVP_DigestSign().
DSA was found to behave exactly like ECDSA does above for multiple calls.

Debugging the 3.1 code:

In 3.1. the code for EVP_DigestSign() has 2 pathways for non legacy

        if (pctx->op.sig.signature->digest_sign != NULL) {
            ....
            return pctx->op.sig.signature->digest_sign(pctx->op.sig.algctx, ....... );
        }
        OR
        ....
        EVP_DigestSignUpdate(ctx, tbs, tbslen);
        return EVP_DigestSignFinal(ctx, sigret, siglen);
}

For ECDSA it takes the EVP_DigestSignUpdate() and EVP_DigestSignFinal() path
For X25519 it takes the ->digest_sign() path.

X25519 is a bit 'special' because it handles it digests internally (since it is a hardwired name)
i.e.

ossl_ed25519_sign()
{
   .....
   EVP_MD *sha512 = EVP_MD_fetch(libctx, SN_sha512, propq);
   EVP_MD_CTX *hash_ctx = EVP_MD_CTX_new();
}

And this is why it handles multiple calls in this case -since the hash_ctx is created each time.

In later versions the following code was added to the EVP_DigestSign() which obviously makes this break with multiple calls.

    if ((ctx->flags & EVP_MD_CTX_FLAG_FINALISED) != 0) {
        ERR_raise(ERR_LIB_EVP, EVP_R_FINAL_ERROR);
        return 0;
    }

I think because of this 'undefined behavior' it would be better to leave this alone and require the DigestInit to happen again.
Now that we support deterministic signatures, the old behavior seems like a bug to me.

This new check was added in #20375.
@simo5 as the author of this PR, perhaps you have an opinion on this issue?

@t8m
Copy link
Member

t8m commented Feb 22, 2024

@slontis Thank you for this thorough analysis. Given the different behavior for different algos on 3.1 we should IMO keep the new behavior as is - i.e., fail if EVP_DigestSignInit is not called. We should probably document it explicitly.

@t8m t8m added the hold: need otc decision The OTC needs to make a decision label Feb 22, 2024
@t8m
Copy link
Member

t8m commented Feb 22, 2024

OTC: What is the preferred resolution given the analysis above?

@simo5
Copy link
Contributor

simo5 commented Feb 22, 2024

@slontis Yeah I am of the opinion that initialization should always be required.

Although OpenSSL internal default provider can cope with reuse in some cases, that is not a given for all providers (and some will probably not be able to), nor for all signature schemes that could be added in the future.

I do not believe the Init() function to be such an overhead that would warrant keeping an ambiguous behavior, if performance is an issue I would rather focus on optimizing the Init() function so that it has as low re-init overhead as possible within specific provider implementations rather than leaving ambiguous and potentially unsafe behavior available.

I guess the semantics of EVP_DigestSign could be change to transparently call a re-init internally, I am not sure it is worth it though, it will complicate the code and cause double calls to provider init functions in the default case, which may fail to work with current providers.

@orignal
Copy link
Author

orignal commented Feb 22, 2024

It's not an overhead, but it might break the functionality of existing project, since this requirement was not mentioned in the specs before.

@simo5
Copy link
Contributor

simo5 commented Feb 22, 2024

It's not an overhead, but it might break the functionality of existing project, since this requirement was not mentioned in the specs before.

Understood, but it was totally undefined behavior, so a hard choice should be made in this case.

@mattcaswell
Copy link
Member

OTC: It is correct that EVP_DigestSign fails when called a second time. This behaviour should be documented.

@mattcaswell mattcaswell added triaged: documentation The issue/pr deals with documentation (errors) and removed hold: need otc decision The OTC needs to make a decision triaged: bug The issue/pr is/fixes a bug severity: regression The issue/pr is a regression from previous released version labels Mar 12, 2024
slontis added a commit to slontis/openssl that referenced this issue Mar 14, 2024
Fixes openssl#23075

In OpenSSL 3.2 EVP_DigestSign and EVP_DigestVerify
were changed so that a flag is set once these functions
do a one-shot sign or verify operation. This PR updates the
documentation to match the behaviour.

Investigations showed that prior to 3.2 different key
type behaved differently if multiple calls were done.

By accident X25519 and X448 would produce the same signature,
but ECDSA and RSA remembered the digest state between calls,
so the signature was different when multiple calls were done.

Because of this undefined behaviour something needed to be done,
so keeping the 'only allow it to be called once' behaviour
seems a reasonable approach.
openssl-machine pushed a commit that referenced this issue Apr 4, 2024
Fixes #23075

In OpenSSL 3.2 EVP_DigestSign and EVP_DigestVerify
were changed so that a flag is set once these functions
do a one-shot sign or verify operation. This PR updates the
documentation to match the behaviour.

Investigations showed that prior to 3.2 different key
type behaved differently if multiple calls were done.

By accident X25519 and X448 would produce the same signature,
but ECDSA and RSA remembered the digest state between calls,
so the signature was different when multiple calls were done.

Because of this undefined behaviour something needed to be done,
so keeping the 'only allow it to be called once' behaviour
seems a reasonable approach.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23834)

(cherry picked from commit 5e908e6)
openssl-machine pushed a commit that referenced this issue Apr 4, 2024
Fixes #23075

In OpenSSL 3.2 EVP_DigestSign and EVP_DigestVerify
were changed so that a flag is set once these functions
do a one-shot sign or verify operation. This PR updates the
documentation to match the behaviour.

Investigations showed that prior to 3.2 different key
type behaved differently if multiple calls were done.

By accident X25519 and X448 would produce the same signature,
but ECDSA and RSA remembered the digest state between calls,
so the signature was different when multiple calls were done.

Because of this undefined behaviour something needed to be done,
so keeping the 'only allow it to be called once' behaviour
seems a reasonable approach.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23834)

(cherry picked from commit 5e908e6)
jvdsn pushed a commit to jvdsn/openssl that referenced this issue Jun 3, 2024
Fixes openssl#23075

In OpenSSL 3.2 EVP_DigestSign and EVP_DigestVerify
were changed so that a flag is set once these functions
do a one-shot sign or verify operation. This PR updates the
documentation to match the behaviour.

Investigations showed that prior to 3.2 different key
type behaved differently if multiple calls were done.

By accident X25519 and X448 would produce the same signature,
but ECDSA and RSA remembered the digest state between calls,
so the signature was different when multiple calls were done.

Because of this undefined behaviour something needed to be done,
so keeping the 'only allow it to be called once' behaviour
seems a reasonable approach.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#23834)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 triaged: documentation The issue/pr deals with documentation (errors)
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants