Fix BIO_f_cipher flushing and other BIO related fixes#19918
Closed
mattcaswell wants to merge 6 commits intoopenssl:masterfrom
Closed
Fix BIO_f_cipher flushing and other BIO related fixes#19918mattcaswell wants to merge 6 commits intoopenssl:masterfrom
mattcaswell wants to merge 6 commits intoopenssl:masterfrom
Conversation
mattcaswell
added
branch: master
This was referenced Dec 15, 2022
Member
Author
|
I am confused by the test failures.... |
If an error occurs during a flush on a BIO_f_cipher() then in some cases we could get into an infinite loop. We add a check to make sure we are making progress during flush and exit if not. This issue was reported by Octavio Galland who also demonstrated an infinite loop in CMS encryption as a result of this bug. The security team has assessed this issue as not a CVE. This occurs on *encryption* only which is typically processing trusted data. We are not aware of a way to trigger this with untrusted data.
If the BIO unexpectedly fails to flush then SMIME_crlf_copy() was not correctly reporting the error. We modify it to properly propagate the error condition.
Some things that may go wrong in asn1_bio_write() are serious errors that should be reported as -1, rather than 0 (which just means "we wrote no data").
If the cipher being used in ossl_cms_EncryptedContent_init_bio() has no associated OID then we should report an error rather than continuing on regardless. Continuing on still ends up failing - but later on and with a more cryptic error message.
mattcaswell
force-pushed
the
fix-bio-handling
branch
from
89754cd to
754d694
Compare
Member
Author
Interesting. This seems to be due to a bad interaction with #19652 which was merged after my local branch containing this fix was created. Github Actions automatically merge a PR with the master branch during the CI run and there were no conflicts but having both in the tree at the same time causes a test failure because of a bad plan (17 tests were run when it expected 16). Rebasing my local branch reproduced the issue. Fixup pushed. |
slontis
reviewed
Dec 15, 2022
| } | ||
| } | ||
| (void)BIO_flush(out); | ||
| ret = BIO_flush(out); |
Member
There was a problem hiding this comment.
I dont know if it matters but there are quite a few other (void)BIO_flush calls related to cms/pkcs7
t8m
added
triaged: bug
t8m
approved these changes
Dec 16, 2022
hlandau
approved these changes
Dec 16, 2022
hlandau
added
approval: done
Collaborator
|
24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually. |
openssl-machine
added
approval: ready to merge
Collaborator
|
This pull request is ready to merge |
Member
|
Merged to master branch. Thank you. |
openssl-machine
pushed a commit
that referenced
this pull request
Dec 22, 2022
If an error occurs during a flush on a BIO_f_cipher() then in some cases we could get into an infinite loop. We add a check to make sure we are making progress during flush and exit if not. This issue was reported by Octavio Galland who also demonstrated an infinite loop in CMS encryption as a result of this bug. The security team has assessed this issue as not a CVE. This occurs on *encryption* only which is typically processing trusted data. We are not aware of a way to trigger this with untrusted data. Reviewed-by: Hugo Landau <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19918)
openssl-machine
pushed a commit
that referenced
this pull request
Dec 22, 2022
If the BIO unexpectedly fails to flush then SMIME_crlf_copy() was not correctly reporting the error. We modify it to properly propagate the error condition. Reviewed-by: Hugo Landau <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19918)
openssl-machine
pushed a commit
that referenced
this pull request
Dec 22, 2022
Some things that may go wrong in asn1_bio_write() are serious errors that should be reported as -1, rather than 0 (which just means "we wrote no data"). Reviewed-by: Hugo Landau <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19918)
openssl-machine
pushed a commit
that referenced
this pull request
Dec 22, 2022
If the cipher being used in ossl_cms_EncryptedContent_init_bio() has no associated OID then we should report an error rather than continuing on regardless. Continuing on still ends up failing - but later on and with a more cryptic error message. Reviewed-by: Hugo Landau <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19918)
openssl-machine
pushed a commit
that referenced
this pull request
Dec 22, 2022
Reviewed-by: Hugo Landau <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19918)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If an error occurs during a flush on a BIO_f_cipher() then in some cases
we could get into an infinite loop. We add a check to make sure we are
making progress during flush and exit if not.
This issue was reported by Octavio Galland who also demonstrated an
infinite loop in CMS encryption as a result of this bug.
The security team has assessed this issue as not a CVE. This occurs on
encryption only which is typically processing trusted data. We are not
aware of a way to trigger this with untrusted data.