Skip to content

backport #21933 to 3.0 branch #23102

Closed
Liu-ErMeng wants to merge 6 commits intoopenssl:openssl-3.0from
Liu-ErMeng:openssl-3.0
Closed

backport #21933 to 3.0 branch #23102
Liu-ErMeng wants to merge 6 commits intoopenssl:openssl-3.0from
Liu-ErMeng:openssl-3.0

Conversation

@Liu-ErMeng
Copy link
Copy Markdown
Contributor

@Liu-ErMeng Liu-ErMeng commented Dec 20, 2023

Checklist
  • documentation is added or updated
  • tests are added or updated

relate to #23018

@Liu-ErMeng
Copy link
Copy Markdown
Contributor Author

Liu-ErMeng commented Dec 20, 2023

@nhorman , help! is the fips-provider-validation test failed caused by newly added code snippet in evp_test.c?

test/evp_test.c Outdated
int ok = 0, tmplen, chunklen, tmpflen, i;
EVP_CIPHER_CTX *ctx_base = NULL;
EVP_CIPHER_CTX *ctx = NULL;
int fips_dupctx_supported = (fips_provider_version_ge(libctx, 3, 0, 11)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fips_provider_version_gt(libctx, 3, 0, 12)

Comment on lines +840 to 846
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an else case where you do:

        EVP_CIPHER_CTX_free(ctx_base);
        ctx_base = NULL;

@t8m
Copy link
Copy Markdown
Member

t8m commented Dec 20, 2023

@nhorman , help! is the fips-provider-validation test failed caused by newly added code snippet in evp_test.c?

Yes the CI is relevant.

@t8m t8m added triaged: bug The issue/pr is/fixes a bug branch: 3.0 Applies to openssl-3.0 branch tests: present The PR has suitable tests present labels Dec 20, 2023
@nhorman
Copy link
Copy Markdown
Contributor

nhorman commented Dec 20, 2023

Will look as soon as.i get to my desk

@nhorman
Copy link
Copy Markdown
Contributor

nhorman commented Dec 20, 2023

Ack to @t8m comments, those changes should fix the ci

There should be no reason that a cipher can't be duplicated

Fixes openssl#21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#21933)
Add dupctx method support to to ciphers implemented with IMPLEMENT_aead_cipher
This includes:
aes-<kbits>-gcm
aria-<kbits>-ccm
aria-<kbits>-gcm

Fixes openssl#21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#21933)
create a dupctx method for aes_WRAP implementations of all sizes

Fixes openssl#21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#21933)
Same as chacha20 in the last commit, just clone the ctx and its
underlying tlsmac array if its allocated

Fixes openssl#21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#21933)
Pretty straightforward, just clone the requested context, no pointers to
fixup

Fixes openssl#21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#21933)
In the dupctx fixups I missed a pointer that needed to be repointed to
the surrounding structures AES_KEY structure for the sm4/aes/aria
ccm/gcm variants.  This caused a colliding use of the key and possible
use after free issues.

Fixes openssl#22076

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#22102)
@Liu-ErMeng
Copy link
Copy Markdown
Contributor Author

@nhorman , help! is the fips-provider-validation test failed caused by newly added code snippet in evp_test.c?

Yes the CI is relevant.

thx, I will fix these.

@Liu-ErMeng Liu-ErMeng requested a review from t8m December 21, 2023 11:15
@Liu-ErMeng
Copy link
Copy Markdown
Contributor Author

@nhorman , help! is the fips-provider-validation test failed caused by newly added code snippet in evp_test.c?

Yes the CI is relevant.

Hi @t8m , I have fix these issues. Please review again.

@t8m t8m added the approval: review pending This pull request needs review by a committer label Dec 22, 2023
@Liu-ErMeng Liu-ErMeng requested a review from t8m January 2, 2024 01:37
@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 2, 2024
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 3, 2024
@openssl-machine
Copy link
Copy Markdown
Collaborator

This pull request is ready to merge

@t8m t8m removed the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jan 3, 2024
@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Jan 4, 2024
@nhorman nhorman linked an issue Jan 4, 2024 that may be closed by this pull request
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 5, 2024
@openssl-machine
Copy link
Copy Markdown
Collaborator

This pull request is ready to merge

@t8m
Copy link
Copy Markdown
Member

t8m commented Jan 5, 2024

Merged to the 3.0 branch. Thank you for your contribution.

@t8m t8m closed this Jan 5, 2024
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
There should be no reason that a cipher can't be duplicated

Fixes #21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
Add dupctx method support to to ciphers implemented with IMPLEMENT_aead_cipher
This includes:
aes-<kbits>-gcm
aria-<kbits>-ccm
aria-<kbits>-gcm

Fixes #21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
create a dupctx method for aes_WRAP implementations of all sizes

Fixes #21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
Same as chacha20 in the last commit, just clone the ctx and its
underlying tlsmac array if its allocated

Fixes #21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
Pretty straightforward, just clone the requested context, no pointers to
fixup

Fixes #21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
In the dupctx fixups I missed a pointer that needed to be repointed to
the surrounding structures AES_KEY structure for the sm4/aes/aria
ccm/gcm variants.  This caused a colliding use of the key and possible
use after free issues.

Fixes #22076

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)
@t8m t8m added the branch: 3.1 Applies to openssl-3.1 (EOL) label Jan 5, 2024
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
There should be no reason that a cipher can't be duplicated

