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

CMS_decrypt*(): fix misconceptions and memory leak - backport to v3.0 and 3.1 #20209

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Feb 3, 2023

This is a backport of #19222 as motivated by #19222 (comment).
It also includes the fixup done in #19981.

Also document CMS_decrypt_set1_password() and fix CMS_EnvelopedData_create.pod.
Fixes openssl#19975
for CMS_decrypt_set1_pkey_and_peer() in the obvious way,
and a related potential crash in CMS_decrypt_set1_password().

The point is that the input might have an unexpected content type,
so a guard is needed at both places after `ec` is obtained.

Note that in CMS_decrypt_set1_pkey_and_peer() there was
no such ec != NULL guard for
```
    if (ris != NULL)
        debug = ec->debug;
```
maybe because it is implied here by ris != NULL.
@DDvO DDvO added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors) branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Feb 3, 2023
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Feb 6, 2023
doc/man3/CMS_decrypt.pod Outdated Show resolved Hide resolved
doc/man3/CMS_decrypt.pod Outdated Show resolved Hide resolved
doc/man3/CMS_decrypt.pod Outdated Show resolved Hide resolved
doc/man3/CMS_decrypt.pod Show resolved Hide resolved
doc/man3/CMS_decrypt.pod Outdated Show resolved Hide resolved
@paulidale paulidale 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 Feb 23, 2023
@tmshort
Copy link
Contributor

tmshort commented Feb 23, 2023

AFAICT, the CI failures are not relevant?

@DDvO
Copy link
Contributor Author

DDvO commented Feb 23, 2023

AFAICT, the CI failures are not relevant?

Yep, since months I see one or two if those Windows runs fail on most PRs,
for whatever reason.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@DDvO DDvO 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 Feb 24, 2023
openssl-machine pushed a commit that referenced this pull request Feb 24, 2023
Also document CMS_decrypt_set1_password() and fix CMS_EnvelopedData_create.pod.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #20209)
openssl-machine pushed a commit that referenced this pull request Feb 24, 2023
…ecryption key

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #20209)
openssl-machine pushed a commit that referenced this pull request Feb 24, 2023
…ent mismatch was not the issue

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #20209)
openssl-machine pushed a commit that referenced this pull request Feb 24, 2023
Fixes #19975
for CMS_decrypt_set1_pkey_and_peer() in the obvious way,
and a related potential crash in CMS_decrypt_set1_password().

The point is that the input might have an unexpected content type,
so a guard is needed at both places after `ec` is obtained.

Note that in CMS_decrypt_set1_pkey_and_peer() there was
no such ec != NULL guard for
```
    if (ris != NULL)
        debug = ec->debug;
```
maybe because it is implied here by ris != NULL.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #20209)
openssl-machine pushed a commit that referenced this pull request Feb 24, 2023
Also document CMS_decrypt_set1_password() and fix CMS_EnvelopedData_create.pod.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #20209)

(cherry picked from commit 26521fa)
openssl-machine pushed a commit that referenced this pull request Feb 24, 2023
…ecryption key

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #20209)

(cherry picked from commit c5febe9)
openssl-machine pushed a commit that referenced this pull request Feb 24, 2023
…ent mismatch was not the issue

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #20209)

(cherry picked from commit 27d8739)
openssl-machine pushed a commit that referenced this pull request Feb 24, 2023
Fixes #19975
for CMS_decrypt_set1_pkey_and_peer() in the obvious way,
and a related potential crash in CMS_decrypt_set1_password().

The point is that the input might have an unexpected content type,
so a guard is needed at both places after `ec` is obtained.

Note that in CMS_decrypt_set1_pkey_and_peer() there was
no such ec != NULL guard for
```
    if (ris != NULL)
        debug = ec->debug;
```
maybe because it is implied here by ris != NULL.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #20209)

(cherry picked from commit ceb767b)
@DDvO
Copy link
Contributor Author

DDvO commented Feb 24, 2023

Merged to 3.0 and cherry-picked to 3.1 - thanks @t8m and @paulidale for the approvals.

@DDvO DDvO closed this Feb 24, 2023
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 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 triaged: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants