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.

Sign up for GitHub

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

Jump to bottom

Fix ssl3_do_write() to correctly handle retries (3.1/3.0) #21435

Closed
wants to merge 2 commits into from
Closed

Fix ssl3_do_write() to correctly handle retries (3.1/3.0) #21435

wants to merge 2 commits into from

Conversation

mattcaswell
Copy link
Member

A BIO is documented to return -1 on write retry - but sometimes they return
0. ssl3_do_write() was incorrectly handling a 0 response.

Fixes #21422

This is a backport of #21434 for the 3.1/3.0 branches.

@mattcaswell
Add a test for a retry during the handshake
137f0f5 
Test various scenarios for a write retry occuring during a handshake.
@mattcaswell
Fix ssl3_do_write() to correctly handle retries
0cfa504 
A BIO is documented to return -1 on write retry - but sometimes they return
0. ssl3_do_write() was incorrectly handling a 0 response.

Fixes openssl#21422
@mattcaswell mattcaswell 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 branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Jul 12, 2023
@mattcaswell
Copy link
Member Author

mattcaswell commented Jul 12, 2023 •
edited

Actually this is only really a problem in the 3.1/3.0 branches. By accident of the record layer refactor this doesn't actually impact the master branch. But the code is still wrong so I think we should apply the fix here anyway.
Ooops...meant to put this comment in #21434

@mattcaswell mattcaswell mentioned this pull request Jul 12, 2023
Closed
@paulidale paulidale added tests: present The PR has suitable tests present and removed approval: otc review pending This pull request needs review by an OTC member labels Jul 13, 2023
@mattcaswell
Copy link
Member Author

Ping for second review

beldmit
beldmit approved these changes Jul 14, 2023
View reviewed changes
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM

@beldmit beldmit 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 Jul 14, 2023
@beldmit
Copy link
Member

beldmit commented Jul 14, 2023

A BIO is documented to return -1 on write retry - but sometimes they return 0

Can we do anything with this behavior? It looks quite counter intuitive...

@mattcaswell
Copy link
Member Author

Can we do anything with this behavior? It looks quite counter intuitive...

Well not really. This is mostly about third party BIOs. You could call it a bug in those BIOs because it doesn't match the documentation - but in reality I think this behaviour has worked for a long time. If there are any of our own BIOs that do the wrong thing then we could fix those, but I haven't checked.

@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.

@paulidale
Copy link
Contributor

Merged to both 3.x branches.

@paulidale paulidale closed this Jul 16, 2023
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
@mattcaswell @paulidale
Add a test for a retry during the handshake
51af5f2 
Test various scenarios for a write retry occuring during a handshake.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21435)

(cherry picked from commit d6179e6)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
@mattcaswell @paulidale
Fix ssl3_do_write() to correctly handle retries
44ba390 
A BIO is documented to return -1 on write retry - but sometimes they return
0. ssl3_do_write() was incorrectly handling a 0 response.

Fixes #21422

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21435)

(cherry picked from commit 034ea1d)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
@mattcaswell @paulidale
Add a test for a retry during the handshake
d6179e6 
Test various scenarios for a write retry occuring during a handshake.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21435)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2023
@mattcaswell @paulidale
Fix ssl3_do_write() to correctly handle retries
034ea1d 
A BIO is documented to return -1 on write retry - but sometimes they return
0. ssl3_do_write() was incorrectly handling a 0 response.

Fixes #21422

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21435)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jul 19, 2023
@mattcaswell @bernd-edlinger
Add a test for a retry during the handshake
56840ed 
Test various scenarios for a write retry occuring during a handshake.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#21435)

(cherry picked from commit d6179e6)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jul 19, 2023
@mattcaswell @bernd-edlinger
Fix ssl3_do_write() to correctly handle retries
54a58e9 
A BIO is documented to return -1 on write retry - but sometimes they return
0. ssl3_do_write() was incorrectly handling a 0 response.

Fixes openssl#21422

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#21435)

(cherry picked from commit 034ea1d)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jul 19, 2023
@mattcaswell @bernd-edlinger
Add a test for a retry during the handshake
93d9dbb 
Test various scenarios for a write retry occuring during a handshake.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#21435)

(cherry picked from commit d6179e6)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jul 19, 2023
@mattcaswell @bernd-edlinger
Fix ssl3_do_write() to correctly handle retries
6818343 
A BIO is documented to return -1 on write retry - but sometimes they return
0. ssl3_do_write() was incorrectly handling a 0 response.

Fixes openssl#21422

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#21435)

(cherry picked from commit 034ea1d)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Aug 1, 2023
@mattcaswell @bernd-edlinger
Add a test for a retry during the handshake
affb5b0 
Test various scenarios for a write retry occuring during a handshake.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#21435)

(cherry picked from commit d6179e6)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Aug 1, 2023
@mattcaswell @bernd-edlinger
Fix ssl3_do_write() to correctly handle retries
6397955 
A BIO is documented to return -1 on write retry - but sometimes they return
0. ssl3_do_write() was incorrectly handling a 0 response.

Fixes openssl#21422

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#21435)

(cherry picked from commit 034ea1d)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Aug 19, 2023
@mattcaswell @bernd-edlinger
Add a test for a retry during the handshake
73a8cb1 
Test various scenarios for a write retry occuring during a handshake.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#21435)

(cherry picked from commit d6179e6)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Aug 19, 2023
@mattcaswell @bernd-edlinger
Fix ssl3_do_write() to correctly handle retries
a20e0be 
A BIO is documented to return -1 on write retry - but sometimes they return
0. ssl3_do_write() was incorrectly handling a 0 response.

Fixes openssl#21422

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#21435)

(cherry picked from commit 034ea1d)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Sep 12, 2023
@mattcaswell @bernd-edlinger
Add a test for a retry during the handshake
b2bad99 
Test various scenarios for a write retry occuring during a handshake.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#21435)

(cherry picked from commit d6179e6)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Sep 12, 2023
@mattcaswell @bernd-edlinger
Fix ssl3_do_write() to correctly handle retries
edc85cb 
A BIO is documented to return -1 on write retry - but sometimes they return
0. ssl3_do_write() was incorrectly handling a 0 response.

Fixes openssl#21422

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#21435)

(cherry picked from commit 034ea1d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beldmit beldmit beldmit approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: present The PR has suitable tests present
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mattcaswell @beldmit @openssl-machine @paulidale