Fixes #21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)

(cherry picked from commit 58a6aa0)
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
Add dupctx method support to to ciphers implemented with IMPLEMENT_aead_cipher
This includes:
aes-<kbits>-gcm
aria-<kbits>-ccm
aria-<kbits>-gcm

Fixes #21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)

(cherry picked from commit 879a853)
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
create a dupctx method for aes_WRAP implementations of all sizes

Fixes #21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)

(cherry picked from commit a5bea0a)
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
Same as chacha20 in the last commit, just clone the ctx and its
underlying tlsmac array if its allocated

Fixes #21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)

(cherry picked from commit e7ef50c)
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
Pretty straightforward, just clone the requested context, no pointers to
fixup

Fixes #21887

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)

(cherry picked from commit f9163ef)
openssl-machine pushed a commit that referenced this pull request Jan 5, 2024
In the dupctx fixups I missed a pointer that needed to be repointed to
the surrounding structures AES_KEY structure for the sm4/aes/aria
ccm/gcm variants.  This caused a colliding use of the key and possible
use after free issues.

Fixes #22076

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23102)

(cherry picked from commit 0398bc2)
@adrien-n
Copy link
Copy Markdown

I am seeing test failures on 3.0.13 which are related to this merge request.

For the testsuite to pass, I have to revert the following commits:

@rsbeckerca
Copy link
Copy Markdown
Contributor

shlibtest needs to be disabled during the back-port.

@t8m
Copy link
Copy Markdown
Member

t8m commented Feb 14, 2024

I am seeing test failures on 3.0.13 which are related to this merge request.

For the testsuite to pass, I have to revert the following commits:

* [58a6aa0](https://github.com/openssl/openssl/commit/58a6aa0c9fe6abad996f45c6b452983035db7105)

* [879a853](https://github.com/openssl/openssl/commit/879a853a1dc968fb010e5bf17d2e8888acc70742)

* [0398bc2](https://github.com/openssl/openssl/commit/0398bc20080de037a8433fe81cfdef3ba0ec9d4c)

As we do not see the failures in our CI could you please open a new issue and report the failures that you see?

@t8m
Copy link
Copy Markdown
Member

t8m commented Feb 14, 2024

@adrien-n ^

@adrien-n
Copy link
Copy Markdown

Sorry for not answering earlier; GH needs a reaction that amounts to "ack, thanks for your answer, I'm currently looking into it and it might take me some time".

I also didn't answer earlier because I had a weird feeling following the comments here. It turns out that the issue happens only when we also patch in a backport of at least one of commits 8a75952ba829784f5cb499c4883a5729c226e38c or 949108dd73de321fb93c8d81b846a2a1d015a9fd . Right now I see several possible causes:

  • the CPU support patches,
  • the backport of these patches,
  • the combination of these patches and the patches I mentioned earlier,
  • something else.

@t8m I'll therefore open a bug when I can tell which one of these possibilities it is in order to make a proper one; this is taking a while as the builds are LTO (including tests) (and, checking whether the builds should be LTO is already scheduled in the coming weeks).

@adrien-n
Copy link
Copy Markdown

I forgot to mention that these CPU feature patches require fairly recent hardware which is always annoying with Intel due to market segmentation but my laptop surprisingly has the features. On the AMD side, Zen 4 is needed and I fortunately have such a machine too now where the behavior is the same. Anyway, I guess hardware support is the reason why something (whatever that is) might have gone un-noticed.

kafei-cy added a commit to kafei-cy/Tongsuo that referenced this pull request Apr 8, 2026
Include 7 commits:
1. make inability to dup/clone ciphers an error
2. Add dupctx support to aead ciphers
3. implement dupctx for aes_WRAP methods
4. implement dupctx for chacha20_poly1305
5. Add dupctx support to rc4_hmac_md5 algo
6. Fix a key repointing in various ciphers
7. Also with SM4 for Tongsuo, delete some codes tend to CI error.

(Merged from openssl/openssl#23102)
kafei-cy added a commit to kafei-cy/Tongsuo that referenced this pull request Apr 8, 2026
Include 7 commits:
1. make inability to dup/clone ciphers an error
2. Add dupctx support to aead ciphers
3. implement dupctx for aes_WRAP methods
4. implement dupctx for chacha20_poly1305
5. Add dupctx support to rc4_hmac_md5 algo
6. Fix a key repointing in various ciphers
7. Also with SM4 for Tongsuo, delete some codes tend to CI error.

(Merged from openssl/openssl#23102)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Backport #21933 to 3.0 branch

7 participants