Skip to content

bio: fix wrong wbio pointer on SSL_get_error#20919

Closed
ihciah wants to merge 1 commit intoopenssl:masterfrom
ihciah:master
Closed

bio: fix wrong wbio pointer on SSL_get_error#20919
ihciah wants to merge 1 commit intoopenssl:masterfrom
ihciah:master

Conversation

@ihciah
Copy link
Contributor

@ihciah ihciah commented May 10, 2023

This PR fixed a wrong wbio pointer usage on SSL_get_error.
The bug will cause SSL_get_error to return SSL_ERROR_SYSCALL even after BIO_set_retry_write in some cases such as BIO_CTRL_FLUSH. Without this fix, when BIO_CTRL_FLUSH, if BIO writer gets a WOULD_BLOCK error, it calls BIO_set_retry_write and returns 0, it will not work and openssl will return a failure.

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label May 10, 2023
t8m
t8m requested changes May 10, 2023
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

This would be acceptable with CLA: trivial. Please see https://www.openssl.org/policies/cla.html

@t8m t8m requested a review from mattcaswell May 10, 2023 08:47
@t8m t8m added branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug labels May 10, 2023
@ihciah ihciah mentioned this pull request May 10, 2023
Open
@ihciah
Copy link
Contributor Author

ihciah commented May 10, 2023

In bio_ssl.c it indeed uses wbio. It is strange ctrl receives wrong BIO pointer...

    case BIO_CTRL_FLUSH:
        BIO_clear_retry_flags(b);
        ret = BIO_ctrl(sc->wbio, cmd, num, ptr);
        BIO_copy_next_retry(b);
        break;

@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label May 10, 2023
@ihciah
Copy link
Contributor Author

ihciah commented May 10, 2023

I added some BIO_copy_next_retry(b); after BIO_ctrl, it should make SSL_get_error work correctly. Is this fix acceptable?
I also found many of the same issue in different files and places, should I fix them all in this PR?

@t8m
Copy link
Member

t8m commented May 10, 2023

Could you please split these two different changes in separate commits as they are unrelated?

@t8m
Copy link
Member

t8m commented May 10, 2023

Also the typo in quic call is master branch only thing but the other fix should probably go into all 3.x branches (no need to do a separate PR but definitely separate commit should be done).

@mattcaswell
Copy link
Member

mattcaswell commented May 10, 2023

After some investigation I found the cause is not passing flags between bios. So I fixed it in another way and it works now.

So the original issue has been resolved. I will close this PR.

@ihciah
Copy link
Contributor Author

ihciah commented May 10, 2023

After some investigation I found the cause is not passing flags between bios. So I fixed it in another way and it works now.

So the original issue has been resolved. I will close this PR.

@mattcaswell "in another way" means passing flags between bios, it should be done in openssl, as the PR does. I've force pushed the branch which maybe you missed it.

@mattcaswell mattcaswell reopened this May 10, 2023
@mattcaswell
Copy link
Member

mattcaswell commented May 10, 2023

Ah. My apologies. I misunderstood. I've reopened it and will take another look.

@ihciah
Copy link
Contributor Author

ihciah commented May 10, 2023

If the fix is right, I'm willing to trying fix all similar issue in this PR :)

mattcaswell
mattcaswell reviewed May 11, 2023
View reviewed changes
crypto/bio/bf_buff.c
}
}
ret = BIO_ctrl(b->next_bio, cmd, num, ptr);
BIO_copy_next_retry(b);
Copy link
Member

@mattcaswell mattcaswell May 11, 2023

Choose a reason for hiding this comment

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

The blanket calls to BIO_copy_next_retry() for all possible IO ctrls does not seem correct to me. Most of those ctrls are not "IO" related ctrls so applications would not expect the "retry" the status to change as a result. The only one where it would is in BIO_CTRL_FLUSH. So I would keep the two you have inserted in this section and remove all the others.

Copy link
Contributor Author

@ihciah ihciah May 11, 2023

Choose a reason for hiding this comment

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

@mattcaswell
Thanks! I have removed all other insertions. In the new pushed commit I also added some for other BIO_CTRL_FLUSH. Is it correct?

Copy link
Contributor Author

@ihciah ihciah May 25, 2023

Choose a reason for hiding this comment

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

ping~ @mattcaswell

@ihciah
bio: pass flags on BIO_ctrl to make flush retriable
74d8e0e 
Co-authored-by: suikammd <suikalala@gmail.com>

CLA: trivial
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label May 17, 2023
@ihciah
Copy link
Contributor Author

ihciah commented Jun 20, 2023

Ping @t8m @mattcaswell Can it be merged?

@paulidale paulidale added the cla: trivial One of the commits is marked as 'CLA: trivial' label Jun 20, 2023
@paulidale
Copy link
Contributor

paulidale commented Jun 20, 2023

Two approvals are required before it can be merged.
This seems to have become lost in the noise.

@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Jun 21, 2023
@t8m
Copy link
Member

t8m commented Jun 23, 2023

@mattcaswell are you OK with CLA: trivial?

Also, I wonder, should we special-case this in BIO_ctrl() actually? I.e. instead of patching all BIOs which might be also third party ones so they won't get the fix, should we instead do it automatically when BIO_ctrl() returns and the BIO_CTRL_FLUSH is used?

@mattcaswell
Copy link
Member

mattcaswell commented Jun 23, 2023

Actually I think this maybe goes beyond CLA trivial. I think we should have a CLA for this one.

Also, I wonder, should we special-case this in BIO_ctrl() actually? I.e. instead of patching all BIOs which might be also third party ones so they won't get the fix, should we instead do it automatically when BIO_ctrl() returns and the BIO_CTRL_FLUSH is used?

Hmm. Maybe, but that's quite ugly. It doesn't actually apply to all BIOs, only filter BIOs (because source/sink BIOs don't have a "next"). And I wonder if there are special cases where you wouldn't want this to happen.

@ihciah
Copy link
Contributor Author

ihciah commented Jun 25, 2023

I've signed and sent the CLA pdf(also with @suikammd which is another co-auther).

@ihciah ihciah mentioned this pull request Jun 27, 2023
Closed
@ihciah
Copy link
Contributor Author

ihciah commented Jun 27, 2023

Close in favor of #21298

@ihciah ihciah closed this Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t8m t8m t8m requested changes

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees

No one assigned

Labels

approval: review pending This pull request needs review by a committer branch: master Applies to master branch cla: trivial One of the commits is marked as 'CLA: trivial' severity: fips change The pull request changes FIPS provider sources triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ihciah @t8m @mattcaswell @paulidale @openssl-machine